From c9269f9bc04bc764dc0c9f71054e19f549f1a47a Mon Sep 17 00:00:00 2001 From: Antonin Bas Date: Mon, 26 Feb 2018 15:30:01 -0800 Subject: [PATCH] Wrap indirect resource index in proto message in P4Runtime 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 --- proto/demo_grpc/simple_router_mgr.cpp | 5 ++- proto/frontend/src/device_mgr.cpp | 58 ++++++++++++++++++++------- proto/p4/p4runtime.proto | 19 +++++++-- proto/tests/test_proto_fe.cpp | 16 ++++++-- 4 files changed, 75 insertions(+), 23 deletions(-) diff --git a/proto/demo_grpc/simple_router_mgr.cpp b/proto/demo_grpc/simple_router_mgr.cpp index d3ed0ab5..88bb51e2 100644 --- a/proto/demo_grpc/simple_router_mgr.cpp +++ b/proto/demo_grpc/simple_router_mgr.cpp @@ -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; @@ -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(rep_entry.index()) == index) { + static_cast(rep_entry.index().index()) == index) { counter_data->CopyFrom(rep_entry.data()); return 0; } diff --git a/proto/frontend/src/device_mgr.cpp b/proto/frontend/src/device_mgr.cpp index 7cbd6eff..ff7ec99f 100644 --- a/proto/frontend/src/device_mgr.cpp +++ b/proto/frontend/src/device_mgr.cpp @@ -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(meter_entry.index().index()); switch (update) { case p4::Update_Type_UNSPECIFIED: RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT); @@ -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"); @@ -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"); @@ -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; } @@ -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(counter_entry.index().index()); switch (update) { case p4::Update_Type_UNSPECIFIED: RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT); @@ -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"); @@ -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"); @@ -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( @@ -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; } @@ -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(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, @@ -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(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); diff --git a/proto/p4/p4runtime.proto b/proto/p4/p4runtime.proto index 7588c2c1..2ac2ca14 100644 --- a/proto/p4/p4runtime.proto +++ b/proto/p4/p4runtime.proto @@ -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; } @@ -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; } diff --git a/proto/tests/test_proto_fe.cpp b/proto/tests/test_proto_fe.cpp index c7cda4fe..aef2a293 100644 --- a/proto/tests/test_proto_fe.cpp +++ b/proto/tests/test_proto_fe.cpp @@ -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}; }; @@ -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 @@ -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}; }; @@ -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) @@ -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)); } }