numan On Wed, Sep 16, 2026, 5:32 a.m. Eelco Chaudron <[email protected]> wrote:
> > > 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. Acked-by: Numan Siddique <[email protected]> Tested-by: Numan Siddique <[email protected]> Numan > > > 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=contro > ller(), <https://www.google.com/maps/search/ller(),?entry=gmail&source=g> > 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
