>-----Original Message----- >From: Eelco Chaudron <[email protected]> >Sent: Thursday, 11 December 2025 15:36 >To: Eli Britstein <[email protected]> >Cc: [email protected] >Subject: Re: [ovs-dev] [PATCH v2 16/41] dpif-offload: Move >hw_miss_packet_postprocess netdev API to dpif. > >External email: Use caution opening links or attachments > > >On 11 Dec 2025, at 11:05, Eli Britstein wrote: > >>> -----Original Message----- >>> From: dev <[email protected]> On Behalf Of Eelco >>> Chaudron >>> Sent: Tuesday, 2 December 2025 16:05 >>> To: [email protected] >>> Subject: [ovs-dev] [PATCH v2 16/41] 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. >> I think "recover" is still a better name than "postprocess". >> If a packet was partially processed in the HW and missed to SW, need to set >its state to be as if it was processed in SW from the beginning. >> In current netdev-offload-dpdk, the only scenario currently supported is with >tunnel decap. >> With CT we will have to recover more metadata of the packet, like recirc-id, >ct-state/mark/label. >> With dp-hash we will have to recover the hash value calculated by the HW. >> The term "postprocess" might imply some processing done after further SW >processing but it's the not. >> I also agree "recover" might not be the perfect term, but still better than >"postprocess". >> In the API definition comment, you also mention "recover": >> " * Recover and/or set the packet state ..." >> If you still stick with renaming, need to be consistent and also change >function names like netdev_offload_dpdk_hw_miss_packet_recover. >> WDYT? > >Hi Eli, > >Thanks for the feedback. I understand your concern about the naming. Here >are some points on why I chose hw_miss_packet_postprocess() and my >thoughts on alternatives: > >- I used “postprocess” in the context of post-hardware processing, not general >SW processing. >- In a later patch, this function will be used for flow lookup as part of >partial HW >offload, so it’s more general than just “recovering” a packet. >- This is a general callback for anything that needs HW post-processing, hence >the name (not SW processing). >- The name already contains hw, so it conveys the hardware context. >- We could consider removing packet_miss from the name entirely to >emphasize the generality, e.g., hw_post_processing(). Yes. I think it makes more sense. Please align the other function names as well. > >WDYT? I’m happy to settle on a name that is both general enough for future >use and clear about its purpose. > >//Eelco > >>> >>> Signed-off-by: Eelco Chaudron <[email protected]> >>> --- >>> 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(-) >>> >>> 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); >> Blank line > >Already has a blank line. >>> >>> - 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 60d6c5321..78cf5d7fa 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 >>> 99cfe2aac..8ac0f8f96 >>> 100644 >>> --- a/lib/dpif-offload.c >>> +++ b/lib/dpif-offload.c >>> @@ -846,6 +846,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 >>> fb7edf269..fe73b278d >>> 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. >>> */
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
