| Message ID | 20230926140641.493252-1-david.marchand@redhat.com |
|---|---|
| State | Under Review |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,dpdk-latest] netdev-offload: Use per packet tunnel metadata restore flag. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 9/26/23 16:06, David Marchand wrote: > Since v23.07, DPDK provides a per packet flag that indicates if a call > to the optional rte_flow_restore_tunnel_info() is necessary. > There is, then, no need at runtime to discover if a driver supports > this feature. Hi, David. Thanks for the patch. Do you have some performance data comparing the previous and the new code for non-offloaded traffic? Currently we have one atomic read per packet arriving from a port that doesn't support tunnel offloading. With this change we have two atomic reads and an indirect function call per packet. That might have some measurable performance impact. One more comment below. > > Link: https://git.dpdk.org/dpdk/commit/?id=fca8cba4f1f1 > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > lib/dpif-netdev.c | 15 +++++---------- > lib/netdev-dpdk.c | 10 ++++++++++ > lib/netdev-offload.c | 18 +----------------- > lib/netdev-offload.h | 1 - > lib/netdev.c | 1 - > 5 files changed, 16 insertions(+), 29 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 157694bcf0..aa51e0a67d 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -8207,16 +8207,11 @@ 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; > - > - 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); > - return -1; > - } > + int err = netdev_hw_miss_packet_recover(rxq->port->netdev, packet); > + > + if (err && err != EOPNOTSUPP) { > + COVERAGE_INC(datapath_drop_hw_miss_recover); > + return -1; > } > #endif > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 55700250df..379c50399d 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1223,6 +1223,10 @@ dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex) > } > } > > +#ifdef ALLOW_EXPERIMENTAL_API > +static uint64_t netdev_dpdk_restore_info_flag; > +#endif > + > static void > dpdk_eth_dev_init_rx_metadata(struct netdev_dpdk *dev) > { > @@ -1251,6 +1255,8 @@ dpdk_eth_dev_init_rx_metadata(struct netdev_dpdk *dev) > if (!(rx_metadata & RTE_ETH_RX_METADATA_TUNNEL_ID)) { > VLOG_DBG("%s: The NIC will not provide per-packet TUNNEL_ID", > netdev_get_name(&dev->up)); > + } else if (!netdev_dpdk_restore_info_flag) { > + netdev_dpdk_restore_info_flag = rte_flow_restore_info_dynflag(); > } > #endif /* ALLOW_EXPERIMENTAL_API */ > } else { > @@ -6230,6 +6236,10 @@ netdev_dpdk_rte_flow_get_restore_info(struct netdev *netdev, > return -1; > } > > + if (!(p->mbuf.ol_flags & netdev_dpdk_restore_info_flag)) { > + return -EOPNOTSUPP; I'm not sure this is a good error code for the situation. Can we find a good way to check the flags higher up the stack? Logically, the recovery API should not be even called if the flags are not set. It seems tricky to do with dynamic flags though. Best regards, Ilya Maximets.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 157694bcf0..aa51e0a67d 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -8207,16 +8207,11 @@ 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; - - 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); - return -1; - } + int err = netdev_hw_miss_packet_recover(rxq->port->netdev, packet); + + if (err && err != EOPNOTSUPP) { + COVERAGE_INC(datapath_drop_hw_miss_recover); + return -1; } #endif diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 55700250df..379c50399d 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1223,6 +1223,10 @@ dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex) } } +#ifdef ALLOW_EXPERIMENTAL_API +static uint64_t netdev_dpdk_restore_info_flag; +#endif + static void dpdk_eth_dev_init_rx_metadata(struct netdev_dpdk *dev) { @@ -1251,6 +1255,8 @@ dpdk_eth_dev_init_rx_metadata(struct netdev_dpdk *dev) if (!(rx_metadata & RTE_ETH_RX_METADATA_TUNNEL_ID)) { VLOG_DBG("%s: The NIC will not provide per-packet TUNNEL_ID", netdev_get_name(&dev->up)); + } else if (!netdev_dpdk_restore_info_flag) { + netdev_dpdk_restore_info_flag = rte_flow_restore_info_dynflag(); } #endif /* ALLOW_EXPERIMENTAL_API */ } else { @@ -6230,6 +6236,10 @@ netdev_dpdk_rte_flow_get_restore_info(struct netdev *netdev, return -1; } + if (!(p->mbuf.ol_flags & netdev_dpdk_restore_info_flag)) { + return -EOPNOTSUPP; + } + dev = netdev_dpdk_cast(netdev); ovs_mutex_lock(&dev->mutex); ret = rte_flow_get_restore_info(dev->port_id, m, info, error); diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index a5fa624875..fce09565dd 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -183,7 +183,6 @@ 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); ovsrcu_set(&netdev->flow_api, rfa->flow_api); VLOG_INFO("%s: Assigned flow API '%s'.", netdev_get_name(netdev), rfa->flow_api->type); @@ -192,7 +191,6 @@ 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); VLOG_INFO("%s: No suitable flow API found.", netdev_get_name(netdev)); return -1; @@ -325,27 +323,13 @@ 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; + return flow_api->hw_miss_packet_recover(netdev, packet); } int diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h index 47f8e6f48b..ba5f203562 100644 --- a/lib/netdev-offload.h +++ b/lib/netdev-offload.h @@ -47,7 +47,6 @@ 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.*/ int offload_count; /* Pending (non-offloaded) flow count */ int pending_count; /* Offloaded flow count */ OVSRCU_TYPE(void *) offload_data; /* Offload metadata. */ diff --git a/lib/netdev.c b/lib/netdev.c index e5ac7713d2..5945aacae4 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -432,7 +432,6 @@ 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); netdev->node = shash_add(&netdev_shash, name, netdev); /* By default enable one tx and rx queue per netdev. */
Since v23.07, DPDK provides a per packet flag that indicates if a call to the optional rte_flow_restore_tunnel_info() is necessary. There is, then, no need at runtime to discover if a driver supports this feature. Link: https://git.dpdk.org/dpdk/commit/?id=fca8cba4f1f1 Signed-off-by: David Marchand <david.marchand@redhat.com> --- lib/dpif-netdev.c | 15 +++++---------- lib/netdev-dpdk.c | 10 ++++++++++ lib/netdev-offload.c | 18 +----------------- lib/netdev-offload.h | 1 - lib/netdev.c | 1 - 5 files changed, 16 insertions(+), 29 deletions(-)