Fix GroupSelectionIface implementation used by PI

PI uses a custom GroupSelectionIface implementation to support watch
ports for P4Runtime action profile group members.

Before this fix, membership information in the GroupSelectionIface
implementation wasn't cleaned up properly when a group was deleted,
which means that when a new group was created with the same id (but
different membership), table lookups would yield invalid members and
bmv2 would assert.

The fix consists in making the appropriate method calls to remove each
member from the group when the group is being deleted.

Fixes #893

PI/src/group_selection.cpp

@@ -57,6 +57,8 @@ GroupSelection::remove_member_from_group(grp_hdl_t grp, mbr_hdl_t mbr) {
   std::lock_guard<std::mutex> lock(mutex);
   auto &group = groups[grp];
   group.remove_member(mbr);
+  // Remove group from map when it is empty to avoid using up memory.
+  if (group.size() == 0) groups.erase(grp);
 }
 
 mbr_hdl_t
@@ -104,4 +106,9 @@ GroupSelection::GroupInfo::get_from_hash(hash_t h) const {
   return activated_members.get_nth(index);
 }
 
+size_t
+GroupSelection::GroupInfo::size() const {
+  return members.size();
+}
+
 }  // namespace pibmv2

PI/src/group_selection.h

@@ -56,6 +56,7 @@ class GroupSelection : public bm::ActionProfile::GroupSelectionIface {
     void add_member(mbr_hdl_t mbr);
     void remove_member(mbr_hdl_t mbr);
     mbr_hdl_t get_from_hash(hash_t h) const;
+    size_t size() const;
 
    private:
     bm::RandAccessUIntSet activated_members{};

src/bm_sim/action_profile.cpp

@@ -339,9 +339,12 @@ ActionProfile::delete_group(grp_hdl_t grp) {
       // the ref count for the members. Note that we do not allow deletion of a
       // member which is in a group
       GroupInfo &group_info = grp_mgr.at(grp);
-      for (auto mbr : group_info)
+      for (auto mbr : group_info) {
         index_ref_count.decrease(IndirectIndex::make_mbr_index(mbr));
 
+        grp_selector->remove_member_from_group(grp, mbr);
+      }
+
       int error = grp_handles.release_handle(grp);
       _BM_UNUSED(error);
       assert(!error);

targets/simple_switch_grpc/tests/test_action_profile.cpp

@@ -28,6 +28,7 @@
 #include <chrono>
 #include <string>
 #include <thread>
+#include <vector>
 
 #include "base_test.h"
 #include "utils.h"
@@ -64,31 +65,7 @@ class SimpleSwitchGrpcTest_ActionProfile : public SimpleSwitchGrpcBaseTest {
     update_json(act_prof_json);
     table_id = get_table_id(p4info, "IndirectWS");
     action_id = get_action_id(p4info, "send");
-    {  // oneshot programming
-      p4v1::WriteRequest req;
-      auto *update = req.add_updates();
-      update->set_type(p4v1::Update::INSERT);
-      auto *entry = update->mutable_entity()->mutable_table_entry();
-      entry->set_table_id(table_id);
-      auto *mf = entry->add_match();
-      mf->set_field_id(get_mf_id(p4info, "IndirectWS", "h.hdr.in_"));
-      mf->mutable_exact()->set_value("\xab");
-      auto *oneshot =
-          entry->mutable_action()->mutable_action_profile_action_set();
-      for (auto port : {port_1, port_2}) {
-        auto *action_entry = oneshot->add_action_profile_actions();
-        auto *action = action_entry->mutable_action();
-        action->set_action_id(action_id);
-        auto *param = action->add_params();
-        param->set_param_id(get_param_id(p4info, "send", "eg_port"));
-        param->set_value(port_to_bytes(port));
-        action_entry->set_weight(1);
-        action_entry->set_watch(port);
-      }
-      ClientContext context;
-      p4v1::WriteResponse rep;
-      EXPECT_TRUE(Write(&context, req, &rep).ok());
-    }
+    act_prof_id = get_act_prof_id(p4info, "ActProfWS");
   }
 
   grpc::Status send_and_receive(const std::string &entropy, int *eg_port) {
@@ -119,15 +96,83 @@ class SimpleSwitchGrpcTest_ActionProfile : public SimpleSwitchGrpcBaseTest {
     return dataplane_stub->SetPortOperStatus(&context, req, &rep);
   }
 
+  Status add_member(int member_id, int port) {
+    p4v1::Entity entity;
+    auto *member = entity.mutable_action_profile_member();
+    member->set_action_profile_id(act_prof_id);
+    member->set_member_id(member_id);
+    auto *action = member->mutable_action();
+    action->set_action_id(action_id);
+    auto *param = action->add_params();
+    param->set_param_id(get_param_id(p4info, "send", "eg_port"));
+    param->set_value(port_to_bytes(port));
+    return write(entity, p4v1::Update::INSERT);
+  }
+
+  Status delete_member(int member_id) {
+    p4v1::Entity entity;
+    auto *member = entity.mutable_action_profile_member();
+    member->set_action_profile_id(act_prof_id);
+    member->set_member_id(member_id);
+    return write(entity, p4v1::Update::DELETE);
+  }
+
+  template <typename It>
+  Status add_group(int group_id, It members_begin, It members_end) {
+    p4v1::Entity entity;
+    auto *group = entity.mutable_action_profile_group();
+    group->set_action_profile_id(act_prof_id);
+    group->set_group_id(group_id);
+    for (auto member_it = members_begin;
+         member_it != members_end;
+         ++member_it) {
+      auto *member = group->add_members();
+      member->set_member_id(*member_it);
+      member->set_weight(1);
+    }
+    return write(entity, p4v1::Update::INSERT);
+  }
+
+  Status delete_group(int group_id) {
+    p4v1::Entity entity;
+    auto *group = entity.mutable_action_profile_group();
+    group->set_action_profile_id(act_prof_id);
+    group->set_group_id(group_id);
+    return write(entity, p4v1::Update::DELETE);
+  }
+
   std::shared_ptr<grpc::Channel> dataplane_channel{nullptr};
   std::unique_ptr<p4::bm::DataplaneInterface::Stub> dataplane_stub{nullptr};
+  int act_prof_id;
   int table_id;
   int action_id;
   int port_1{1}, port_2{2};
   int ig_port{99};
 };
 
 TEST_F(SimpleSwitchGrpcTest_ActionProfile, WatchPort) {
+  {  // oneshot programming
+    p4v1::Entity entity;
+    auto *entry = entity.mutable_table_entry();
+    entry->set_table_id(table_id);
+    auto *mf = entry->add_match();
+    mf->set_field_id(get_mf_id(p4info, "IndirectWS", "h.hdr.in_"));
+    mf->mutable_exact()->set_value("\xab");
+    auto *oneshot =
+        entry->mutable_action()->mutable_action_profile_action_set();
+    for (auto port : {port_1, port_2}) {
+      auto *action_entry = oneshot->add_action_profile_actions();
+      auto *action = action_entry->mutable_action();
+      action->set_action_id(action_id);
+      auto *param = action->add_params();
+      param->set_param_id(get_param_id(p4info, "send", "eg_port"));
+      param->set_value(port_to_bytes(port));
+      action_entry->set_weight(1);
+      action_entry->set_watch(port);
+    }
+    EXPECT_TRUE(write(entity, p4v1::Update::INSERT).ok());
+  }
+
   std::string entropy("\x01\x02\x03\x04");
   int eg_port_0, eg_port;
   EXPECT_TRUE(send_and_receive(entropy, &eg_port_0).ok());
@@ -145,6 +190,45 @@ TEST_F(SimpleSwitchGrpcTest_ActionProfile, WatchPort) {
   EXPECT_EQ(eg_port, eg_port_0);
 }
 
+// See https://github.com/p4lang/behavioral-model/issues/893
+// This test ensures that when a group is deleted, state is cleaned-up properly
+// in the GroupSelectionIface implementation used by the PI bmv2
+// implementation. Before the above issue was fixed, membership information
+// wasn't cleaned up, which means that when a new group was created with the
+// same id (but different membership), table lookups would yield invalid
+// members.
+TEST_F(SimpleSwitchGrpcTest_ActionProfile, DeleteGroupWithMembers) {
+  int group_id(1);
+  std::vector<int> members{1, 2};
+  EXPECT_TRUE(add_member(members.at(0), port_1).ok());
+  EXPECT_TRUE(add_member(members.at(1), port_2).ok());
+  EXPECT_TRUE(add_group(group_id, members.begin(), members.end()).ok());
+  EXPECT_TRUE(delete_group(group_id).ok());
+  EXPECT_TRUE(delete_member(members.at(0)).ok());
+  EXPECT_TRUE(delete_member(members.at(1)).ok());
+
+  EXPECT_TRUE(add_member(members.at(0), port_1).ok());
+  EXPECT_TRUE(add_group(group_id, members.begin(), members.begin() + 1).ok());
+
+  {
+    p4v1::Entity entity;
+    auto *entry = entity.mutable_table_entry();
+    entry->set_table_id(table_id);
+    auto *mf = entry->add_match();
+    mf->set_field_id(get_mf_id(p4info, "IndirectWS", "h.hdr.in_"));
+    mf->mutable_exact()->set_value("\xab");
+    entry->mutable_action()->set_action_profile_group_id(group_id);
+    EXPECT_TRUE(write(entity, p4v1::Update::INSERT).ok());
+  }
+
+  int eg_port;
+  for (int i = 0; i < 32; i++) {
+    std::string entropy("\x01\x02\x03");
+    entropy.append(1, static_cast<char>(i));
+    EXPECT_TRUE(send_and_receive(entropy, &eg_port).ok());
+  }
+}
+
 }  // namespace
 
 }  // namespace testing

targets/simple_switch_grpc/tests/test_ternary.cpp

@@ -65,7 +65,7 @@ class SimpleSwitchGrpcTest_Ternary : public SimpleSwitchGrpcBaseTest {
   }
 
   p4v1::Entity make_entry(const std::string &v, const std::string &mask,
-                        int32_t priority, int a_id) const {
+                          int32_t priority, int a_id) const {
     p4v1::Entity entity;
     auto table_entry = entity.mutable_table_entry();
     table_entry->set_table_id(t_id);