On 11 Dec 2025, at 15:54, Eli Britstein wrote:
>> -----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. ACK, done, will be in the v3. >> 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
