[P4-PDPI] Add entity-level convenience functions for reading entries …

…and deprecate table entry-level counterparts.Refactor SplitUpWriteRequest into ExtractWriteRequets to avoid copies.[PDPI] Add utility functions for determining if a field has a p4 runtime translated type.[p4rt session] Reduce log spam. (sonic-net#508) Co-authored-by: kishanps <[email protected]> Co-authored-by: smolkaj <[email protected]>
divyagayathri-hcl · Aug 29, 2024 · 6d37e7d · 6d37e7d
1 parent 286ce86
commit 6d37e7d
Show file tree

Hide file tree

Showing 10 changed files with 235 additions and 45 deletions.
diff --git a/p4_pdpi/BUILD.bazel b/p4_pdpi/BUILD.bazel
@@ -432,6 +432,26 @@ cc_library(
     ],
 )
 
+cc_library(
+    name = "ir_properties",
+    srcs = ["ir_properties.cc"],
+    hdrs = ["ir_properties.h"],
+    deps = [
+        ":ir_cc_proto",
+        "@com_github_p4lang_p4runtime//:p4info_cc_proto",
+    ],
+)
+
+cc_test(
+    name = "ir_properties_test",
+    srcs = ["ir_properties_test.cc"],
+    deps = [
+        ":ir_properties",
+        "//gutil:testing",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 cc_library(
     name = "ir_tools",
     srcs = ["ir_tools.cc"],

diff --git a/p4_pdpi/ir_properties.cc b/p4_pdpi/ir_properties.cc
@@ -0,0 +1,19 @@
+#include "p4_pdpi/ir_properties.h"
+
+#include "p4/config/v1/p4info.pb.h"
+#include "p4_pdpi/ir.pb.h"
+
+namespace pdpi {
+
+bool HasP4RuntimeTranslatedType(const IrMatchFieldDefinition& match_field) {
+  return match_field.match_field().has_type_name() &&
+         match_field.format() == pdpi::Format::STRING;
+}
+
+bool HasP4RuntimeTranslatedType(
+    const IrActionDefinition::IrActionParamDefinition& param) {
+  return param.param().has_type_name() &&
+         param.format() == pdpi::Format::STRING;
+}
+
+}  // namespace pdpi
diff --git a/p4_pdpi/ir_properties.h b/p4_pdpi/ir_properties.h
@@ -0,0 +1,19 @@
+#ifndef PINS_P4_PDPI_IR_PROPERTIES_H_
+#define PINS_P4_PDPI_IR_PROPERTIES_H_
+
+#include "p4_pdpi/ir.pb.h"
+
+namespace pdpi {
+
+// Returns true if `match_field` has a P4Runtime-translated translated type,as
+// expressed through an @p4runtime_translation annotation.
+bool HasP4RuntimeTranslatedType(const IrMatchFieldDefinition& match_field);
+
+// Returns true if `param` has a P4Runtime-translated type, as expressed through
+// an @p4runtime_translation annotation
+bool HasP4RuntimeTranslatedType(
+    const IrActionDefinition::IrActionParamDefinition& param);
+
+}  // namespace pdpi
+
+#endif  // PINS_P4_PDPI_IR_PROPERTIES_H_
diff --git a/p4_pdpi/ir_properties_test.cc b/p4_pdpi/ir_properties_test.cc
@@ -0,0 +1,76 @@
+#include "p4_pdpi/ir_properties.h"
+
+#include "gtest/gtest.h"
+#include "gutil/testing.h"
+
+namespace pdpi {
+namespace {
+
+using gutil::ParseProtoOrDie;
+
+TEST(HasP4RuntimeTranslatedType, P4RuntimeTranslatedTypeMatchFieldReturnsTrue) {
+  EXPECT_FALSE(
+      HasP4RuntimeTranslatedType(ParseProtoOrDie<IrMatchFieldDefinition>(
+          R"pb(
+            match_field { id: 1 name: "dragon_field" match_type: EXACT }
+            format: HEX_STRING
+          )pb")));
+
+  EXPECT_FALSE(
+      HasP4RuntimeTranslatedType(ParseProtoOrDie<IrMatchFieldDefinition>(
+          R"pb(
+            match_field {
+              id: 1
+              name: "dragon_field"
+              match_type: EXACT
+              type_name { name: "dragon_type" }
+            }
+            format: HEX_STRING
+          )pb")));
+
+  EXPECT_TRUE(
+      HasP4RuntimeTranslatedType(ParseProtoOrDie<IrMatchFieldDefinition>(
+          R"pb(
+            match_field {
+              id: 1
+              name: "dragon_field"
+              match_type: EXACT
+              type_name { name: "dragon_type" }
+            }
+            format: STRING
+          )pb")));
+}
+
+TEST(HasP4RuntimeTranslatedType, P4RuntimeTranslatedTypeParameterReturnsTrue) {
+  EXPECT_FALSE(HasP4RuntimeTranslatedType(
+      ParseProtoOrDie<IrActionDefinition::IrActionParamDefinition>(
+          R"pb(
+            param { id: 1 name: "dragon_param" }
+            format: HEX_STRING
+          )pb")));
+
+  EXPECT_FALSE(HasP4RuntimeTranslatedType(
+      ParseProtoOrDie<IrActionDefinition::IrActionParamDefinition>(
+          R"pb(
+            param {
+              id: 1
+              name: "dragon_param"
+              type_name { name: "dragon_type" }
+            }
+            format: HEX_STRING
+          )pb")));
+
+  EXPECT_TRUE(HasP4RuntimeTranslatedType(
+      ParseProtoOrDie<IrActionDefinition::IrActionParamDefinition>(
+          R"pb(
+            param {
+              id: 1
+              name: "dragon_param"
+              type_name { name: "dragon_type" }
+            }
+            format: STRING
+          )pb")));
+}
+
+}  // namespace
+}  // namespace pdpi
diff --git a/p4_pdpi/p4_runtime_session.cc b/p4_pdpi/p4_runtime_session.cc
@@ -331,14 +331,10 @@ P4RuntimeSession::GetAllStreamMessagesFor(absl::Duration duration) {
 }
 
 absl::Status P4RuntimeSession::Finish() {
-  ASSIGN_OR_RETURN(std::vector<p4::v1::StreamMessageResponse> responses,
-                   ReadStreamChannelResponsesAndFinish());
-  for (auto& response : responses) {
-    LOG(WARNING) << "dropping unread message from switch on stream channel "
-                    "when trying to Finish P4RuntimeSession: "
-                 << response.DebugString();
-  }
-  return absl::OkStatus();
+  // Discarding unread messages without warning, since finishing with unread
+  // messages is the common case and thus a warning would produce a lot of log
+  // spam.
+  return ReadStreamChannelResponsesAndFinish().status();
 }
 
 absl::StatusOr<std::vector<p4::v1::StreamMessageResponse>>

diff --git a/p4_pdpi/p4_runtime_session_extras.cc b/p4_pdpi/p4_runtime_session_extras.cc
@@ -126,6 +126,24 @@ absl::Status InstallPiEntities(P4RuntimeSession& p4rt,
   return InstallPiEntities(p4rt, parsed_entities);
 }
 
+absl::StatusOr<std::vector<p4::v1::Entity>> ReadPiEntitiesSorted(
+    P4RuntimeSession& p4rt) {
+  ASSIGN_OR_RETURN(std::vector<p4::v1::Entity> entities, ReadPiEntities(&p4rt));
+  absl::Status status;
+  // We sort entities based on their key. Since they were read back from the
+  // switch, we know that no two entities can have the same key.
+  absl::c_sort(entities,
+               [&](const p4::v1::Entity& e1, const p4::v1::Entity& e2) {
+                 absl::StatusOr<EntityKey> k1 = EntityKey::MakeEntityKey(e1);
+                 absl::StatusOr<EntityKey> k2 = EntityKey::MakeEntityKey(e2);
+                 status.Update(k1.status());
+                 status.Update(k2.status());
+                 return status.ok() ? *k1 < *k2 : true;
+               });
+  RETURN_IF_ERROR(status);
+  return entities;
+}
+
 absl::StatusOr<std::vector<p4::v1::TableEntry>> ReadPiTableEntriesSorted(
     P4RuntimeSession& p4rt) {
   ASSIGN_OR_RETURN(std::vector<p4::v1::TableEntry> entries,
@@ -140,20 +158,39 @@ absl::StatusOr<std::vector<p4::v1::TableEntry>> ReadPiTableEntriesSorted(
   return entries;
 }
 
+absl::StatusOr<IrEntities> ReadIrEntities(P4RuntimeSession& p4rt) {
+  ASSIGN_OR_RETURN(IrP4Info info, GetIrP4Info(p4rt),
+                   _.SetPrepend() << "cannot read entities from switch: failed "
+                                     "to pull P4Info from switch: ");
+
+  ASSIGN_OR_RETURN(std::vector<p4::v1::Entity> entities, ReadPiEntities(&p4rt));
+  return PiEntitiesToIr(info, entities);
+}
+
 absl::StatusOr<IrTableEntries> ReadIrTableEntries(P4RuntimeSession& p4rt) {
   ASSIGN_OR_RETURN(IrP4Info info, GetIrP4Info(p4rt),
-                   _.SetPrepend() << "cannot install entries on switch: failed "
+                   _.SetPrepend() << "cannot read entries from switch: failed "
                                      "to pull P4Info from switch: ");
 
   ASSIGN_OR_RETURN(std::vector<p4::v1::TableEntry> entries,
                    ReadPiTableEntries(&p4rt));
   return PiTableEntriesToIr(info, entries);
 }
 
+absl::StatusOr<IrEntities> ReadIrEntitiesSorted(P4RuntimeSession& p4rt) {
+  ASSIGN_OR_RETURN(IrP4Info info, GetIrP4Info(p4rt),
+                   _.SetPrepend() << "cannot read entities from switch: failed "
+                                     "to pull P4Info from switch: ");
+
+  ASSIGN_OR_RETURN(std::vector<p4::v1::Entity> entities,
+                   ReadPiEntitiesSorted(p4rt));
+  return PiEntitiesToIr(info, entities);
+}
+
 absl::StatusOr<IrTableEntries> ReadIrTableEntriesSorted(
     P4RuntimeSession& p4rt) {
   ASSIGN_OR_RETURN(IrP4Info info, GetIrP4Info(p4rt),
-                   _.SetPrepend() << "cannot install entries on switch: failed "
+                   _.SetPrepend() << "cannot read entries from switch: failed "
                                      "to pull P4Info from switch: ");
 
   ASSIGN_OR_RETURN(std::vector<p4::v1::TableEntry> entries,

diff --git a/p4_pdpi/p4_runtime_session_extras.h b/p4_pdpi/p4_runtime_session_extras.h
@@ -96,19 +96,37 @@ absl::Status InstallPiEntities(P4RuntimeSession& p4rt,
 absl::Status InstallPiEntities(P4RuntimeSession& p4rt,
                                absl::string_view entities);
 
+// Reads control plane entities from the switch using `p4rt` and returns them in
+// PI representation in an order determined by `pdpi::EntityKey`.
+absl::StatusOr<std::vector<p4::v1::Entity>> ReadPiEntitiesSorted(
+    P4RuntimeSession& p4rt);
+
 // Reads table entries from the switch using `p4rt` and returns them in PI
-// representation in an order determined by pdpi::TableEntryKey.
+// representation in an order determined by `pdpi::TableEntryKey`.
+ABSL_DEPRECATED("Prefer ReadPiEntitiesSorted instead.")
 absl::StatusOr<std::vector<p4::v1::TableEntry>> ReadPiTableEntriesSorted(
     P4RuntimeSession& p4rt);
 
+// Reads control plane entities from the switch using `p4rt` and returns them in
+// IR representation. Reads the P4Info used in translation from the switch.
+absl::StatusOr<IrEntities> ReadIrEntities(P4RuntimeSession& p4rt);
+
 // Reads table entries from the switch using `p4rt` and returns them in IR
 // representation. Reads the P4Info used in translation from the switch.
+ABSL_DEPRECATED("Prefer ReadIrEntities instead.")
 absl::StatusOr<IrTableEntries> ReadIrTableEntries(P4RuntimeSession& p4rt);
 
+// Reads control plane entities from the switch using `p4rt` and returns them in
+// IR representation in an order determined `by pdpi::EntityKey` on the
+// corresponding PI representation. Reads the P4Info used in translation from
+// the switch.
+absl::StatusOr<IrEntities> ReadIrEntitiesSorted(P4RuntimeSession& p4rt);
+
 // Reads table entries from the switch using `p4rt` and returns them in IR
-// representation in an order determined by pdpi::TableEntryKey on the
+// representation in an order determined by `pdpi::TableEntryKey` on the
 // corresponding PI representation. Reads the P4Info used in translation from
 // the switch.
+ABSL_DEPRECATED("Prefer ReadIrEntitiesSorted instead.")
 absl::StatusOr<IrTableEntries> ReadIrTableEntriesSorted(P4RuntimeSession& p4rt);
 
 // Constructs a write request with metadata from `p4rt` and sends it to the

diff --git a/p4_pdpi/sequencing.cc b/p4_pdpi/sequencing.cc
@@ -271,11 +271,12 @@ SequencePiUpdatesIntoWriteRequests(const IrP4Info& info,
   return requests;
 }
 
-absl::StatusOr<std::vector<p4::v1::WriteRequest>> SplitUpWriteRequest(
-    const p4::v1::WriteRequest& write_request,
+absl::StatusOr<std::vector<p4::v1::WriteRequest>> ExtractWriteRequests(
+    p4::v1::WriteRequest&& write_request,
     std::optional<int> max_write_request_size) {
-  if (!max_write_request_size.has_value()) {
-    return std::vector<p4::v1::WriteRequest>{write_request};
+  if (!max_write_request_size.has_value() ||
+      write_request.updates_size() <= *max_write_request_size) {
+    return std::vector<p4::v1::WriteRequest>{std::move(write_request)};
   }
 
   if (*max_write_request_size < 1) {
@@ -291,7 +292,7 @@ absl::StatusOr<std::vector<p4::v1::WriteRequest>> SplitUpWriteRequest(
       requests.push_back(std::move(request));
     }
     p4::v1::WriteRequest& request = requests.back();
-    *request.add_updates() = write_request.updates(i);
+    *request.add_updates() = std::move(write_request.updates(i));
   }
   return requests;
 }

diff --git a/p4_pdpi/sequencing.h b/p4_pdpi/sequencing.h
@@ -15,7 +15,6 @@
 #ifndef PINS_P4_PDPI_SEQUENCING_H_
 #define PINS_P4_PDPI_SEQUENCING_H_
 
-#include <functional>
 #include <optional>
 #include <vector>
 
@@ -50,12 +49,12 @@ SequencePiUpdatesIntoWriteRequests(const IrP4Info& info,
                                    absl::Span<const p4::v1::Update> updates,
                                    std::optional<int> max_batch_size = 5000);
 
-// Splits up `write_request` into ceil(write_request.update_size() /
-// max_write_request_size) number of p4::WriteRequest(s). The order of updates
-// across the resulting WriteRequests remains the same as the order of updates
-// in `write_request`.
-absl::StatusOr<std::vector<p4::v1::WriteRequest>> SplitUpWriteRequest(
-    const p4::v1::WriteRequest& write_request,
+// Extracts updates in `write_request` and form
+// ceil(write_request.update_size() / max_write_request_size) number of
+// p4::WriteRequest(s). The order of updates across the resulting WriteRequests
+// remains the same as the order of updates in `write_request`.
+absl::StatusOr<std::vector<p4::v1::WriteRequest>> ExtractWriteRequests(
+    p4::v1::WriteRequest&& write_request,
     std::optional<int> max_write_request_size = 5000);
 
 // Returns a vector of batches, where each batch is given by a vector of indices