From 498202d98d83a6c580aa86a3497b9b496bcd43b2 Mon Sep 17 00:00:00 2001 From: Antonin Bas Date: Fri, 5 Jun 2020 09:41:20 -0700 Subject: [PATCH] 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 | 35 ++++++++++++++----------- src/bm_sim/switch.cpp | 7 +++++ targets/psa_switch/psa_switch.cpp | 6 ----- targets/simple_router/simple_router.cpp | 7 ++--- targets/simple_switch/simple_switch.cpp | 20 ++++++++------ targets/simple_switch/simple_switch.h | 2 ++ 6 files changed, 44 insertions(+), 33 deletions(-) diff --git a/include/bm/bm_sim/switch.h b/include/bm/bm_sim/switch.h index 99207e7fe..0a17d742e 100644 --- a/include/bm/bm_sim/switch.h +++ b/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) diff --git a/src/bm_sim/switch.cpp b/src/bm_sim/switch.cpp index 68b8e84a8..f252ba26a 100644 --- a/src/bm_sim/switch.cpp +++ b/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 diff --git a/targets/psa_switch/psa_switch.cpp b/targets/psa_switch/psa_switch.cpp index e2007a22f..a15a2aea2 100644 --- a/targets/psa_switch/psa_switch.cpp +++ b/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 diff --git a/targets/simple_router/simple_router.cpp b/targets/simple_router/simple_router.cpp index ef68b78e4..929a8d42d 100644 --- a/targets/simple_router/simple_router.cpp +++ b/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(); diff --git a/targets/simple_switch/simple_switch.cpp b/targets/simple_switch/simple_switch.cpp index 469933d01..3fc21a2ba 100644 --- a/targets/simple_switch/simple_switch.cpp +++ b/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 diff --git a/targets/simple_switch/simple_switch.h b/targets/simple_switch/simple_switch.h index 3daa080d1..7ab36a4da 100644 --- a/targets/simple_switch/simple_switch.h +++ b/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);