On Mon, Jul 6, 2026 at 11:45 AM Eelco Chaudron <[email protected]> wrote: > > When tc_netdev_flow_put() rejects a flow because the upstream > recirc chain is not yet registered (used_chains count is zero), > the flow is permanently stranded in the kernel. Return EAGAIN > instead of EOPNOTSUPP so the revalidator retries the offload > once a sibling flow registers the chain. > > A new bool offload_deferred on struct dpif_op and struct udpif_key > propagates the rejection to the revalidator, which issues a retry > on the next cycle. Retries are bounded to > OFFLOAD_DEFERRED_RETRY_MSEC (2000ms) from the first failed attempt. > > Fixes: 273a4fce951a ("netdev-offload-tc: Only install recirc flows if the > parent is present.") > Reported-by: Numan Siddique <[email protected]> > Signed-off-by: Eelco Chaudron <[email protected]>
Thanks Eelco for the patch. I totally missed this one. FYI we already using the fix provided by you here - https://mail.openvswitch.org/pipermail/ovs-dev/2026-June/433035.html in our prod which is an older version of the submitted patch. It would be great if this patch can be reviewed. I'll take a look, test again and provide my Tested-by tag soon. Can you please add the below tag in the commit message ? Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2026-June/432774.html Thanks for the fix. Numan > --- > lib/dpif-offload-tc-netdev.c | 27 +++++++------ > lib/dpif-offload-tc.c | 10 ++++- > lib/dpif.c | 7 ++++ > lib/dpif.h | 3 ++ > ofproto/ofproto-dpif-upcall.c | 27 +++++++++++++ > tests/system-offloads-traffic.at | 68 ++++++++++++++++++++++++++++++++ > 6 files changed, 129 insertions(+), 13 deletions(-) > > diff --git a/lib/dpif-offload-tc-netdev.c b/lib/dpif-offload-tc-netdev.c > index a2f4ab8b5..3a521aee8 100644 > --- a/lib/dpif-offload-tc-netdev.c > +++ b/lib/dpif-offload-tc-netdev.c > @@ -2355,17 +2355,6 @@ tc_netdev_flow_put(struct dpif *dpif, struct netdev > *netdev, > chain = key->recirc_id; > mask->recirc_id = 0; > > - if (chain) { > - /* If we match on a recirculation ID, we must ensure the previous > - * flow is also in the TC datapath; otherwise, the entry is useless, > - * as the related packets will be handled by upcalls. */ > - if (!ccmap_find(&used_chains, chain)) { > - VLOG_DBG_RL(&rl, "match for chain %u failed due to non-existing " > - "goto chain action", chain); > - return EOPNOTSUPP; > - } > - } > - > if (flow_tnl_dst_is_set(&key->tunnel) || > flow_tnl_src_is_set(&key->tunnel)) { > VLOG_DBG_RL(&rl, > @@ -2687,6 +2676,22 @@ tc_netdev_flow_put(struct dpif *dpif, struct netdev > *netdev, > return EOPNOTSUPP; > } > > + if (chain && !ccmap_find(&used_chains, chain)) { > + /* If we match on a recirculation ID, we must ensure the previous > + * flow is also in the TC datapath; otherwise, the entry is useless, > + * as the related packets will be handled by upcalls. > + * > + * Return EAGAIN rather than EOPNOTSUPP: the chain may not be > + * registered yet because the upstream flow install is still in > + * progress. Unlike EOPNOTSUPP, EAGAIN tells the revalidator to > + * retry the TC offload on a subsequent cycle. The flow is still > + * installed in the kernel datapath (dp:ovs) in either case. */ > + VLOG_DBG_RL(&rl, "match for chain %u failed due to non-existing " > + "goto chain action", > + chain); > + return EAGAIN; > + } > + > memset(&adjust_stats, 0, sizeof adjust_stats); > if (get_ufid_tc_mapping(ufid, &id) == 0) { > VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", > diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c > index 3b84d9f20..cb996fa4a 100644 > --- a/lib/dpif-offload-tc.c > +++ b/lib/dpif-offload-tc.c > @@ -579,7 +579,8 @@ tc_parse_flow_put(struct tc_offload *offload_tc, struct > dpif *dpif, > } > netdev_set_hw_info(oor_netdev, HW_INFO_TYPE_OOR, true); > } > - level = (err == ENOSPC || err == EOPNOTSUPP) ? VLL_DBG : VLL_ERR; > + level = (err == ENOSPC || err == EOPNOTSUPP > + || err == EAGAIN) ? VLL_DBG : VLL_ERR; > VLOG_RL(&rl, level, "failed to offload flow: %s: %s", > ovs_strerror(err), > (oor_netdev ? netdev_get_name(oor_netdev) : > @@ -705,7 +706,12 @@ tc_operate(struct dpif *dpif, const struct dpif_offload > *offload_, > break; > } /* End of 'switch (op->type)'. */ > > - if (error != EOPNOTSUPP && error != ENOENT) { > + if (error == EAGAIN) { > + /* The recirc ID matched by this flow is not yet registered in > + * TC (no upstream flow has a goto action for this chain). > + * Defer the offload so the revalidator retries next cycle. */ > + op->offload_deferred = true; > + } else if (error != EOPNOTSUPP && error != ENOENT) { > /* If the operation is unsupported or the entry was not found, > * we are skipping this flow operation. Otherwise, it was > * processed and we should report the result. */ > diff --git a/lib/dpif.c b/lib/dpif.c > index 1afc7c662..0b93b0fae 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -1354,10 +1354,17 @@ dpif_operate(struct dpif *dpif, struct dpif_op **ops, > size_t n_ops, > for (i = 0; i < n_ops; i++) { > struct dpif_op *op = ops[i]; > op->error = EINVAL; > + op->offload_deferred = false; > } > return; > } > > + /* Initialize offload_deferred for all ops; the offload provider sets > + * it to true if the flow cannot be offloaded yet. */ > + for (size_t i = 0; i < n_ops; i++) { > + ops[i]->offload_deferred = false; > + } > + > while (n_ops > 0) { > size_t chunk; > > diff --git a/lib/dpif.h b/lib/dpif.h > index 3e6a34a25..79621887c 100644 > --- a/lib/dpif.h > +++ b/lib/dpif.h > @@ -782,6 +782,9 @@ int dpif_execute(struct dpif *, struct dpif_execute *); > struct dpif_op { > enum dpif_op_type type; > int error; > + bool offload_deferred; /* True if the offload provider requires a > + * prerequisite flow to be programmed first. > + * The revalidator will retry on the next cycle. > */ > union { > struct dpif_flow_put flow_put; > struct dpif_flow_del flow_del; > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 8e4897202..85071e852 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -50,6 +50,7 @@ > #define UPCALL_MAX_BATCH 64 > #define REVALIDATE_MAX_BATCH 50 > #define UINT64_THREE_QUARTERS (UINT64_MAX / 4 * 3) > +#define OFFLOAD_DEFERRED_RETRY_MSEC 2000 > > VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall); > > @@ -347,6 +348,8 @@ struct udpif_key { > #define OFFL_REBAL_INTVL_MSEC 3000 /* dynamic offload rebalance freq */ > struct netdev *in_netdev; /* in_odp_port's netdev */ > bool offloaded; /* True if flow is offloaded */ > + bool offload_deferred; /* Deferred, retry next cycle. */ > + long long int offload_deferred_expire; /* Retry deadline in msec. */ > uint64_t flow_pps_rate; /* Packets-Per-Second rate */ > long long int flow_time; /* last pps update time */ > uint64_t flow_packets; /* #pkts seen in interval */ > @@ -1746,6 +1749,11 @@ handle_upcalls(struct udpif *udpif, struct upcall > *upcalls, > transition_ukey(ukey, UKEY_EVICTED); > } else if (ukey->state < UKEY_OPERATIONAL) { > transition_ukey(ukey, UKEY_OPERATIONAL); > + if (ops[i].dop.offload_deferred) { > + ukey->offload_deferred_expire = > + time_msec() + OFFLOAD_DEFERRED_RETRY_MSEC; > + ukey->offload_deferred = true; > + } > } > ovs_mutex_unlock(&ukey->mutex); > } > @@ -1836,6 +1844,8 @@ ukey_create__(const struct nlattr *key, size_t key_len, > ukey->xcache = NULL; > > ukey->offloaded = false; > + ukey->offload_deferred = false; > + ukey->offload_deferred_expire = 0; > ukey->in_netdev = NULL; > ukey->flow_packets = ukey->flow_backlog_packets = 0; > > @@ -2552,6 +2562,18 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, > size_t n_ops) > for (i = 0; i < n_ops; i++) { > struct ukey_op *op = &ops[i]; > > + if (op->ukey && op->dop.offload_deferred) { > + long long int now = time_msec(); > + > + ovs_mutex_lock(&op->ukey->mutex); > + if (now >= op->ukey->offload_deferred_expire) { > + op->ukey->offload_deferred_expire = > + now + OFFLOAD_DEFERRED_RETRY_MSEC; > + } > + op->ukey->offload_deferred = true; > + ovs_mutex_unlock(&op->ukey->mutex); > + } > + > if (op->dop.error) { > if (op->ukey) { > ovs_mutex_lock(&op->ukey->mutex); > @@ -2984,6 +3006,11 @@ revalidate(struct revalidator *revalidator) > /* Takes ownership of 'recircs'. */ > reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs, > &odp_actions); > + } else if (ukey->offload_deferred) { > + ukey->offload_deferred = false; > + if (time_msec() < ukey->offload_deferred_expire) { > + put_op_init(&ops[n_ops++], ukey, DPIF_FP_MODIFY); > + } > } > ovs_mutex_unlock(&ukey->mutex); > } > diff --git a/tests/system-offloads-traffic.at > b/tests/system-offloads-traffic.at > index 55c26a0ce..484654cb1 100644 > --- a/tests/system-offloads-traffic.at > +++ b/tests/system-offloads-traffic.at > @@ -1200,6 +1200,74 @@ OVS_WAIT_WHILE( > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([offloads - split recirc rules kernel vs offload]) > +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . > other_config:hw-offload=true]) > + > +ADD_NAMESPACES(at_ns0, at_ns1) > + > +ADD_VETH(p1, at_ns0, br0, "10.0.0.1/24") > +ADD_VETH(p2, at_ns1, br0, "10.0.0.2/24") > + > +dnl Two source IPs on the same veth guarantee the same in_port and therefore > +dnl the same recirc ID R1 for the second datapath flow. > +NS_CHECK_EXEC([at_ns0], [ip addr add 10.0.0.3/24 dev p1]) > + > +AT_CHECK([ovs-vsctl -- set interface ovs-p1 ofport_request=1 \ > + -- set interface ovs-p2 ofport_request=2]) > + > +AT_CHECK([ovs-appctl vlog/set dpif_offload_tc_netdev:dbg]) > + > +dnl controller() is not TC-offloadable, so both datapath flows land in > dp:ovs. > +AT_DATA([flows.txt], [dnl > +table=0,arp,actions=NORMAL > +table=0,priority=100,in_port=ovs-p1,ip,nw_dst=10.0.0.2,actions=controller(),ct(commit,table=1) > +table=0,priority=100,in_port=ovs-p2,ip,actions=ovs-p1 > +table=1,ip,actions=ovs-p2 > +]) > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2 10.0.0.2 | FORMAT_PING], > [0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > +AT_CHECK([ovs-appctl revalidator/wait]) > + > +AT_CHECK([grep -q "match for chain .* failed due to non-existing goto chain > action" \ > + ovs-vswitchd.log]) > +AT_CHECK([ovs-appctl dpctl/dump-flows --names type=ovs \ > + filter='in_port(ovs-p1),ipv4' | grep -c "in_port(ovs-p1)"], [0], [2 > +]) > +AT_CHECK([ovs-appctl dpctl/dump-flows --names type=tc \ > + filter='in_port(ovs-p1),ipv4'], [0], [dnl > +]) > + > +dnl Replace the rule with plain ct (TC-offloadable). Traffic from 10.0.0.3 > +dnl offloads its first datapath flow to dp:tc, which registers the recirc ID. > +dnl After the fix, the second datapath flow should also be offloaded to > dp:tc. > +AT_CHECK([ovs-ofctl del-flows br0 > "table=0,in_port=ovs-p1,ip,nw_dst=10.0.0.2"]) > +AT_DATA([flows2.txt], [dnl > +table=0,priority=100,in_port=ovs-p1,ip,nw_dst=10.0.0.2,actions=ct(commit,table=1) > +]) > +AT_CHECK([ovs-ofctl add-flows br0 flows2.txt]) > + > +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2 -I 10.0.0.3 10.0.0.2 | > FORMAT_PING], [0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > +AT_CHECK([ovs-appctl revalidator/wait]) > +OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-flows --names type=tc \ > + filter='in_port(ovs-p1),ipv4' | grep -q "actions:ovs-p2"]) > + > +AT_CHECK([ovs-appctl dpctl/dump-flows --names type=tc \ > + filter='in_port(ovs-p1),ipv4' | grep -c "in_port(ovs-p1)"], [0], [2 > +]) > +AT_CHECK([ovs-appctl dpctl/dump-flows --names type=ovs \ > + filter='in_port(ovs-p1),ipv4' | grep "actions:ovs-p2"], [1]) > +AT_CHECK([ovs-appctl dpctl/dump-flows --names type=tc \ > + filter='in_port(ovs-p1),ipv4' | grep "actions:ovs-p2"], [0], > [ignore]) > + > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([offloads - ovs-appctl dpif/offload/show - offloads enabled]) > AT_KEYWORDS([dpif-offload]) > OVS_TRAFFIC_VSWITCHD_START([], [], > -- > 2.54.0 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
