Ensure that bmv2 targets are notified in case of config swap

In some case, the do_swap() call in simple_switch never returned 0
(because the swap was performed independently of the target), which
meant that the with_queueing_metadata flag was not updated.

This created an issue in the following specific scenario:
 * simple_switch is started with a JSON config that does not include
   queueing metadata fields
 * a JSON config swap is performed (using the runtime_CLI or P4Runtime,
   in the case of simple_switch_grpc), and the new config includes all
   queueing metadata fields
 * queueing metadata fields still show as 0 for all subsequent traffic

To fix this, we get rid of the assumption that targets are in charge of
calling do_swap() themselves, which must data back to when Packet
instantiation was not guarded by a lock (thus requiring the
collaboration of targets to ensure no new Packets were created as the
swap if being performed).

From now targets are notified using the swap_notify_() virtual method,
which targets can override.

Fixes #905

include/bm/bm_sim/switch.h

@@ -37,8 +37,8 @@
 //!
 //! When subclassing on of these two classes, you need to remember to implement
 //! the two pure virtual functions:
-//! bm::SwitchWContexts::receive(int port_num, const char *buffer, int len) and
-//! bm::SwitchWContexts::start_and_return(). Your receive() implementation will
+//! bm::SwitchWContexts::receive_(int port_num, const char *buffer, int len) and
+//! bm::SwitchWContexts::start_and_return_(). Your receive() implementation will
 //! be called for you every time a new packet is received by the device. In your
 //! start_and_return() function, you are supposed to start the different
 //! processing threads of your target switch and return immediately. Note that
@@ -49,20 +49,14 @@
 //! Both switch classes support live swapping of P4-JSON configurations. To
 //! enable it you need to provide the correct flag to the constructor (see
 //! bm::SwitchWContexts::SwitchWContexts()). Swaps are ordered through the
-//! runtime interfaces. However, it is the target switch responsibility to
-//! decide when to "commit" the swap. This is because swapping configurations
-//! invalidate certain pointers that you may still be using. We plan on trying
-//! to make this more target-friendly in the future but it is not trivial. Here
-//! is an example of how the simple router target implements swapping:
-//! @code
-//! // swap is enabled, so update pointers if needed
-//! if (this->do_swap() == 0) {  // a swap took place
-//!   ingress_mau = this->get_pipeline("ingress");
-//!   egress_mau = this->get_pipeline("egress");
-//!   parser = this->get_parser("parser");
-//!   deparser = this->get_deparser("deparser");
-//! }
-//! @endcode
+//! runtime interfaces. We ensure that during the actual swap operation
+//! (bm::SwitchWContexts::do_swap() method), there is no Packet instance
+//! inflight, which we achieve using the process_packet_mutex mutex). The final
+//! step of the swap is to call bm::SwitchWContexts::swap_notify_(), which
+//! targets can override if they need to perform some operations as part of the
+//! swap. Targets are guaranteed that no Packet instances exist as that
+//! time. Note that swapping configurations may invalidate pointers that you are
+//! still using, and it is your responsibility to refresh them.
 
 #ifndef BM_BM_SIM_SWITCH_H_
 #define BM_BM_SIM_SWITCH_H_
