| Message ID | 1539188042-20673-2-git-send-email-ophirmu@mellanox.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,dpdk-howl,v5,1/2] netdev-dpdk: Upgrade to dpdk v18.08 | expand |
On 10 Oct 2018, at 18:14, Ophir Munk wrote: > Before this commit setting scatter offload was based on checking > net_nfp device. > Since DPDK 17.11 more PMD drivers are reporting offload > capabilities. Therefore this commit removes the specific check > against net_nfp device and replaces it with a generic check of > device capabilities before setting the scatter offload. > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > --- > > v1-v4 > This patch was not included in version v1-v4 of the series > > v5 > Initial version > > lib/netdev-dpdk.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 4dd0ec3..ecca276 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -367,6 +367,7 @@ struct ingress_policer { > enum dpdk_hw_ol_features { > NETDEV_RX_CHECKSUM_OFFLOAD = 1 << 0, > NETDEV_RX_HW_CRC_STRIP = 1 << 1, > + NETDEV_RX_HW_SCATTER = 1 << 2 > }; > > /* > @@ -894,13 +895,11 @@ dpdk_eth_dev_port_config(struct netdev_dpdk > *dev, int n_rxq, int n_txq) > rte_eth_dev_info_get(dev->port_id, &info); > > /* As of DPDK 17.11.1 a few PMDs require to explicitly enable > - * scatter to support jumbo RX. Checking the offload capabilities > - * is not an option as PMDs are not required yet to report > - * them. The only reliable info is the driver name and knowledge > - * (testing or code review). Listing all such PMDs feels harder > - * than highlighting the one known not to need scatter */ > + * scatter to support jumbo RX. > + * Setting scatter for the device is done after checking for > + * scatter support in the device capabilites. */ > if (dev->mtu > ETHER_MTU) { > - if (strncmp(info.driver_name, "net_nfp", 7)) { > + if (dev->hw_ol_features & NETDEV_RX_HW_SCATTER) { > conf.rxmode.offloads |= DEV_RX_OFFLOAD_SCATTER; > } > } > @@ -1035,6 +1034,13 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) > dev->hw_ol_features |= NETDEV_RX_CHECKSUM_OFFLOAD; > } > > + if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) { > + dev->hw_ol_features |= NETDEV_RX_HW_SCATTER; > + } else { > + /* Do not warn on lack of scatter support */ > + dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; Don’t think we need to clear it explicitly, none of the other capabilities do this. For the rest this patch look good to me. > + } > + > n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq); > n_txq = MIN(info.max_tx_queues, dev->up.n_txq); > > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On , October 12, 2018 12:25 PM Eelco Chaudron wrote: > > On 10 Oct 2018, at 18:14, Ophir Munk wrote: > > > Before this commit setting scatter offload was based on checking > > net_nfp device. > > Since DPDK 17.11 more PMD drivers are reporting offload capabilities. > > Therefore this commit removes the specific check against net_nfp > > device and replaces it with a generic check of device capabilities > > before setting the scatter offload. > > > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > > --- > > > > v1-v4 > > This patch was not included in version v1-v4 of the series > > > > v5 > > Initial version > > > > lib/netdev-dpdk.c | 18 ++++++++++++------ > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > > 4dd0ec3..ecca276 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -367,6 +367,7 @@ struct ingress_policer { enum > dpdk_hw_ol_features > > { > > NETDEV_RX_CHECKSUM_OFFLOAD = 1 << 0, > > NETDEV_RX_HW_CRC_STRIP = 1 << 1, > > + NETDEV_RX_HW_SCATTER = 1 << 2 > > }; > > > > /* > > @@ -894,13 +895,11 @@ dpdk_eth_dev_port_config(struct netdev_dpdk > > *dev, int n_rxq, int n_txq) > > rte_eth_dev_info_get(dev->port_id, &info); > > > > /* As of DPDK 17.11.1 a few PMDs require to explicitly enable > > - * scatter to support jumbo RX. Checking the offload capabilities > > - * is not an option as PMDs are not required yet to report > > - * them. The only reliable info is the driver name and knowledge > > - * (testing or code review). Listing all such PMDs feels harder > > - * than highlighting the one known not to need scatter */ > > + * scatter to support jumbo RX. > > + * Setting scatter for the device is done after checking for > > + * scatter support in the device capabilites. */ > > if (dev->mtu > ETHER_MTU) { > > - if (strncmp(info.driver_name, "net_nfp", 7)) { > > + if (dev->hw_ol_features & NETDEV_RX_HW_SCATTER) { > > conf.rxmode.offloads |= DEV_RX_OFFLOAD_SCATTER; > > } > > } > > @@ -1035,6 +1034,13 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) > > dev->hw_ol_features |= NETDEV_RX_CHECKSUM_OFFLOAD; > > } > > > > + if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) { > > + dev->hw_ol_features |= NETDEV_RX_HW_SCATTER; > > + } else { > > + /* Do not warn on lack of scatter support */ > > + dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; > > Don’t think we need to clear it explicitly, none of the other capabilities do > this. The other capabilities do clear explicitly. There is a difference between NETDEV_RX_HW_SCATTER flag (which should be set or cleared in dpdk_eth_dev_init()) and DEV_RX_OFFLOAD_SCATTER flag (which is only set in dpdk_eth_dev_port_config()). Flags of type "NETDEV_RX_XXXXXX" are cleared explicitly. For example, we clear: dev->hw_ol_features &= ~NETDEV_RX_HW_CRC_STRIP; dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD; in the following code snippet: if (strstr(info.driver_name, "vf") != NULL) { VLOG_INFO("Virtual function detected, HW_CRC_STRIP will be enabled"); dev->hw_ol_features |= NETDEV_RX_HW_CRC_STRIP; } else { dev->hw_ol_features &= ~NETDEV_RX_HW_CRC_STRIP; } if ((info.rx_offload_capa & rx_chksm_offload_capa) != rx_chksm_offload_capa) { VLOG_WARN("Rx checksum offload is not supported on port " DPDK_PORT_ID_FMT, dev->port_id); dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD; } else { dev->hw_ol_features |= NETDEV_RX_CHECKSUM_OFFLOAD; } if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) { dev->hw_ol_features |= NETDEV_RX_HW_SCATTER; } else { /* Do not warn on lack of scatter support */ dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; } During configuration we only set DEV_RX_OFFLOAD_SCATTER flag: if (dev->mtu > ETHER_MTU) { if (dev->hw_ol_features & NETDEV_RX_HW_SCATTER) { conf.rxmode.offloads |= DEV_RX_OFFLOAD_SCATTER; I suggest keeping the patch as is. > For the rest this patch look good to me. > > > + } > > + > > n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq); > > n_txq = MIN(info.max_tx_queues, dev->up.n_txq); > > > > -- > > 1.8.3.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmai > > l.openvswitch.org%2Fmailman%2Flistinfo%2Fovs- > dev&data=02%7C01%7Cop > > > hirmu%40mellanox.com%7C7b49470cf2934809bbb708d6302488c4%7Ca652 > 971c7d2e > > > 4d9ba6a4d149256f461b%7C0%7C0%7C636749330816444832&sdata=i > OkI1RtzoA > > yZJgi0eMWTW1AJZ73fm2EuhGKkWQCvFLI%3D&reserved=0
On 13 Oct 2018, at 23:12, Ophir Munk wrote: > On , October 12, 2018 12:25 PM Eelco Chaudron wrote: > >> >> On 10 Oct 2018, at 18:14, Ophir Munk wrote: >> >>> Before this commit setting scatter offload was based on checking >>> net_nfp device. >>> Since DPDK 17.11 more PMD drivers are reporting offload >>> capabilities. >>> Therefore this commit removes the specific check against net_nfp >>> device and replaces it with a generic check of device capabilities >>> before setting the scatter offload. >>> >>> Signed-off-by: Ophir Munk <ophirmu@mellanox.com> >>> --- >>> >>> v1-v4 >>> This patch was not included in version v1-v4 of the series >>> >>> v5 >>> Initial version >>> >>> lib/netdev-dpdk.c | 18 ++++++++++++------ >>> 1 file changed, 12 insertions(+), 6 deletions(-) >>> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >>> 4dd0ec3..ecca276 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -367,6 +367,7 @@ struct ingress_policer { enum >> dpdk_hw_ol_features >>> { >>> NETDEV_RX_CHECKSUM_OFFLOAD = 1 << 0, >>> NETDEV_RX_HW_CRC_STRIP = 1 << 1, >>> + NETDEV_RX_HW_SCATTER = 1 << 2 >>> }; >>> >>> /* >>> @@ -894,13 +895,11 @@ dpdk_eth_dev_port_config(struct netdev_dpdk >>> *dev, int n_rxq, int n_txq) >>> rte_eth_dev_info_get(dev->port_id, &info); >>> >>> /* As of DPDK 17.11.1 a few PMDs require to explicitly enable >>> - * scatter to support jumbo RX. Checking the offload >>> capabilities >>> - * is not an option as PMDs are not required yet to report >>> - * them. The only reliable info is the driver name and >>> knowledge >>> - * (testing or code review). Listing all such PMDs feels harder >>> - * than highlighting the one known not to need scatter */ >>> + * scatter to support jumbo RX. >>> + * Setting scatter for the device is done after checking for >>> + * scatter support in the device capabilites. */ >>> if (dev->mtu > ETHER_MTU) { >>> - if (strncmp(info.driver_name, "net_nfp", 7)) { >>> + if (dev->hw_ol_features & NETDEV_RX_HW_SCATTER) { >>> conf.rxmode.offloads |= DEV_RX_OFFLOAD_SCATTER; >>> } >>> } >>> @@ -1035,6 +1034,13 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) >>> dev->hw_ol_features |= NETDEV_RX_CHECKSUM_OFFLOAD; >>> } >>> >>> + if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) { >>> + dev->hw_ol_features |= NETDEV_RX_HW_SCATTER; >>> + } else { >>> + /* Do not warn on lack of scatter support */ >>> + dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; >> >> Don’t think we need to clear it explicitly, none of the other >> capabilities do >> this. > > The other capabilities do clear explicitly. > There is a difference between NETDEV_RX_HW_SCATTER flag (which should > be set or cleared in dpdk_eth_dev_init()) and DEV_RX_OFFLOAD_SCATTER > flag (which is only set in dpdk_eth_dev_port_config()). > > Flags of type "NETDEV_RX_XXXXXX" are cleared explicitly. For example, > we clear: > dev->hw_ol_features &= ~NETDEV_RX_HW_CRC_STRIP; > dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD; > > in the following code snippet: > if (strstr(info.driver_name, "vf") != NULL) { > VLOG_INFO("Virtual function detected, HW_CRC_STRIP will be > enabled"); > dev->hw_ol_features |= NETDEV_RX_HW_CRC_STRIP; > } else { > dev->hw_ol_features &= ~NETDEV_RX_HW_CRC_STRIP; > } > > if ((info.rx_offload_capa & rx_chksm_offload_capa) != > rx_chksm_offload_capa) { > VLOG_WARN("Rx checksum offload is not supported on port " > DPDK_PORT_ID_FMT, dev->port_id); > dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD; > } else { > dev->hw_ol_features |= NETDEV_RX_CHECKSUM_OFFLOAD; > } > > if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) { > dev->hw_ol_features |= NETDEV_RX_HW_SCATTER; > } else { > /* Do not warn on lack of scatter support */ > dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; > } > > During configuration we only set DEV_RX_OFFLOAD_SCATTER flag: > if (dev->mtu > ETHER_MTU) { > if (dev->hw_ol_features & NETDEV_RX_HW_SCATTER) { > conf.rxmode.offloads |= DEV_RX_OFFLOAD_SCATTER; > > I suggest keeping the patch as is. I agree, and the rest of this patch is fine. Acked-by: Eelco Chaudron <echaudro@redhat.com>
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 4dd0ec3..ecca276 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -367,6 +367,7 @@ struct ingress_policer { enum dpdk_hw_ol_features { NETDEV_RX_CHECKSUM_OFFLOAD = 1 << 0, NETDEV_RX_HW_CRC_STRIP = 1 << 1, + NETDEV_RX_HW_SCATTER = 1 << 2 }; /* @@ -894,13 +895,11 @@ dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int n_txq) rte_eth_dev_info_get(dev->port_id, &info); /* As of DPDK 17.11.1 a few PMDs require to explicitly enable - * scatter to support jumbo RX. Checking the offload capabilities - * is not an option as PMDs are not required yet to report - * them. The only reliable info is the driver name and knowledge - * (testing or code review). Listing all such PMDs feels harder - * than highlighting the one known not to need scatter */ + * scatter to support jumbo RX. + * Setting scatter for the device is done after checking for + * scatter support in the device capabilites. */ if (dev->mtu > ETHER_MTU) { - if (strncmp(info.driver_name, "net_nfp", 7)) { + if (dev->hw_ol_features & NETDEV_RX_HW_SCATTER) { conf.rxmode.offloads |= DEV_RX_OFFLOAD_SCATTER; } } @@ -1035,6 +1034,13 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) dev->hw_ol_features |= NETDEV_RX_CHECKSUM_OFFLOAD; } + if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) { + dev->hw_ol_features |= NETDEV_RX_HW_SCATTER; + } else { + /* Do not warn on lack of scatter support */ + dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER; + } + n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq); n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
Before this commit setting scatter offload was based on checking net_nfp device. Since DPDK 17.11 more PMD drivers are reporting offload capabilities. Therefore this commit removes the specific check against net_nfp device and replaces it with a generic check of device capabilities before setting the scatter offload. Signed-off-by: Ophir Munk <ophirmu@mellanox.com> --- v1-v4 This patch was not included in version v1-v4 of the series v5 Initial version lib/netdev-dpdk.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)