Skip to content

Commit

Permalink
Wrap indirect resource index in proto message in P4Runtime
Browse files Browse the repository at this point in the history 
Wrap indirect resource index in proto message in P4Runtime

This ensures that we can distinguish between an unset index (wildcard
read / write) and a zero index (first entry in the resource index).

Fixes #309
  • Loading branch information
@antoninbas
antoninbas committed Feb 26, 2018
1 parent 6901098 commit c9269f9
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 23 deletions.
5 changes: 3 additions & 2 deletions proto/demo_grpc/simple_router_mgr.cpp
View file
Expand Up @@ -653,7 +653,8 @@ SimpleRouterMgr::query_counter_(const std::string &counter_name, size_t index,
auto entity = request.add_entities();
auto counter_entry = entity->mutable_counter_entry();
counter_entry->set_counter_id(counter_id);
counter_entry->set_index(index);
auto index_msg = counter_entry->mutable_index();
index_msg->set_index(index);

p4::ReadResponse rep;
ClientContext context;
Expand All @@ -662,7 +663,7 @@ SimpleRouterMgr::query_counter_(const std::string &counter_name, size_t index,
for (const auto &entity : rep.entities()) {
const auto &rep_entry = entity.counter_entry();
if (rep_entry.counter_id() == counter_id &&
static_cast<size_t>(rep_entry.index()) == index) {
static_cast<size_t>(rep_entry.index().index()) == index) {
counter_data->CopyFrom(rep_entry.data());
return 0;
}
Expand Down
58 changes: 44 additions & 14 deletions proto/frontend/src/device_mgr.cpp
View file
Expand Up @@ -427,6 +427,16 @@ class DeviceMgrImp {
const SessionTemp &session) {
if (!check_p4_id(meter_entry.meter_id(), P4ResourceType::METER))
return make_invalid_p4_id_status();
if (!meter_entry.has_index()) {
RETURN_ERROR_STATUS(
Code::UNIMPLEMENTED,
"Wildcard write is not supported for indirect meters yet");
}
if (meter_entry.index().index() < 0) {
RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT,
"A negative number is not a valid index value");
}
auto index = static_cast<size_t>(meter_entry.index().index());
switch (update) {
case p4::Update_Type_UNSPECIFIED:
RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT);
Expand All @@ -439,7 +449,7 @@ class DeviceMgrImp {
meter_entry.config(), meter_entry.meter_id());
auto pi_status = pi_meter_set(session.get(), device_tgt,
meter_entry.meter_id(),
meter_entry.index(),
index,
&pi_meter_spec);
if (pi_status != PI_STATUS_SUCCESS)
RETURN_ERROR_STATUS(Code::UNKNOWN, "Error when writing meter spec");
Expand All @@ -451,7 +461,7 @@ class DeviceMgrImp {
{0, 0, 0, 0, PI_METER_UNIT_DEFAULT, PI_METER_TYPE_DEFAULT};
auto pi_status = pi_meter_set(session.get(), device_tgt,
meter_entry.meter_id(),
meter_entry.index(),
index,
&pi_meter_spec);
if (pi_status != PI_STATUS_SUCCESS)
RETURN_ERROR_STATUS(Code::UNKNOWN, "Error when writing meter spec");
Expand Down Expand Up @@ -550,19 +560,22 @@ class DeviceMgrImp {
p4::ReadResponse *response) const {
assert(pi_p4info_meter_get_direct(p4info.get(), meter_id) ==
PI_INVALID_ID);
if (meter_entry.index() != 0) {
if (meter_entry.has_index()) {
if (meter_entry.index().index() < 0) {
RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT,
"A negative number is not a valid index value");
}
auto entry = response->add_entities()->mutable_meter_entry();
entry->CopyFrom(meter_entry);
return meter_read_one_index(session, meter_id, entry);
}
// default index, read all
// TODO(antonin): change this code when we introduce Index wrapper message
// (see issue #309)
auto meter_size = pi_p4info_meter_get_size(p4info.get(), meter_id);
for (size_t index = 0; index < meter_size; index++) {
auto entry = response->add_entities()->mutable_meter_entry();
entry->set_meter_id(meter_id);
entry->set_index(index);
auto index_msg = entry->mutable_index();
index_msg->set_index(index);
auto status = meter_read_one_index(session, meter_id, entry);
if (IS_ERROR(status)) return status;
}
Expand Down Expand Up @@ -1049,6 +1062,16 @@ class DeviceMgrImp {
const SessionTemp &session) {
if (!check_p4_id(counter_entry.counter_id(), P4ResourceType::COUNTER))
return make_invalid_p4_id_status();
if (!counter_entry.has_index()) {
RETURN_ERROR_STATUS(
Code::UNIMPLEMENTED,
"Wildcard write is not supported for indirect counters yet");
}
if (counter_entry.index().index() < 0) {
RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT,
"A negative number is not a valid index value");
}
auto index = static_cast<size_t>(counter_entry.index().index());
switch (update) {
case p4::Update_Type_UNSPECIFIED:
RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT);
Expand All @@ -1061,7 +1084,7 @@ class DeviceMgrImp {
counter_entry.data(), counter_entry.counter_id());
auto pi_status = pi_counter_write(session.get(), device_tgt,
counter_entry.counter_id(),
counter_entry.index(),
index,
&pi_counter_data);
if (pi_status != PI_STATUS_SUCCESS)
RETURN_ERROR_STATUS(Code::UNKNOWN, "Error when writing to counter");
Expand All @@ -1073,7 +1096,7 @@ class DeviceMgrImp {
{PI_COUNTER_UNIT_PACKETS | PI_COUNTER_UNIT_BYTES, 0u, 0u};
auto pi_status = pi_counter_write(session.get(), device_tgt,
counter_entry.counter_id(),
counter_entry.index(),
index,
&pi_counter_data);
if (pi_status != PI_STATUS_SUCCESS)
RETURN_ERROR_STATUS(Code::UNKNOWN, "Error when writing to counter");
Expand Down Expand Up @@ -1155,14 +1178,16 @@ class DeviceMgrImp {
p4::ReadResponse *response) const {
assert(pi_p4info_counter_get_direct(p4info.get(), counter_id) ==
PI_INVALID_ID);
if (counter_entry.index() != 0) {
if (counter_entry.has_index()) {
if (counter_entry.index().index() < 0) {
RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT,
"A negative number is not a valid index value");
}
auto entry = response->add_entities()->mutable_counter_entry();
entry->CopyFrom(counter_entry);
return counter_read_one_index(session, counter_id, entry, true);
}
// default index, read all
// TODO(antonin): change this code when we introduce Index wrapper message
// (see issue #309)
auto counter_size = pi_p4info_counter_get_size(p4info.get(), counter_id);
{ // sync the entire counter array with HW
auto pi_status = pi_counter_hw_sync(
Expand All @@ -1173,7 +1198,8 @@ class DeviceMgrImp {
for (size_t index = 0; index < counter_size; index++) {
auto entry = response->add_entities()->mutable_counter_entry();
entry->set_counter_id(counter_id);
entry->set_index(index);
auto index_msg = entry->mutable_index();
index_msg->set_index(index);
auto status = counter_read_one_index(session, counter_id, entry);
if (IS_ERROR(status)) return status;
}
Expand Down Expand Up @@ -1837,7 +1863,9 @@ class DeviceMgrImp {
Status counter_read_one_index(const SessionTemp &session, uint32_t counter_id,
p4::CounterEntry *entry,
bool hw_sync = false) const {
auto index = entry->index();
// checked by caller
assert(entry->has_index() && entry->index().index() >= 0);
auto index = static_cast<size_t>(entry->index().index());
int flags = hw_sync ? PI_COUNTER_FLAGS_HW_SYNC : PI_COUNTER_FLAGS_NONE;
pi_counter_data_t counter_data;
pi_status_t pi_status = pi_counter_read(session.get(), device_tgt,
Expand All @@ -1853,7 +1881,9 @@ class DeviceMgrImp {

Status meter_read_one_index(const SessionTemp &session, uint32_t meter_id,
p4::MeterEntry *entry) const {
auto index = entry->index();
// checked by caller
assert(entry->has_index() && entry->index().index() >= 0);
auto index = static_cast<size_t>(entry->index().index());
pi_meter_spec_t meter_spec;
pi_status_t pi_status = pi_meter_read(session.get(), device_tgt,
meter_id, index, &meter_spec);
Expand Down
19 changes: 15 additions & 4 deletions proto/p4/p4runtime.proto
View file
Expand Up @@ -241,14 +241,25 @@ message ActionProfileGroup {
int32 max_size = 4; // cannot be modified after a group has been created
}

// An index as a protobuf message. In proto3, fields cannot be optional and
// there is no difference between an unset integer field and an integer field
// set to 0. This is inconvenient for reading from P4 array-like structures,
// such as indirect counters and meters. We need a way to do a wildcard read on
// these but we cannot use a default zero index value to do so, as zero is a
// valid index (first entry in the array). We therefore wrap the index in a
// message.
message Index {
int64 index = 1;
}

//------------------------------------------------------------------------------
// For WriteRequest, Update.Type must be MODIFY.
// For ReadRequest, the scope is defined as follows:
// - All meter cells for all meters if meter_id = 0 (default).
// - All meter cells for given meter_id if index = 0 (default).
// - All meter cells for given meter_id if index is unset (default).
message MeterEntry {
uint32 meter_id = 1;
int64 index = 2;
Index index = 2;
MeterConfig config = 3;
}

Expand Down Expand Up @@ -290,10 +301,10 @@ message MeterConfig {
// For WriteRequest, Update.Type must be MODIFY.
// For ReadRequest, the scope is defined as follows:
// - All counter cells for all meters if counter_id = 0 (default).
// - All counter cells for given counter_id if index = 0 (default).
// - All counter cells for given counter_id if index is unset (default).
message CounterEntry {
uint32 counter_id = 1;
int64 index = 2;
Index index = 2;
CounterData data = 3;
}

Expand Down
16 changes: 13 additions & 3 deletions proto/tests/test_proto_fe.cpp
View file
Expand Up @@ -1452,6 +1452,11 @@ class IndirectMeterTest : public DeviceMgrTest {
return config;
}

void set_index(p4::MeterEntry *meter_entry, int index) const {
auto *index_msg = meter_entry->mutable_index();
index_msg->set_index(index);
}

pi_p4_id_t m_id{0};
size_t m_size{0};
};
Expand All @@ -1461,7 +1466,7 @@ TEST_F(IndirectMeterTest, WriteAndRead) {
p4::ReadResponse response;
p4::MeterEntry meter_entry;
meter_entry.set_meter_id(m_id);
meter_entry.set_index(index);
set_index(&meter_entry, index);
auto meter_config = make_meter_config();
meter_entry.mutable_config()->CopyFrom(meter_config);
// meter type & unit as per the P4 program
Expand Down Expand Up @@ -1666,6 +1671,11 @@ class IndirectCounterTest : public DeviceMgrTest {
return status;
}

void set_index(p4::CounterEntry *counter_entry, int index) const {
auto *index_msg = counter_entry->mutable_index();
index_msg->set_index(index);
}

pi_p4_id_t c_id{0};
size_t c_size{0};
};
Expand All @@ -1675,7 +1685,7 @@ TEST_F(IndirectCounterTest, WriteAndRead) {
p4::ReadResponse response;
p4::CounterEntry counter_entry;
counter_entry.set_counter_id(c_id);
counter_entry.set_index(index);
set_index(&counter_entry, index);
auto *counter_data = counter_entry.mutable_data();
counter_data->set_packet_count(3);
// check packets, but not bytes, as per P4 program (packet-only counter)
Expand Down Expand Up @@ -1715,7 +1725,7 @@ TEST_F(IndirectCounterTest, ReadAll) {
counter_data->set_packet_count(0);
for (size_t i = 0; i < c_size; i++) {
const auto &entry = entities.Get(i).counter_entry();
counter_entry.set_index(i);
set_index(&counter_entry, i);
ASSERT_TRUE(MessageDifferencer::Equals(counter_entry, entry));
}
}
Expand Down

0 comments on commit c9269f9

Please sign in to comment.