On 16 Sep 2026, at 5:26, Numan Siddique wrote:
> 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. Maybe you can take a stab at it, so we get this jump started? > I'll take a look, test again and provide my Tested-by tag soon. Thanks... > Can you please add the below tag in the commit message ? > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2026-June/432774.html I'll add it, as well as your test tag once I receive it. > 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
