| Message ID | 20230630024630.6464-2-ivan.malov@arknetworks.am |
|---|---|
| State | Superseded |
| Headers | show |
| Series | DPDK: align flow offloads with 22.11 | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 6/30/23 04:46, Ivan Malov wrote: > This may be required by some PMDs in offload scenarios. > > Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am> Hi, Ivan. Thanks for the patch! I suppose, it can be considered as a bug fix. Could you add a Fixes tag to the commit message? > --- > lib/netdev-dpdk.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 63dac689e..d9d1b43f6 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -492,6 +492,9 @@ struct netdev_dpdk { > > /* Array of vhost rxq states, see vring_state_changed. */ > bool *vhost_rxq_enabled; > + > + /* Ensures that Rx metadata delivery is configured only once. */ > + bool rx_metadata_delivery_configured; > ); > > PADDED_MEMBERS(CACHE_LINE_SIZE, > @@ -1187,6 +1190,42 @@ dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex) > } > } > > +static void > +dpdk_eth_dev_init_rx_metadata(struct netdev_dpdk *dev) > +{ > + uint64_t rx_metadata = 0; > + int ret; > + > + if (dev->rx_metadata_delivery_configured) { > + return; > + } > + > + /* For the fallback offload (non-"transfer" rules) */ > + rx_metadata |= RTE_ETH_RX_METADATA_USER_MARK; > + /* For the full offload ("transfer" rules) */ This doesn't seem to be correct. This flag is only needed for tunnel offload. Should also be under experimental api ifdef? > + rx_metadata |= RTE_ETH_RX_METADATA_TUNNEL_ID; > + > + ret = rte_eth_rx_metadata_negotiate(dev->port_id, &rx_metadata); > + if (ret == 0) { > + if (!(rx_metadata & RTE_ETH_RX_METADATA_USER_MARK)) { > + VLOG_DBG("The NIC will not provide per-packet USER_MARK on port " > + DPDK_PORT_ID_FMT, dev->port_id); > + } > + if (!(rx_metadata & RTE_ETH_RX_METADATA_TUNNEL_ID)) { > + VLOG_DBG("The NIC will not provide per-packet TUNNEL_ID on port " > + DPDK_PORT_ID_FMT, dev->port_id); > + } > + } else if (ret == -ENOTSUP) { > + VLOG_DBG("Rx metadata negotiate procedure is not supported on port " > + DPDK_PORT_ID_FMT, dev->port_id); > + } else { > + VLOG_WARN("Cannot negotiate Rx metadata on port " > + DPDK_PORT_ID_FMT, dev->port_id); For all the logs above, use the following format instead: VLOG_*("%s: ...", netdev_get_name(&dev->up)); For the last two, you may use: VLOG(ret == -ENOTSUP ? VLL_DBG : VLL_WARN, "%s: Cannot negotiate Rx metadata: %s", netdev_get_name(&dev->up), rte_strerror(-ret)); > + } > + > + dev->rx_metadata_delivery_configured = true; > +} > + > static int > dpdk_eth_dev_init(struct netdev_dpdk *dev) > OVS_REQUIRES(dev->mutex) > @@ -1200,6 +1239,16 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) > RTE_ETH_RX_OFFLOAD_TCP_CKSUM | > RTE_ETH_RX_OFFLOAD_IPV4_CKSUM; > > + /* > + * Full tunnel offload requires that tunnel ID metadata be > + * delivered with "miss" packets from the hardware to the > + * PMD. The same goes for megaflow mark metadata which is > + * used in MARK + RSS offload scenario. > + * > + * Request delivery of such metadata. > + */ > + dpdk_eth_dev_init_rx_metadata(dev); It's a bit strange that we request metadata regardless of hw-offload being enabled. Should this be under netdev_is_flow_api_enabled() ? > + > rte_eth_dev_info_get(dev->port_id, &info); > > if (strstr(info.driver_name, "vf") != NULL) { > @@ -1382,6 +1431,8 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, > /* Initilize the hardware offload flags to 0 */ > dev->hw_ol_features = 0; > > + dev->rx_metadata_delivery_configured = false; > + > dev->flags = NETDEV_UP | NETDEV_PROMISC; > > ovs_list_push_back(&dpdk_list, &dev->list_node);
Hi Ilya, Thanks for reviewing this. PSB. On Fri, 14 Jul 2023, Ilya Maximets wrote: > On 6/30/23 04:46, Ivan Malov wrote: >> This may be required by some PMDs in offload scenarios. >> >> Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am> > > Hi, Ivan. Thanks for the patch! > I suppose, it can be considered as a bug fix. Could you add > a Fixes tag to the commit message? > >> --- >> lib/netdev-dpdk.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 63dac689e..d9d1b43f6 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -492,6 +492,9 @@ struct netdev_dpdk { >> >> /* Array of vhost rxq states, see vring_state_changed. */ >> bool *vhost_rxq_enabled; >> + >> + /* Ensures that Rx metadata delivery is configured only once. */ >> + bool rx_metadata_delivery_configured; >> ); >> >> PADDED_MEMBERS(CACHE_LINE_SIZE, >> @@ -1187,6 +1190,42 @@ dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex) >> } >> } >> >> +static void >> +dpdk_eth_dev_init_rx_metadata(struct netdev_dpdk *dev) >> +{ >> + uint64_t rx_metadata = 0; >> + int ret; >> + >> + if (dev->rx_metadata_delivery_configured) { >> + return; >> + } >> + >> + /* For the fallback offload (non-"transfer" rules) */ >> + rx_metadata |= RTE_ETH_RX_METADATA_USER_MARK; >> + /* For the full offload ("transfer" rules) */ > > This doesn't seem to be correct. This flag is only needed for > tunnel offload. Should also be under experimental api ifdef? My guess, it's comment which is incorrect. I'm going to fix it, yes. But, regarding the experimental api ifdef, according to review [1] from Eli, it might be unneeded. Furthermore, the dpdk api being invoked is no longer experimental. What do you think? [1] https://patchwork.ozlabs.org/project/openvswitch/patch/20220720121823.2497727-4-ivan.malov@oktetlabs.ru/#2935847 > >> + rx_metadata |= RTE_ETH_RX_METADATA_TUNNEL_ID; >> + >> + ret = rte_eth_rx_metadata_negotiate(dev->port_id, &rx_metadata); >> + if (ret == 0) { >> + if (!(rx_metadata & RTE_ETH_RX_METADATA_USER_MARK)) { >> + VLOG_DBG("The NIC will not provide per-packet USER_MARK on port " >> + DPDK_PORT_ID_FMT, dev->port_id); >> + } >> + if (!(rx_metadata & RTE_ETH_RX_METADATA_TUNNEL_ID)) { >> + VLOG_DBG("The NIC will not provide per-packet TUNNEL_ID on port " >> + DPDK_PORT_ID_FMT, dev->port_id); >> + } >> + } else if (ret == -ENOTSUP) { >> + VLOG_DBG("Rx metadata negotiate procedure is not supported on port " >> + DPDK_PORT_ID_FMT, dev->port_id); >> + } else { >> + VLOG_WARN("Cannot negotiate Rx metadata on port " >> + DPDK_PORT_ID_FMT, dev->port_id); > > For all the logs above, use the following format instead: > > VLOG_*("%s: ...", netdev_get_name(&dev->up)); > > For the last two, you may use: > > VLOG(ret == -ENOTSUP ? VLL_DBG : VLL_WARN, > "%s: Cannot negotiate Rx metadata: %s", > netdev_get_name(&dev->up), rte_strerror(-ret)); > >> + } >> + >> + dev->rx_metadata_delivery_configured = true; >> +} >> + >> static int >> dpdk_eth_dev_init(struct netdev_dpdk *dev) >> OVS_REQUIRES(dev->mutex) >> @@ -1200,6 +1239,16 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) >> RTE_ETH_RX_OFFLOAD_TCP_CKSUM | >> RTE_ETH_RX_OFFLOAD_IPV4_CKSUM; >> >> + /* >> + * Full tunnel offload requires that tunnel ID metadata be >> + * delivered with "miss" packets from the hardware to the >> + * PMD. The same goes for megaflow mark metadata which is >> + * used in MARK + RSS offload scenario. >> + * >> + * Request delivery of such metadata. >> + */ >> + dpdk_eth_dev_init_rx_metadata(dev); > > It's a bit strange that we request metadata regardless of hw-offload > being enabled. Should this be under netdev_is_flow_api_enabled() ? > >> + >> rte_eth_dev_info_get(dev->port_id, &info); >> >> if (strstr(info.driver_name, "vf") != NULL) { >> @@ -1382,6 +1431,8 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, >> /* Initilize the hardware offload flags to 0 */ >> dev->hw_ol_features = 0; >> >> + dev->rx_metadata_delivery_configured = false; >> + >> dev->flags = NETDEV_UP | NETDEV_PROMISC; >> >> ovs_list_push_back(&dpdk_list, &dev->list_node); > >
Hi Ilya, A quick follow-up from me: I made a new version of this patch [1]. [1] https://patchwork.ozlabs.org/project/openvswitch/patch/20230716115720.6789-1-ivan.malov@arknetworks.am/ Thank you. On Fri, 14 Jul 2023, Ilya Maximets wrote: > On 6/30/23 04:46, Ivan Malov wrote: >> This may be required by some PMDs in offload scenarios. >> >> Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am> > > Hi, Ivan. Thanks for the patch! > I suppose, it can be considered as a bug fix. Could you add > a Fixes tag to the commit message? > >> --- >> lib/netdev-dpdk.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 63dac689e..d9d1b43f6 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -492,6 +492,9 @@ struct netdev_dpdk { >> >> /* Array of vhost rxq states, see vring_state_changed. */ >> bool *vhost_rxq_enabled; >> + >> + /* Ensures that Rx metadata delivery is configured only once. */ >> + bool rx_metadata_delivery_configured; >> ); >> >> PADDED_MEMBERS(CACHE_LINE_SIZE, >> @@ -1187,6 +1190,42 @@ dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex) >> } >> } >> >> +static void >> +dpdk_eth_dev_init_rx_metadata(struct netdev_dpdk *dev) >> +{ >> + uint64_t rx_metadata = 0; >> + int ret; >> + >> + if (dev->rx_metadata_delivery_configured) { >> + return; >> + } >> + >> + /* For the fallback offload (non-"transfer" rules) */ >> + rx_metadata |= RTE_ETH_RX_METADATA_USER_MARK; >> + /* For the full offload ("transfer" rules) */ > > This doesn't seem to be correct. This flag is only needed for > tunnel offload. Should also be under experimental api ifdef? > >> + rx_metadata |= RTE_ETH_RX_METADATA_TUNNEL_ID; >> + >> + ret = rte_eth_rx_metadata_negotiate(dev->port_id, &rx_metadata); >> + if (ret == 0) { >> + if (!(rx_metadata & RTE_ETH_RX_METADATA_USER_MARK)) { >> + VLOG_DBG("The NIC will not provide per-packet USER_MARK on port " >> + DPDK_PORT_ID_FMT, dev->port_id); >> + } >> + if (!(rx_metadata & RTE_ETH_RX_METADATA_TUNNEL_ID)) { >> + VLOG_DBG("The NIC will not provide per-packet TUNNEL_ID on port " >> + DPDK_PORT_ID_FMT, dev->port_id); >> + } >> + } else if (ret == -ENOTSUP) { >> + VLOG_DBG("Rx metadata negotiate procedure is not supported on port " >> + DPDK_PORT_ID_FMT, dev->port_id); >> + } else { >> + VLOG_WARN("Cannot negotiate Rx metadata on port " >> + DPDK_PORT_ID_FMT, dev->port_id); > > For all the logs above, use the following format instead: > > VLOG_*("%s: ...", netdev_get_name(&dev->up)); > > For the last two, you may use: > > VLOG(ret == -ENOTSUP ? VLL_DBG : VLL_WARN, > "%s: Cannot negotiate Rx metadata: %s", > netdev_get_name(&dev->up), rte_strerror(-ret)); > >> + } >> + >> + dev->rx_metadata_delivery_configured = true; >> +} >> + >> static int >> dpdk_eth_dev_init(struct netdev_dpdk *dev) >> OVS_REQUIRES(dev->mutex) >> @@ -1200,6 +1239,16 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) >> RTE_ETH_RX_OFFLOAD_TCP_CKSUM | >> RTE_ETH_RX_OFFLOAD_IPV4_CKSUM; >> >> + /* >> + * Full tunnel offload requires that tunnel ID metadata be >> + * delivered with "miss" packets from the hardware to the >> + * PMD. The same goes for megaflow mark metadata which is >> + * used in MARK + RSS offload scenario. >> + * >> + * Request delivery of such metadata. >> + */ >> + dpdk_eth_dev_init_rx_metadata(dev); > > It's a bit strange that we request metadata regardless of hw-offload > being enabled. Should this be under netdev_is_flow_api_enabled() ? > >> + >> rte_eth_dev_info_get(dev->port_id, &info); >> >> if (strstr(info.driver_name, "vf") != NULL) { >> @@ -1382,6 +1431,8 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, >> /* Initilize the hardware offload flags to 0 */ >> dev->hw_ol_features = 0; >> >> + dev->rx_metadata_delivery_configured = false; >> + >> dev->flags = NETDEV_UP | NETDEV_PROMISC; >> >> ovs_list_push_back(&dpdk_list, &dev->list_node); > >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 63dac689e..d9d1b43f6 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -492,6 +492,9 @@ struct netdev_dpdk { /* Array of vhost rxq states, see vring_state_changed. */ bool *vhost_rxq_enabled; + + /* Ensures that Rx metadata delivery is configured only once. */ + bool rx_metadata_delivery_configured; ); PADDED_MEMBERS(CACHE_LINE_SIZE, @@ -1187,6 +1190,42 @@ dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex) } } +static void +dpdk_eth_dev_init_rx_metadata(struct netdev_dpdk *dev) +{ + uint64_t rx_metadata = 0; + int ret; + + if (dev->rx_metadata_delivery_configured) { + return; + } + + /* For the fallback offload (non-"transfer" rules) */ + rx_metadata |= RTE_ETH_RX_METADATA_USER_MARK; + /* For the full offload ("transfer" rules) */ + rx_metadata |= RTE_ETH_RX_METADATA_TUNNEL_ID; + + ret = rte_eth_rx_metadata_negotiate(dev->port_id, &rx_metadata); + if (ret == 0) { + if (!(rx_metadata & RTE_ETH_RX_METADATA_USER_MARK)) { + VLOG_DBG("The NIC will not provide per-packet USER_MARK on port " + DPDK_PORT_ID_FMT, dev->port_id); + } + if (!(rx_metadata & RTE_ETH_RX_METADATA_TUNNEL_ID)) { + VLOG_DBG("The NIC will not provide per-packet TUNNEL_ID on port " + DPDK_PORT_ID_FMT, dev->port_id); + } + } else if (ret == -ENOTSUP) { + VLOG_DBG("Rx metadata negotiate procedure is not supported on port " + DPDK_PORT_ID_FMT, dev->port_id); + } else { + VLOG_WARN("Cannot negotiate Rx metadata on port " + DPDK_PORT_ID_FMT, dev->port_id); + } + + dev->rx_metadata_delivery_configured = true; +} + static int dpdk_eth_dev_init(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex) @@ -1200,6 +1239,16 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) RTE_ETH_RX_OFFLOAD_TCP_CKSUM | RTE_ETH_RX_OFFLOAD_IPV4_CKSUM; + /* + * Full tunnel offload requires that tunnel ID metadata be + * delivered with "miss" packets from the hardware to the + * PMD. The same goes for megaflow mark metadata which is + * used in MARK + RSS offload scenario. + * + * Request delivery of such metadata. + */ + dpdk_eth_dev_init_rx_metadata(dev); + rte_eth_dev_info_get(dev->port_id, &info); if (strstr(info.driver_name, "vf") != NULL) { @@ -1382,6 +1431,8 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, /* Initilize the hardware offload flags to 0 */ dev->hw_ol_features = 0; + dev->rx_metadata_delivery_configured = false; + dev->flags = NETDEV_UP | NETDEV_PROMISC; ovs_list_push_back(&dpdk_list, &dev->list_node);
This may be required by some PMDs in offload scenarios. Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am> --- lib/netdev-dpdk.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)