| Message ID | 20230606113533.6866-2-ivan.malov@arknetworks.am |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | DPDK: align flow offloads with 22.11 | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings Ivan Malov, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
ERROR: Inappropriate bracing around statement
#37 FILE: lib/netdev-dpdk.c:1152:
if (dev->rx_metadata_delivery_configured)
Lines checked: 98, Warnings: 0, Errors: 1
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Tue, Jun 06, 2023 at 03:35:32PM +0400, Ivan Malov via dev wrote: > This may be required by some PMDs in offload scenarios. > > Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am> > --- > lib/netdev-dpdk.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 8cb1a7703..98765fe6e 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -542,6 +542,9 @@ struct netdev_dpdk { > int rte_xstats_names_size; > int rte_xstats_ids_size; > uint64_t *rte_xstats_ids; > + > + /* Ensures that Rx metadata delivery is configured only once */ > + bool rx_metadata_delivery_configured; > ); > }; > > @@ -1140,6 +1143,41 @@ 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; Hi Ivan, IIRC, the OVS coding style calls for the use of { } for all conditional blocks. So please update this to: if (dev->rx_metadata_delivery_configured) { return; } Likewise, a similar update is needed for patch 2/2. Thanks!
On 6/29/23 15:58, Simon Horman wrote: > On Tue, Jun 06, 2023 at 03:35:32PM +0400, Ivan Malov via dev wrote: >> This may be required by some PMDs in offload scenarios. >> >> Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am> >> --- >> lib/netdev-dpdk.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 8cb1a7703..98765fe6e 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -542,6 +542,9 @@ struct netdev_dpdk { >> int rte_xstats_names_size; >> int rte_xstats_ids_size; >> uint64_t *rte_xstats_ids; >> + >> + /* Ensures that Rx metadata delivery is configured only once */ This also needs a period at the end of a comment. >> + bool rx_metadata_delivery_configured; And this doesn't really belong in the 'stats' section of the structure. Might be better fit in one of the previous sections. The one with 'up', for example. >> ); >> }; >> >> @@ -1140,6 +1143,41 @@ 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; > > Hi Ivan, > > IIRC, the OVS coding style calls for the use of { } for all conditional blocks. > So please update this to: > > if (dev->rx_metadata_delivery_configured) { > return; > } > > Likewise, a similar update is needed for patch 2/2. > > Thanks!
On Thu, 29 Jun 2023, Simon Horman wrote: > On Tue, Jun 06, 2023 at 03:35:32PM +0400, Ivan Malov via dev wrote: >> This may be required by some PMDs in offload scenarios. >> >> Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am> >> --- >> lib/netdev-dpdk.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 8cb1a7703..98765fe6e 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -542,6 +542,9 @@ struct netdev_dpdk { >> int rte_xstats_names_size; >> int rte_xstats_ids_size; >> uint64_t *rte_xstats_ids; >> + >> + /* Ensures that Rx metadata delivery is configured only once */ >> + bool rx_metadata_delivery_configured; >> ); >> }; >> >> @@ -1140,6 +1143,41 @@ 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; > > Hi Ivan, > > IIRC, the OVS coding style calls for the use of { } for all conditional blocks. > So please update this to: > > if (dev->rx_metadata_delivery_configured) { > return; > } > > Likewise, a similar update is needed for patch 2/2. > > Thanks! > Hi Simon, Very valuable information indeed. Now I understand the true reason behind the check-bot warning. Thank you.
Hi Ilya, Thanks again for reviewing this. I attempted to fix review notes in https://patchwork.ozlabs.org/project/openvswitch/list/?series=361784 . Thank you. On Thu, 29 Jun 2023, Ilya Maximets wrote: > On 6/29/23 15:58, Simon Horman wrote: >> On Tue, Jun 06, 2023 at 03:35:32PM +0400, Ivan Malov via dev wrote: >>> This may be required by some PMDs in offload scenarios. >>> >>> Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am> >>> --- >>> lib/netdev-dpdk.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 50 insertions(+) >>> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index 8cb1a7703..98765fe6e 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -542,6 +542,9 @@ struct netdev_dpdk { >>> int rte_xstats_names_size; >>> int rte_xstats_ids_size; >>> uint64_t *rte_xstats_ids; >>> + >>> + /* Ensures that Rx metadata delivery is configured only once */ > > This also needs a period at the end of a comment. > >>> + bool rx_metadata_delivery_configured; > > And this doesn't really belong in the 'stats' section of the structure. > Might be better fit in one of the previous sections. The one with 'up', > for example. > >>> ); >>> }; >>> >>> @@ -1140,6 +1143,41 @@ 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; >> >> Hi Ivan, >> >> IIRC, the OVS coding style calls for the use of { } for all conditional blocks. >> So please update this to: >> >> if (dev->rx_metadata_delivery_configured) { >> return; >> } >> >> Likewise, a similar update is needed for patch 2/2. >> >> Thanks! > >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 8cb1a7703..98765fe6e 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -542,6 +542,9 @@ struct netdev_dpdk { int rte_xstats_names_size; int rte_xstats_ids_size; uint64_t *rte_xstats_ids; + + /* Ensures that Rx metadata delivery is configured only once */ + bool rx_metadata_delivery_configured; ); }; @@ -1140,6 +1143,41 @@ 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) @@ -1154,6 +1192,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) { @@ -1320,6 +1368,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 | 50 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)