[ovs-dev,dpdk-howl,v5,2/2] netdev-dpdk: Set scatter based on capabilities

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
Related show

Commit Message

Ophir Munk Oct. 10, 2018, 4:14 p.m.
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(-)

Comments

Eelco Chaudron Oct. 12, 2018, 9:24 a.m. | #1
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
Ophir Munk Oct. 13, 2018, 9:12 p.m. | #2
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&amp;data=02%7C01%7Cop
> >
> hirmu%40mellanox.com%7C7b49470cf2934809bbb708d6302488c4%7Ca652
> 971c7d2e
> >
> 4d9ba6a4d149256f461b%7C0%7C0%7C636749330816444832&amp;sdata=i
> OkI1RtzoA
> > yZJgi0eMWTW1AJZ73fm2EuhGKkWQCvFLI%3D&amp;reserved=0
Eelco Chaudron Oct. 24, 2018, 10:41 a.m. | #3
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>

Patch

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);