On 7/6/26 5:27 PM, Eelco Chaudron via dev 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]>
> ---
> 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(-)
Hi, Eelco and Numan. Thanks for working on this and sorry for delay.
I have a few concerns about this solution:
1. The EAGAIN may be returned for different cases, not just the missing
chain, but the code and the comments in the generic code above seems
to assume that. Maybe we should introduce a flag for the info
structure and pass it up from the tc_netdev_flow_put() while keep
returning EOPNOTSUPP?
2. The time-based re-tries do not seem robust enough. Various operations
could take more than two seconds under the high load and we may miss
the window to re-try. Maybe a better solution would be to expose a
sequence number from the dpif-offload module that providers can bump
when they anticipate potential change in the offloadability? E.g.
the tc provider could bump this sequence number when ccmap_inc()
returns one, i.e. when the new chain is used. The revalidators could
then re-try deferred flows that have lower sequence number and update
it when it still fails, so it's not retried more than once per change.
We would still probbaly want a global limit, so we do not re-try
forever, but maybe someting like a flat attmepts counter per ukey
would suffice, e.g. no more than 16 attempts, or something like this.
What do you think?
A few more comments below.
>
> 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. */
This is not always the case, right? Only in case of AUTO.
> + VLOG_DBG_RL(&rl, "match for chain %u failed due to non-existing "
> + "goto chain action",
> + chain);
nit: keep on the same line.
> + 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;
> + }
May it be possible that revlidator move it into operation state already?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