@@ -880,6 +874,8 @@ class SwitchWContexts : public DevMgr, public RuntimeInterface {
 
   void reset_target_state();
 
+  void swap_notify();
+
   //! Override in your switch implementation; it will be called every time a
   //! packet is received.
   virtual int receive_(port_t port_num, const char *buffer, int len) = 0;
@@ -895,6 +891,13 @@ class SwitchWContexts : public DevMgr, public RuntimeInterface {
   //! simple_switch target uses this to reset PRE state.
   virtual void reset_target_state_() { }
 
+  //! You can override this method in your target. It will be called at the end
+  //! of a config swap operation. At that time, you will be guaranteed that no
+  //! Packet instances exist, as long as your target uses the correct methods to
+  //! instantiate these objects (bm::SwitchWContexts::new_packet_ptr() and
+  //! bm::SwitchWContexts::new_packet()).
+  virtual void swap_notify_() { }
+
  private:
   size_t nb_cxts{};
   // TODO(antonin)

src/bm_sim/switch.cpp

@@ -93,6 +93,11 @@ SwitchWContexts::reset_target_state() {
   reset_target_state_();
 }
 
+void
+SwitchWContexts::swap_notify() {
+  swap_notify_();
+}
+
 std::string
 SwitchWContexts::get_debugger_addr() const {
 #ifdef BM_DEBUG_ON
@@ -467,6 +472,8 @@ SwitchWContexts::do_swap() {
       phv_source->set_phv_factory(cxt_id, &cxt.get_phv_factory());
     rc &= swap_done;
   }
+  // at this stage, we have no more Packet instances in bmv2
+  swap_notify();
 #ifdef BM_DEBUG_ON
   Debugger::get()->config_change();
 #endif

targets/psa_switch/psa_switch.cpp

@@ -142,12 +142,6 @@ PsaSwitch::PsaSwitch(bool enable_swap)
 
 int
 PsaSwitch::receive_(port_t port_num, const char *buffer, int len) {
-
-  // for p4runtime program swap - antonin
-  // putting do_swap call here is ok because blocking this thread will not
-  // block processing of existing packet instances, which is a requirement
-  do_swap();
-
   // we limit the packet buffer to original size + 512 bytes, which means we
   // cannot add more than 512 bytes of header data to the packet, which should
   // be more than enough

targets/simple_router/simple_router.cpp

@@ -57,9 +57,6 @@ class SimpleSwitch : public Switch {
   int receive_(port_t port_num, const char *buffer, int len) override {
     static int pkt_id = 0;
 
-    if (this->do_swap() == 0)  // a swap took place
-      swap_happened = true;
-
     auto packet = new_packet_ptr(port_num, pkt_id++, len,
                                  bm::PacketBuffer(2048, buffer, len));
 
@@ -76,6 +73,10 @@ class SimpleSwitch : public Switch {
     t2.detach();
   }
 
+  void swap_notify_() override {
+    swap_happened = true;
+  }
+
  private:
   void pipeline_thread();
   void transmit_thread();

targets/simple_switch/simple_switch.cpp

@@ -236,12 +236,6 @@ SimpleSwitch::SimpleSwitch(bool enable_swap, port_t drop_port)
 
 int
 SimpleSwitch::receive_(port_t port_num, const char *buffer, int len) {
-  // this is a good place to call this, because blocking this thread will not
-  // block the processing of existing packet instances, which is a requirement
-  if (do_swap() == 0) {
-    check_queueing_metadata();
-  }
-
   // we limit the packet buffer to original size + 512 bytes, which means we
   // cannot add more than 512 bytes of header data to the packet, which should
   // be more than enough
@@ -287,6 +281,13 @@ SimpleSwitch::start_and_return_() {
   threads_.push_back(std::thread(&SimpleSwitch::transmit_thread, this));
 }
 
+void
+SimpleSwitch::swap_notify_() {
+  bm::Logger::get()->debug(
+      "simple_switch target has been notified of a config swap");
+  check_queueing_metadata();
+}
+
 SimpleSwitch::~SimpleSwitch() {
   input_buffer->push_front(
       InputBuffer::PacketType::SENTINEL, nullptr);
@@ -437,12 +438,15 @@ SimpleSwitch::check_queueing_metadata() {
   bool deq_timedelta_e = field_exists("queueing_metadata", "deq_timedelta");
   bool deq_qdepth_e = field_exists("queueing_metadata", "deq_qdepth");
   if (enq_timestamp_e || enq_qdepth_e || deq_timedelta_e || deq_qdepth_e) {
-    if (enq_timestamp_e && enq_qdepth_e && deq_timedelta_e && deq_qdepth_e)
+    if (enq_timestamp_e && enq_qdepth_e && deq_timedelta_e && deq_qdepth_e) {
       with_queueing_metadata = true;
-    else
+      return;
+    } else {
       bm::Logger::get()->warn(
           "Your JSON input defines some but not all queueing metadata fields");
+    }
   }
+  with_queueing_metadata = false;
 }
 
 void

targets/simple_switch/simple_switch.h

@@ -96,6 +96,8 @@ class SimpleSwitch : public Switch {
 
   void reset_target_state_() override;
 
+  void swap_notify_() override;
+
   bool mirroring_add_session(mirror_id_t mirror_id,
                              const MirroringSessionConfig &config);