On Wed, Dec 17, 2025 at 11:01 AM Eelco Chaudron <[email protected]> wrote: > > > > On 29 Nov 2025, at 0:08, [email protected] wrote: > > > From: Numan Siddique <[email protected]> > > > > netdev-offload-tc when offloading a flow to tc, uses the flow's > > recirc_id as flower chain id. recirc_id is of type 'uint32_t'. > > Kernel tc reserves the upper nibble (4 bits) of the tc flower's > > chain index for extended action opcode [1]. If a flow's recirc_id > > value is greater than 268,435,455 (0x0fffffff), kernel returns > > EINVAL when ovs-vswitchd tries to offload the flow. > > > > This commit fixes this offload issue by setting the maximum > > value of recirc id to 268,435,455 when hw-offload is enabled. > > > > Note: This commit renames a static function - mirror_destroy() > > to mirror_delete() in vswitchd/bridge.c since a public function > > with the same name exists in ofproto/ofproto-dpif-mirror.c. > > Hi Numan, > > Rather than adding all this infrastructure, should we maybe limit the > recirc_id to 2^28 - 1?
I don't have a strong opinion. Just that userspace datapaths will be limited as well. Based on the comments from Ilya and others, please let me know which way you prefer and I'll update the patch accordingly. > > Others WDYT? Anyhow, some comments below. > > //Eelco > > > > > [1] - > > https://elixir.bootlin.com/linux/v6.17.6/source/tools/include/uapi/linux/pkt_cls.h#L50-L64 > > > > Reported-at: > > https://mail.openvswitch.org/pipermail/ovs-dev/2025-November/427485.html > > Suggested-by: Ilya Maximets <[email protected]> > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > lib/netdev-offload-provider.h | 4 ++++ > > lib/netdev-offload-tc.c | 6 ++++++ > > lib/netdev-offload.c | 23 +++++++++++++++++++++++ > > lib/netdev-offload.h | 1 + > > ofproto/ofproto-dpif-rid.c | 18 ++++++++++++++++-- > > ofproto/ofproto-dpif-rid.h | 2 ++ > > vswitchd/bridge.c | 12 +++++++----- > > 7 files changed, 59 insertions(+), 7 deletions(-) > > > > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h > > index 9108856d18..9def5858c7 100644 > > --- a/lib/netdev-offload-provider.h > > +++ b/lib/netdev-offload-provider.h > > @@ -121,6 +121,10 @@ struct netdev_flow_api { > > int (*meter_del)(ofproto_meter_id meter_id, > > struct ofputil_meter_stats *stats); > > > > + /* Returns the maximum value of the flow recirc_id which the provider > > can > > + * support, if it uses the flow recirc_id to offload. */ > > + uint32_t (*get_max_recirc_id)(void); > > + > > /* Initializies the netdev flow api. > > * Return 0 if successful, otherwise returns a positive errno value. */ > > int (*init_flow_api)(struct netdev *); > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > > index d4b6e72f0a..ba157ce2c8 100644 > > --- a/lib/netdev-offload-tc.c > > +++ b/lib/netdev-offload-tc.c > > @@ -3430,6 +3430,11 @@ meter_tc_del_policer(ofproto_meter_id meter_id, > > return err; > > } > > > > +static uint32_t > > +netdev_tc_get_max_recird_id(void) { > > Guess you made a spelling error, should be netdev_tc_get_max_recirc_id()? Ack. > > > + return TC_ACT_EXT_VAL_MASK; > > We return this value independent of whether offload is enabled or not. Will > this not always limit the number of recirculation IDs? Right. I thought that tc flow api will be registered only if hw-offload is enabled. I'll address this in v2 if we decide to go this approach. > > > +} > > + > > const struct netdev_flow_api netdev_offload_tc = { > > .type = "linux_tc", > > .flow_flush = netdev_tc_flow_flush, > > @@ -3443,5 +3448,6 @@ const struct netdev_flow_api netdev_offload_tc = { > > .meter_set = meter_tc_set_policer, > > .meter_get = meter_tc_get_policer, > > .meter_del = meter_tc_del_policer, > > + .get_max_recirc_id = netdev_tc_get_max_recird_id, > > .init_flow_api = netdev_tc_init_flow_api, > > }; > > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > > index 8a9d365559..c6c5ae486d 100644 > > --- a/lib/netdev-offload.c > > +++ b/lib/netdev-offload.c > > @@ -908,3 +908,26 @@ netdev_set_flow_api_enabled(const struct smap > > *ovs_other_config) > > } > > } > > } > > + > > +/* Returns the minimum of the max recirc id supported by each registered > > flow, > > + * API, if it supports get_max_recirc_id(). > > + * Returns 0, if none of the registered flow API supports > > get_max_recirc_id(). > > + */ > > +uint32_t > > +netdev_get_max_recird_id(void) > > +{ > > + uint32_t max_recirc_id = 0; > > + > > + struct netdev_registered_flow_api *rfa; > > + > > + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > > + if (rfa->flow_api->get_max_recirc_id) { > > Maybe if (!rfa->flow_api->get_max_recirc_id) { > continue > } To save one indent level. Ack. > > > + uint32_t rfa_max_recirc_id = > > rfa->flow_api->get_max_recirc_id(); > > Needs a new line here. Ack. > > > + if (!max_recirc_id || rfa_max_recirc_id < max_recirc_id) { > > + max_recirc_id = rfa_max_recirc_id; > > + } > > + } > > + } > > + > > + return max_recirc_id; > > +} > > diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h > > index 7fc30a4243..f62a6f5d94 100644 > > --- a/lib/netdev-offload.h > > +++ b/lib/netdev-offload.h > > @@ -127,6 +127,7 @@ bool netdev_is_flow_api_enabled(void); > > void netdev_set_flow_api_enabled(const struct smap *ovs_other_config); > > bool netdev_is_offload_rebalance_policy_enabled(void); > > int netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows); > > +uint32_t netdev_get_max_recird_id(void); > > > > struct dpif_port; > > int netdev_ports_insert(struct netdev *, struct dpif_port *); > > diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c > > index f01468025c..f7bef09c3d 100644 > > --- a/ofproto/ofproto-dpif-rid.c > > +++ b/ofproto/ofproto-dpif-rid.c > > @@ -36,6 +36,8 @@ static struct ovs_list expired OVS_GUARDED_BY(mutex) > > > > static uint32_t next_id OVS_GUARDED_BY(mutex) = 1; /* Possible next free > > id. */ > > > > +static uint32_t max_recirc_id; > > + > > #define RECIRC_POOL_STATIC_IDS 1024 > > > > static void recirc_id_node_free(struct recirc_id_node *); > > @@ -83,6 +85,16 @@ recirc_run(void) > > ovs_mutex_unlock(&mutex); > > } > > > > +/* Sets the max recirc id. Caller cannnot reduce the max recirc id once > > set. > > Three nnn’s Accck :) > > > + */ > > +void > > +recirc_set_max_recirc_id(uint32_t max_recirc_id_) > > +{ > > + if (max_recirc_id_ > max_recirc_id) { > > + max_recirc_id = max_recirc_id_; > > + } > > +} > > + > > /* We use the id as the hash value, which works due to cmap internal > > rehashing. > > * We also only insert nodes with unique IDs, so all possible hash > > collisions > > * remain internal to the cmap. */ > > @@ -228,7 +240,8 @@ frozen_state_free(struct frozen_state *state) > > > > /* Allocate a unique recirculation id for the given set of flow metadata. > > * The ID space is 2^^32, so there should never be a situation in which all > > - * the IDs are used up. We loop until we find a free one. */ > > + * the IDs are used up. Although, the ID space could be limited to less > > + * than 2^^32, if max_recirc_id is set. We loop until we find a free one. > > */ > > static struct recirc_id_node * > > recirc_alloc_id__(const struct frozen_state *state, uint32_t hash) > > { > > @@ -247,7 +260,8 @@ recirc_alloc_id__(const struct frozen_state *state, > > uint32_t hash) > > RECIRC_POOL_STATIC_IDS IDs on the later rounds, though, as some > > of > > the initial allocations may be for long term uses (like bonds). > > */ > > node->id = next_id++; > > - if (OVS_UNLIKELY(!node->id)) { > > + if (OVS_UNLIKELY(!node->id || > > + (max_recirc_id && node->id >= max_recirc_id))) { > > next_id = RECIRC_POOL_STATIC_IDS + 1; > > node->id = next_id++; > > } > > diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h > > index 4df630c62b..712f5b81e8 100644 > > --- a/ofproto/ofproto-dpif-rid.h > > +++ b/ofproto/ofproto-dpif-rid.h > > @@ -212,6 +212,8 @@ static inline bool recirc_id_node_try_ref_rcu(const > > struct recirc_id_node *n_) > > > > void recirc_id_node_unref(const struct recirc_id_node *); > > > > +void recirc_set_max_recirc_id(uint32_t max_recirc_id_); > > + > > void recirc_run(void); > > > > /* Recirculation IDs on which references are held. */ > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > > index 475eefefaa..c00ee4b7c9 100644 > > --- a/vswitchd/bridge.c > > +++ b/vswitchd/bridge.c > > @@ -43,6 +43,7 @@ > > #include "odp-execute.h" > > #include "ofproto/bond.h" > > #include "ofproto/ofproto.h" > > +#include "ofproto/ofproto-dpif-rid.h" > > #include "openvswitch/dynamic-string.h" > > #include "openvswitch/list.h" > > #include "openvswitch/meta-flow.h" > > @@ -328,7 +329,7 @@ static void run_system_stats(void); > > static void bridge_configure_mirrors(struct bridge *); > > static struct mirror *mirror_create(struct bridge *, > > const struct ovsrec_mirror *); > > -static void mirror_destroy(struct mirror *); > > +static void mirror_delete(struct mirror *); > > static bool mirror_configure(struct mirror *); > > static void mirror_refresh_stats(struct mirror *); > > > > @@ -3398,6 +3399,7 @@ bridge_run(void) > > netdev_set_flow_api_enabled(&cfg->other_config); > > dpdk_init(&cfg->other_config); > > userspace_tso_init(&cfg->other_config); > > + recirc_set_max_recirc_id(netdev_get_max_recird_id()); > > } > > > > /* Initialize the ofproto library. This only needs to run once, but > > @@ -3708,7 +3710,7 @@ bridge_destroy(struct bridge *br, bool del) > > port_destroy(port); > > } > > HMAP_FOR_EACH_SAFE (mirror, hmap_node, &br->mirrors) { > > - mirror_destroy(mirror); > > + mirror_delete(mirror); > > } > > > > hmap_remove(&all_bridges, &br->node); > > @@ -5153,7 +5155,7 @@ bridge_configure_mirrors(struct bridge *br) > > > > atom.uuid = m->uuid; > > if (!ovsdb_datum_find_key(mc, &atom, OVSDB_TYPE_UUID, NULL)) { > > - mirror_destroy(m); > > + mirror_delete(m); > > } > > } > > > > @@ -5166,7 +5168,7 @@ bridge_configure_mirrors(struct bridge *br) > > } > > m->cfg = cfg; > > if (!mirror_configure(m)) { > > - mirror_destroy(m); > > + mirror_delete(m); > > } > > } > > > > @@ -5192,7 +5194,7 @@ mirror_create(struct bridge *br, const struct > > ovsrec_mirror *cfg) > > } > > > > static void > > -mirror_destroy(struct mirror *m) > > +mirror_delete(struct mirror *m) > > { > > if (m) { > > struct bridge *br = m->bridge; > > -- > > 2.52.0 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
