diff mbox series

[ovs-dev,16/41] dpif-offload: Move hw_miss_packet_postprocess netdev API to dpif.

Message ID 1fce8af49ca4563047827aead37339a4891523b7.1762950453.git.echaudro@redhat.com
State Changes Requested
Headers show
Series Architectural refactoring of hardware offload infrastructure | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Eelco Chaudron Nov. 12, 2025, 3:35 p.m. UTC 
This patch moves the hardware packet miss recovery API from the
netdev-offload to the dpif-offload provider. The API name has been
changed from hw_miss_packet_recover() to hw_miss_packet_postprocess()
to reflect that it may also be used for other tasks in the future,
such as conntrack post-processing.

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
 lib/dpif-netdev.c             | 18 +++++++++++-------
 lib/dpif-offload-dpdk.c       | 10 ++++++++++
 lib/dpif-offload-provider.h   | 12 ++++++++++++
 lib/dpif-offload.c            | 36 +++++++++++++++++++++++++++++++++++
 lib/dpif-offload.h            |  2 ++
 lib/netdev-offload-dpdk.c     |  3 +--
 lib/netdev-offload-dpdk.h     |  2 ++
 lib/netdev-offload-provider.h |  6 ------
 lib/netdev-offload.c          | 33 +++-----------------------------
 lib/netdev-offload.h          |  4 ++--
 lib/netdev.c                  |  2 +-
 11 files changed, 80 insertions(+), 48 deletions(-)

Comments

0-day Robot Nov. 14, 2025, 2:16 p.m. UTC | #1 
git sha: 1fce8af49ca4563047827aead37339a4891523b7
Author: Eelco Chaudron <echaudro@redhat.com>
Subject: dpif-offload: Move hw_miss_packet_postprocess netdev API to dpif.

This patch moves the hardware packet miss recovery API from the
netdev-offload to the dpif-offload provider. The API name has been
changed from hw_miss_packet_recover() to hw_miss_packet_postprocess()
to reflect that it may also be used for other tasks in the future,
such as conntrack post-processing.

> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 82df85823..5d32a47b3 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -8423,14 +8423,18 @@ dp_netdev_hw_flow(const struct dp_netdev_pmd_thread *pmd,
>  #ifdef ALLOW_EXPERIMENTAL_API /* Packet restoration API required. */
>      /* Restore the packet if HW processing was terminated before completion. */
>      struct dp_netdev_rxq *rxq = pmd->ctx.last_rxq;
> -    bool miss_api_supported;
> +    bool postprocess_api_supported;
> +
> +    atomic_read_relaxed(&rxq->port->netdev->hw_info.postprocess_api_supported,
> +                        &postprocess_api_supported);
> +    if (postprocess_api_supported) {
> +        int err = dpif_offload_netdev_hw_miss_packet_postprocess(
> +            rxq->port->netdev, packet);
>  
> -    atomic_read_relaxed(&rxq->port->netdev->hw_info.miss_api_supported,
> -                        &miss_api_supported);
> -    if (miss_api_supported) {
> -        int err = netdev_hw_miss_packet_recover(rxq->port->netdev, packet);
>          if (err && err != EOPNOTSUPP) {
> -            COVERAGE_INC(datapath_drop_hw_miss_recover);
> +            if (err != ECANCELED) {
> +                COVERAGE_INC(datapath_drop_hw_miss_postprocess);
> +            }
>              return -1;
>          }
>      }

This introduces a behavior change in error handling. The original code
incremented coverage for all errors except EOPNOTSUPP, but now the code
only increments coverage if err != ECANCELED. Does this change in
coverage reporting mask potentially important error conditions?

[ ... ]

> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index bb2a63323..12dac4239 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
> @@ -835,6 +835,42 @@ dpif_offload_netdev_flush_flows(struct netdev *netdev)
>      return EOPNOTSUPP;
>  }
>  
> +int
> +dpif_offload_netdev_hw_miss_packet_postprocess(struct netdev *netdev,
> +                                               struct dp_packet *packet)
> +{
> +    const struct dpif_offload *offload;
> +    bool postprocess_api_supported;
> +    int rc;
> +
> +    atomic_read_relaxed(&netdev->hw_info.postprocess_api_supported,
> +                        &postprocess_api_supported);
> +    if (!postprocess_api_supported) {
> +        return EOPNOTSUPP;
> +    }
> +
> +    offload = ovsrcu_get(const struct dpif_offload *, &netdev->dpif_offload);
> +
> +    if (!offload || !offload->class->netdev_hw_miss_packet_postprocess) {
> +        if (offload) {
> +            /* Offload is configured and API unsupported by the port;
> +             * avoid subsequent calls. */
> +            atomic_store_relaxed(&netdev->hw_info.postprocess_api_supported,
> +                                 false);
> +        }
> +        return EOPNOTSUPP;
> +    }
> +
> +    rc = offload->class->netdev_hw_miss_packet_postprocess(offload, netdev,
> +                                                           packet);
> +    if (rc == EOPNOTSUPP) {
> +        /* API unsupported by the port; avoid subsequent calls. */
> +        atomic_store_relaxed(&netdev->hw_info.postprocess_api_supported,
> +                             false);
> +    }
> +    return rc;
> +}
> +

Could this function race with initialization where postprocess_api_supported
gets set to false after being initially set to true? What happens if the
atomic flag gets modified between checking it and using the offload pointer?

[ ... ]

> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 0c4209290..be5787f4d 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -186,7 +186,8 @@ netdev_assign_flow_api(struct netdev *netdev)
>      CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
>          if (!rfa->flow_api->init_flow_api(netdev)) {
>              ovs_refcount_ref(&rfa->refcnt);
> -            atomic_store_relaxed(&netdev->hw_info.miss_api_supported, true);
> +            atomic_store_relaxed(&netdev->hw_info.postprocess_api_supported,
> +                                 true);
>              ovsrcu_set(&netdev->flow_api, rfa->flow_api);
>              VLOG_INFO("%s: Assigned flow API '%s'.",
>                        netdev_get_name(netdev), rfa->flow_api->type);

This unconditionally sets postprocess_api_supported to true when any flow
API initializes successfully. However, not all flow APIs may support the
postprocess functionality. Should this check if the specific API actually
supports postprocessing before setting the flag?

[ ... ]

> -int
> -netdev_hw_miss_packet_recover(struct netdev *netdev,
> -                              struct dp_packet *packet)
> -{
> -    const struct netdev_flow_api *flow_api;
> -    bool miss_api_supported;
> -    int rv;
> -
> -    atomic_read_relaxed(&netdev->hw_info.miss_api_supported,
> -                        &miss_api_supported);
> -    if (!miss_api_supported) {
> -        return EOPNOTSUPP;
> -    }
> -
> -    flow_api = ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api);
> -    if (!flow_api || !flow_api->hw_miss_packet_recover) {
> -        return EOPNOTSUPP;
> -    }
> -
> -    rv = flow_api->hw_miss_packet_recover(netdev, packet);
> -    if (rv == EOPNOTSUPP) {
> -        /* API unsupported by the port; avoid subsequent calls. */
> -        atomic_store_relaxed(&netdev->hw_info.miss_api_supported, false);
> -    }
> -
> -    return rv;
> -}

The removed function netdev_hw_miss_packet_recover had different error
handling behavior compared to the new dpif_offload_netdev_hw_miss_packet_postprocess.
The old version only disabled the flag on EOPNOTSUPP, while the new version
in dpif-netdev.c has special handling for ECANCELED. Could this behavioral
change cause issues for existing callers?
diff mbox series

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 82df85823..5d32a47b3 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -122,7 +122,7 @@  COVERAGE_DEFINE(datapath_drop_invalid_bond);
 COVERAGE_DEFINE(datapath_drop_invalid_tnl_port);
 COVERAGE_DEFINE(datapath_drop_rx_invalid_packet);
 #ifdef ALLOW_EXPERIMENTAL_API /* Packet restoration API required. */
-COVERAGE_DEFINE(datapath_drop_hw_miss_recover);
+COVERAGE_DEFINE(datapath_drop_hw_miss_postprocess);
 #endif
 
 /* Protects against changes to 'dp_netdevs'. */
@@ -8423,14 +8423,18 @@  dp_netdev_hw_flow(const struct dp_netdev_pmd_thread *pmd,
 #ifdef ALLOW_EXPERIMENTAL_API /* Packet restoration API required. */
     /* Restore the packet if HW processing was terminated before completion. */
     struct dp_netdev_rxq *rxq = pmd->ctx.last_rxq;
-    bool miss_api_supported;
+    bool postprocess_api_supported;
+
+    atomic_read_relaxed(&rxq->port->netdev->hw_info.postprocess_api_supported,
+                        &postprocess_api_supported);
+    if (postprocess_api_supported) {
+        int err = dpif_offload_netdev_hw_miss_packet_postprocess(
+            rxq->port->netdev, packet);
 
-    atomic_read_relaxed(&rxq->port->netdev->hw_info.miss_api_supported,
-                        &miss_api_supported);
-    if (miss_api_supported) {
-        int err = netdev_hw_miss_packet_recover(rxq->port->netdev, packet);
         if (err && err != EOPNOTSUPP) {
-            COVERAGE_INC(datapath_drop_hw_miss_recover);
+            if (err != ECANCELED) {
+                COVERAGE_INC(datapath_drop_hw_miss_postprocess);
+            }
             return -1;
         }
     }
diff --git a/lib/dpif-offload-dpdk.c b/lib/dpif-offload-dpdk.c
index 76a8946c9..c8ae1adb2 100644
--- a/lib/dpif-offload-dpdk.c
+++ b/lib/dpif-offload-dpdk.c
@@ -286,6 +286,14 @@  dpif_offload_dpdk_netdev_flow_flush(const struct dpif_offload *offload
     return netdev_offload_dpdk_flow_flush(netdev);
 }
 
+static int
+dpif_offload_dpdk_netdev_hw_miss_packet_postprocess(
+    const struct dpif_offload *offload_ OVS_UNUSED, struct netdev *netdev,
+    struct dp_packet *packet)
+{
+    return netdev_offload_dpdk_hw_miss_packet_recover(netdev, packet);
+}
+
 struct dpif_offload_class dpif_offload_dpdk_class = {
     .type = "dpdk",
     .supported_dpif_types = (const char *const[]) {
@@ -300,6 +308,8 @@  struct dpif_offload_class dpif_offload_dpdk_class = {
     .port_del = dpif_offload_dpdk_port_del,
     .flow_get_n_offloaded = dpif_offload_dpdk_get_n_offloaded,
     .netdev_flow_flush = dpif_offload_dpdk_netdev_flow_flush,
+    .netdev_hw_miss_packet_postprocess = \
+        dpif_offload_dpdk_netdev_hw_miss_packet_postprocess,
 };
 
 /* XXX: Temporary functions below, which will be removed once fully
diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h
index c3cc80dfe..974cf0ae1 100644
--- a/lib/dpif-offload-provider.h
+++ b/lib/dpif-offload-provider.h
@@ -168,6 +168,18 @@  struct dpif_offload_class {
     /* Deletes all offloaded flows on this netdev.  Return 0 if successful,
      *  otherwise returns a positive errno value. */
     int (*netdev_flow_flush)(const struct dpif_offload *, struct netdev *);
+
+    /* Recover and/or set the packet state (contents and metadata) for
+     * continued processing in software, and perform any additional
+     * post-processing required by the offload provider.
+     *
+     * Return 0 if successful and the packet requires further processing;
+     * otherwise, return a positive errno value and take ownership of the
+     * packet if errno != EOPNOTSUPP.  Return ECANCELED if the packet was
+     * fully consumed by the provider for non-error conditions. */
+    int (*netdev_hw_miss_packet_postprocess)(const struct dpif_offload *,
+                                             struct netdev *,
+                                             struct dp_packet *);
 };
 
 
diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
index bb2a63323..12dac4239 100644
--- a/lib/dpif-offload.c
+++ b/lib/dpif-offload.c
@@ -835,6 +835,42 @@  dpif_offload_netdev_flush_flows(struct netdev *netdev)
     return EOPNOTSUPP;
 }
 
+int
+dpif_offload_netdev_hw_miss_packet_postprocess(struct netdev *netdev,
+                                               struct dp_packet *packet)
+{
+    const struct dpif_offload *offload;
+    bool postprocess_api_supported;
+    int rc;
+
+    atomic_read_relaxed(&netdev->hw_info.postprocess_api_supported,
+                        &postprocess_api_supported);
+    if (!postprocess_api_supported) {
+        return EOPNOTSUPP;
+    }
+
+    offload = ovsrcu_get(const struct dpif_offload *, &netdev->dpif_offload);
+
+    if (!offload || !offload->class->netdev_hw_miss_packet_postprocess) {
+        if (offload) {
+            /* Offload is configured and API unsupported by the port;
+             * avoid subsequent calls. */
+            atomic_store_relaxed(&netdev->hw_info.postprocess_api_supported,
+                                 false);
+        }
+        return EOPNOTSUPP;
+    }
+
+    rc = offload->class->netdev_hw_miss_packet_postprocess(offload, netdev,
+                                                           packet);
+    if (rc == EOPNOTSUPP) {
+        /* API unsupported by the port; avoid subsequent calls. */
+        atomic_store_relaxed(&netdev->hw_info.postprocess_api_supported,
+                             false);
+    }
+    return rc;
+}
+
 
 struct dpif_offload_port_mgr *
 dpif_offload_port_mgr_init(void)
diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h
index a75c80612..c7d69c3bc 100644
--- a/lib/dpif-offload.h
+++ b/lib/dpif-offload.h
@@ -75,5 +75,7 @@  void dpif_offload_meter_del(const struct dpif *dpif, ofproto_meter_id meter_id,
 
 /* Netdev specific function, which can be used in the fast path. */
 int dpif_offload_netdev_flush_flows(struct netdev *);
+int dpif_offload_netdev_hw_miss_packet_postprocess(struct netdev *,
+                                                   struct dp_packet *);
 
 #endif /* DPIF_OFFLOAD_H */
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index d1781d8e4..c3d5e83f5 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -2685,7 +2685,7 @@  get_vport_netdev(const char *dpif_type,
     return aux.vport;
 }
 
-static int
+int
 netdev_offload_dpdk_hw_miss_packet_recover(struct netdev *netdev,
                                            struct dp_packet *packet)
 {
@@ -2803,5 +2803,4 @@  const struct netdev_flow_api netdev_offload_dpdk = {
     .init_flow_api = netdev_offload_dpdk_init_flow_api,
     .uninit_flow_api = netdev_offload_dpdk_uninit_flow_api,
     .flow_get = netdev_offload_dpdk_flow_get,
-    .hw_miss_packet_recover = netdev_offload_dpdk_hw_miss_packet_recover,
 };
diff --git a/lib/netdev-offload-dpdk.h b/lib/netdev-offload-dpdk.h
index d5061b40c..475822e1b 100644
--- a/lib/netdev-offload-dpdk.h
+++ b/lib/netdev-offload-dpdk.h
@@ -24,5 +24,7 @@  struct netdev;
  * associated dpif offload provider. */
 int netdev_offload_dpdk_flow_flush(struct netdev *);
 uint64_t netdev_offload_dpdk_flow_get_n_offloaded(struct netdev *);
+int netdev_offload_dpdk_hw_miss_packet_recover(struct netdev *,
+                                               struct dp_packet *);
 
 #endif /* NETDEV_OFFLOAD_DPDK_H */
diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h
index 898df8333..f762af19a 100644
--- a/lib/netdev-offload-provider.h
+++ b/lib/netdev-offload-provider.h
@@ -80,12 +80,6 @@  struct netdev_flow_api {
     int (*flow_del)(struct netdev *, const ovs_u128 *ufid,
                     struct dpif_flow_stats *);
 
-    /* Recover the packet state (contents and data) for continued processing
-     * in software.
-     * Return 0 if successful, otherwise returns a positive errno value and
-     * takes ownership of a packet if errno != EOPNOTSUPP. */
-    int (*hw_miss_packet_recover)(struct netdev *, struct dp_packet *);
-
     /* 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.c b/lib/netdev-offload.c
index 0c4209290..be5787f4d 100644
--- a/lib/netdev-offload.c
+++ b/lib/netdev-offload.c
@@ -186,7 +186,8 @@  netdev_assign_flow_api(struct netdev *netdev)
     CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
         if (!rfa->flow_api->init_flow_api(netdev)) {
             ovs_refcount_ref(&rfa->refcnt);
-            atomic_store_relaxed(&netdev->hw_info.miss_api_supported, true);
+            atomic_store_relaxed(&netdev->hw_info.postprocess_api_supported,
+                                 true);
             ovsrcu_set(&netdev->flow_api, rfa->flow_api);
             VLOG_INFO("%s: Assigned flow API '%s'.",
                       netdev_get_name(netdev), rfa->flow_api->type);
@@ -195,7 +196,7 @@  netdev_assign_flow_api(struct netdev *netdev)
         VLOG_DBG("%s: flow API '%s' is not suitable.",
                  netdev_get_name(netdev), rfa->flow_api->type);
     }
-    atomic_store_relaxed(&netdev->hw_info.miss_api_supported, false);
+    atomic_store_relaxed(&netdev->hw_info.postprocess_api_supported, false);
     VLOG_INFO("%s: No suitable flow API found.", netdev_get_name(netdev));
 
     return -1;
@@ -254,34 +255,6 @@  netdev_flow_put(struct netdev *netdev, struct match *match,
            : EOPNOTSUPP;
 }
 
-int
-netdev_hw_miss_packet_recover(struct netdev *netdev,
-                              struct dp_packet *packet)
-{
-    const struct netdev_flow_api *flow_api;
-    bool miss_api_supported;
-    int rv;
-
-    atomic_read_relaxed(&netdev->hw_info.miss_api_supported,
-                        &miss_api_supported);
-    if (!miss_api_supported) {
-        return EOPNOTSUPP;
-    }
-
-    flow_api = ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api);
-    if (!flow_api || !flow_api->hw_miss_packet_recover) {
-        return EOPNOTSUPP;
-    }
-
-    rv = flow_api->hw_miss_packet_recover(netdev, packet);
-    if (rv == EOPNOTSUPP) {
-        /* API unsupported by the port; avoid subsequent calls. */
-        atomic_store_relaxed(&netdev->hw_info.miss_api_supported, false);
-    }
-
-    return rv;
-}
-
 int
 netdev_flow_get(struct netdev *netdev, struct match *match,
                 struct nlattr **actions, const ovs_u128 *ufid,
diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
index 2b32179ec..6006396b9 100644
--- a/lib/netdev-offload.h
+++ b/lib/netdev-offload.h
@@ -47,7 +47,8 @@  struct ovs_action_push_tnl;
 /* Offload-capable (HW) netdev information */
 struct netdev_hw_info {
     bool oor;                         /* Out of Offload Resources ? */
-    atomic_bool miss_api_supported;   /* hw_miss_packet_recover() supported.*/
+    /* Is hw_miss_packet_postprocess() supported.*/
+    atomic_bool postprocess_api_supported;
     int offload_count;                /* Offloaded flow count */
     int pending_count;                /* Pending (non-offloaded) flow count */
     OVSRCU_TYPE(void *) offload_data; /* Offload metadata. */
@@ -110,7 +111,6 @@  bool netdev_flow_dump_next(struct netdev_flow_dump *, struct match *,
 int netdev_flow_put(struct netdev *, struct match *, struct nlattr *actions,
                     size_t actions_len, const ovs_u128 *,
                     struct offload_info *, struct dpif_flow_stats *);
-int netdev_hw_miss_packet_recover(struct netdev *, struct dp_packet *);
 int netdev_flow_get(struct netdev *, struct match *, struct nlattr **actions,
                     const ovs_u128 *, struct dpif_flow_stats *,
                     struct dpif_flow_attrs *, struct ofpbuf *wbuffer);
diff --git a/lib/netdev.c b/lib/netdev.c
index 6a05e9a7e..13f3d707e 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -435,7 +435,7 @@  netdev_open(const char *name, const char *type, struct netdev **netdevp)
                     seq_read(netdev->reconfigure_seq);
                 ovsrcu_set(&netdev->flow_api, NULL);
                 netdev->hw_info.oor = false;
-                atomic_init(&netdev->hw_info.miss_api_supported, false);
+                atomic_init(&netdev->hw_info.postprocess_api_supported, false);
                 netdev->node = shash_add(&netdev_shash, name, netdev);
 
                 /* By default enable one tx and rx queue per netdev. */