On Wed, May 27, 2026 at 7:06 PM Lorenzo Bianconi < [email protected]> wrote:
> > The packet buffering, which is part of the ARP processing, required > > a pinctrl lock. As a consequence we could block packet processing > > when the lock is taken by main thread and possibly drop some traffic > > that is heading towards pinctrl. Make sure the packet buffering is > > now handled without any lock, that is done by using cmap instead of > > hmap to store the objects and all concurrent members are accessed > > via atomics instead. This should increase the throughput of the > > packet-in processing for ARP handling. > > Hi Ales, > > I think the patch is fine. Moreover, reading the comments from Marc, I > consider > this one the first step toward reducing the contention in pinctrl_thread > codepath. > > Acked-by: Lorenzo Bianconi <[email protected]> > > Regards, > Lorenzo > > > > Reported-at: https://redhat.atlassian.net/browse/FDP-3301 > > Signed-off-by: Ales Musil <[email protected]> > > --- > > controller/mac-cache.c | 127 +++++++++++++++++++++++------------------ > > controller/mac-cache.h | 48 +++++++--------- > > controller/pinctrl.c | 81 +++++++++++--------------- > > 3 files changed, 125 insertions(+), 131 deletions(-) > > > > [...] > > > @@ -115,25 +115,25 @@ struct bp_packet_data { > > }; > > > > struct buffered_packets { > > - struct hmap_node hmap_node; > > + struct cmap_node cmap_node; > > > > - struct mac_binding_data mb_data; > > + struct mac_binding_data mb_data; /* Immutable after insert. */ > > nit: in order to make it really immutable we can do something like: > > const struct mac_binding_data * const mb_data; > That would require extra allocation because it would become a pointer instead of embedded struct. Which is probably not worth Also, making it const struct is a bit clunky during initialization. > > > > - /* Queue of packet_data associated with this struct. */ > > + /* Queue of packet_data associated with this struct. > > + * Handler thread only. */ > > struct vector queue; > > nit: can we make vector (and the fields below) really accessible just by > pinctrl > thread? > I was thinking about that, but it didn't seem worth splitting into two structs just because of this. > > > > - /* Timestamp in ms when the buffered packet should expire. */ > > + /* Timestamp in ms when the buffered packet should expire. > > + * Handler thread only. */ > > long long int expire_at_ms; > > > > - /* Timestamp in ms when the buffered packet should do full SB > lookup.*/ > > - long long int lookup_at_ms; > > -}; > > + /* Resolved MAC address packed as uint64. 0 means unresolved. > > + * Written by main thread, read by handler thread. */ > > + atomic_uint64_t resolved_mac; > > > > -struct buffered_packets_ctx { > > - /* Map of all buffered packets waiting for the MAC address. */ > > - struct hmap buffered_packets; > > - /* List of packet data that are ready to be sent. */ > > - struct vector ready_packets_data; > > + /* Timestamp in ms when the buffered packet should do full SB > lookup. > > + * Main thread only. */ > > + long long int lookup_at_ms; > > }; > > > > /* Thresholds. */ > > @@ -211,27 +211,23 @@ void fdb_stats_run(struct vector *stats_vec, > uint64_t *req_delay, void *data); > > void bp_packet_data_destroy(struct bp_packet_data *pd); > > > > struct buffered_packets * > > -buffered_packets_add(struct buffered_packets_ctx *ctx, > > +buffered_packets_add(struct cmap *bp_map, > > struct mac_binding_data mb_data); > > > > void buffered_packets_packet_data_enqueue(struct buffered_packets *bp, > > const struct > ofputil_packet_in *pin, > > const struct ofpbuf > *continuation); > > > > -void buffered_packets_ctx_run(struct buffered_packets_ctx *ctx, > > - const struct hmap *recent_mbs, > > - struct ovsdb_idl_index *sbrec_pb_by_key, > > - struct ovsdb_idl_index *sbrec_dp_by_key, > > - struct ovsdb_idl_index *sbrec_pb_by_name, > > - struct ovsdb_idl_index > *sbrec_mb_by_lport_ip); > > - > > -void buffered_packets_ctx_init(struct buffered_packets_ctx *ctx); > > - > > -void buffered_packets_ctx_destroy(struct buffered_packets_ctx *ctx); > > +bool buffered_packets_lookup_run(struct cmap *bp_map, > > + const struct hmap *recent_mbs, > > + struct ovsdb_idl_index > *sbrec_pb_by_key, > > + struct ovsdb_idl_index > *sbrec_dp_by_key, > > + struct ovsdb_idl_index > *sbrec_pb_by_name, > > + struct ovsdb_idl_index > *sbrec_mb_by_lport_ip); > > > > -bool buffered_packets_ctx_is_ready_to_send(struct buffered_packets_ctx > *ctx); > > +void buffered_packets_run(struct cmap *bp_map, struct vector *rpd); > > > > -bool buffered_packets_ctx_has_packets(struct buffered_packets_ctx *ctx); > > +void buffered_packets_map_destroy(struct cmap *bp_map); > > > > void mac_binding_probe_stats_process_flow_stats( > > struct vector *stats_vec, > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 4473ec668..bb8d20e7f 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -185,16 +185,15 @@ struct pinctrl { > > static struct pinctrl pinctrl; > > > > static bool pinctrl_is_sb_commited(int64_t commit_cfg, int64_t cur_cfg); > > -static void init_buffered_packets_ctx(void); > > -static void destroy_buffered_packets_ctx(void); > > +static void init_buffered_packets_map(void); > > +static void destroy_buffered_packets_map(void); > > static void > > run_buffered_binding(const struct sbrec_mac_binding_table > *mac_binding_table, > > const struct hmap *local_datapaths, > > struct ovsdb_idl_index *sbrec_port_binding_by_key, > > struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > - struct ovsdb_idl_index > *sbrec_mac_binding_by_lport_ip) > > - OVS_REQUIRES(pinctrl_mutex); > > + struct ovsdb_idl_index > *sbrec_mac_binding_by_lport_ip); > > > > static void pinctrl_handle_put_mac_binding(const struct flow *md, > > const struct flow *headers, > > @@ -209,8 +208,7 @@ static void run_put_mac_bindings( > > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip) > > OVS_REQUIRES(pinctrl_mutex); > > static void wait_put_mac_bindings(void); > > -static void send_mac_binding_buffered_pkts(struct rconn *swconn) > > - OVS_REQUIRES(pinctrl_mutex); > > +static void send_mac_binding_buffered_pkts(struct rconn *swconn); > > > > static void pinctrl_activation_strategy_handler(const struct match *md); > > > > @@ -562,7 +560,7 @@ pinctrl_init(void) > > init_send_arps_nds(); > > init_ipv6_ras(); > > init_ipv6_prefixd(); > > - init_buffered_packets_ctx(); > > + init_buffered_packets_map(); > > init_activated_ports(); > > init_event_table(); > > ip_mcast_snoop_init(); > > @@ -1577,18 +1575,18 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > } > > } > > > > -static struct buffered_packets_ctx buffered_packets_ctx; > > +static struct cmap buffered_packets_map; > > > > static void > > -init_buffered_packets_ctx(void) > > +init_buffered_packets_map(void) > > { > > - buffered_packets_ctx_init(&buffered_packets_ctx); > > + cmap_init(&buffered_packets_map); > > } > > > > static void > > -destroy_buffered_packets_ctx(void) > > +destroy_buffered_packets_map(void) > > { > > - buffered_packets_ctx_destroy(&buffered_packets_ctx); > > + buffered_packets_map_destroy(&buffered_packets_map); > > } > > > > /* Called with in the pinctrl_handler thread context. */ > > @@ -1596,7 +1594,6 @@ static void > > pinctrl_handle_buffered_packets(const struct ofputil_packet_in *pin, > > const struct ofpbuf *continuation, > > bool is_arp) > > -OVS_REQUIRES(pinctrl_mutex) > > { > > const struct match *md = &pin->flow_metadata; > > struct mac_binding_data mb_data; > > @@ -1613,7 +1610,7 @@ OVS_REQUIRES(pinctrl_mutex) > > md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0], > > ip, eth_addr_zero); > > > > - struct buffered_packets *bp = > buffered_packets_add(&buffered_packets_ctx, > > + struct buffered_packets *bp = > buffered_packets_add(&buffered_packets_map, > > mb_data); > > if (!bp) { > > COVERAGE_INC(pinctrl_drop_buffered_packets_map); > > @@ -1643,9 +1640,7 @@ pinctrl_handle_arp(struct rconn *swconn, const > struct flow *ip_flow, > > return; > > } > > > > - ovs_mutex_lock(&pinctrl_mutex); > > pinctrl_handle_buffered_packets(pin, continuation, true); > > - ovs_mutex_unlock(&pinctrl_mutex); > > > > /* Compose an ARP packet. */ > > uint64_t packet_stub[128 / 8]; > > @@ -4081,12 +4076,12 @@ pinctrl_handler(void *arg_) > > send_arp_nd_run(swconn, &send_arp_nd_time); > > send_ipv6_ras(swconn, &send_ipv6_ra_time); > > send_ipv6_prefixd(swconn, &send_prefixd_time); > > - send_mac_binding_buffered_pkts(swconn); > > bfd_monitor_send_msg(swconn, &bfd_time); > > ovs_mutex_unlock(&pinctrl_mutex); > > } else { > > lock_failed = true; > > } > > + send_mac_binding_buffered_pkts(swconn); > > send_garp_rarp_run(swconn, &send_garp_rarp_time); > > ip_mcast_querier_run(swconn, &send_mcast_query_time); > > } > > @@ -4226,11 +4221,6 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > sbrec_port_binding_by_key, > > sbrec_igmp_groups, > > sbrec_ip_multicast_opts); > > - run_buffered_binding(mac_binding_table, local_datapaths, > > - sbrec_port_binding_by_key, > > - sbrec_datapath_binding_by_key, > > - sbrec_port_binding_by_name, > > - sbrec_mac_binding_by_lport_ip); > > sync_svc_monitors(ovnsb_idl_txn, svc_mon_table, > sbrec_port_binding_by_name, > > chassis); > > bfd_monitor_run(ovnsb_idl_txn, bfd_table, > sbrec_port_binding_by_name, > > @@ -4241,6 +4231,12 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > run_activated_ports(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > > sbrec_port_binding_by_key, chassis); > > ovs_mutex_unlock(&pinctrl_mutex); > > + > > + run_buffered_binding(mac_binding_table, local_datapaths, > > + sbrec_port_binding_by_key, > > + sbrec_datapath_binding_by_key, > > + sbrec_port_binding_by_name, > > + sbrec_mac_binding_by_lport_ip); > > } > > > > /* Table of ipv6_ra_state structures, keyed on logical port name. > > @@ -4758,7 +4754,7 @@ pinctrl_destroy(void) > > destroy_send_arps_nds(); > > destroy_ipv6_ras(); > > destroy_ipv6_prefixd(); > > - destroy_buffered_packets_ctx(); > > + destroy_buffered_packets_map(); > > destroy_activated_ports(); > > event_table_destroy(); > > destroy_put_mac_bindings(); > > @@ -4843,30 +4839,24 @@ pinctrl_handle_put_mac_binding(const struct flow > *md, > > notify_pinctrl_main(); > > } > > > > -#define READY_PACKETS_VEC_CAPACITY_THRESHOLD 1024 > > - > > /* Called with in the pinctrl_handler thread context. */ > > static void > > send_mac_binding_buffered_pkts(struct rconn *swconn) > > - OVS_REQUIRES(pinctrl_mutex) > > { > > enum ofp_version version = rconn_get_version(swconn); > > enum ofputil_protocol proto = > ofputil_protocol_from_ofp_version(version); > > - struct vector *rpd = &buffered_packets_ctx.ready_packets_data; > > + struct vector rpd = VECTOR_EMPTY_INITIALIZER(struct bp_packet_data); > > + > > + buffered_packets_run(&buffered_packets_map, &rpd); > > > > struct bp_packet_data *pd; > > - VECTOR_FOR_EACH_PTR (rpd, pd) { > > + VECTOR_FOR_EACH_PTR (&rpd, pd) { > > queue_msg(swconn, ofputil_encode_resume(&pd->pin, > pd->continuation, > > proto)); > > bp_packet_data_destroy(pd); > > } > > > > - vector_clear(rpd); > > - if (vector_capacity(rpd) >= READY_PACKETS_VEC_CAPACITY_THRESHOLD) { > > - VLOG_DBG("The ready_packets_data vector capacity (%"PRIuSIZE") " > > - "is over threshold.", vector_capacity(rpd)); > > - vector_shrink_to_fit(rpd); > > - } > > + vector_destroy(&rpd); > > } > > > > static void > > @@ -4935,9 +4925,8 @@ run_buffered_binding(const struct > sbrec_mac_binding_table *mac_binding_table, > > struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > struct ovsdb_idl_index > *sbrec_mac_binding_by_lport_ip) > > - OVS_REQUIRES(pinctrl_mutex) > > { > > - if (!buffered_packets_ctx_has_packets(&buffered_packets_ctx)) { > > + if (cmap_is_empty(&buffered_packets_map)) { > > return; > > } > > > > @@ -4983,18 +4972,16 @@ run_buffered_binding(const struct > sbrec_mac_binding_table *mac_binding_table, > > mac_binding_add(&recent_mbs, mb_data, smb, 0); > > } > > > > - buffered_packets_ctx_run(&buffered_packets_ctx, &recent_mbs, > > - sbrec_port_binding_by_key, > > - sbrec_datapath_binding_by_key, > > - sbrec_port_binding_by_name, > > - sbrec_mac_binding_by_lport_ip); > > + if (buffered_packets_lookup_run(&buffered_packets_map, &recent_mbs, > > + sbrec_port_binding_by_key, > > + sbrec_datapath_binding_by_key, > > + sbrec_port_binding_by_name, > > + sbrec_mac_binding_by_lport_ip)) { > > + notify_pinctrl_handler(); > > + } > > > > mac_bindings_clear(&recent_mbs); > > hmap_destroy(&recent_mbs); > > - > > - if (buffered_packets_ctx_is_ready_to_send(&buffered_packets_ctx)) { > > - notify_pinctrl_handler(); > > - } > > } > > > > static void > > @@ -6438,7 +6425,7 @@ may_inject_pkts(void) > > !cmap_is_empty(&garp_rarp_get_data()->data) || > > ipv6_prefixd_should_inject() || > > !ovs_list_is_empty(&mcast_query_list) || > > - > buffered_packets_ctx_is_ready_to_send(&buffered_packets_ctx) || > > + !cmap_is_empty(&buffered_packets_map) || > > bfd_monitor_should_inject()); > > } > > > > @@ -6528,9 +6515,7 @@ pinctrl_handle_nd_ns(struct rconn *swconn, const > struct flow *ip_flow, > > return; > > } > > > > - ovs_mutex_lock(&pinctrl_mutex); > > pinctrl_handle_buffered_packets(pin, continuation, false); > > - ovs_mutex_unlock(&pinctrl_mutex); > > > > uint64_t packet_stub[128 / 8]; > > struct dp_packet packet; > > -- > > 2.54.0 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > Thank you Mark and Lorenzo, applied to main. I was also considering backporting this to 26.03, are there any concerns or objections? Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
