[ovs-dev,v2,1/1] netdev-dpdk: fix RX jumbo for NICs not supporting scatter

Message ID 1523630806-26879-2-git-send-email-pablo.cascon@netronome.com
State Changes Requested
Delegated to: Ian Stokes
Headers show
Series
  • netdev-dpdk: fix RX jumbo for NICs not supporting scatter
Related show

Commit Message

Pablo Cascón April 13, 2018, 2:46 p.m.
Currently to RX jumbo packets fails for NICs not supporting scatter.
Scatter is not strictly needed for jumbo support on RX. This change
fixes the issue by only enabling scatter for NICs supporting it. Add a
quirk for "igb" while the PMD is fixed to advertise scatter.

Reported-by: Louis Peens <louis.peens@netronome.com>
Signed-off-by: Pablo Cascón <pablo.cascon@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 lib/netdev-dpdk.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Ian Stokes April 13, 2018, 3:20 p.m. | #1
> Currently to RX jumbo packets fails for NICs not supporting scatter.
> Scatter is not strictly needed for jumbo support on RX. This change fixes
> the issue by only enabling scatter for NICs supporting it. Add a quirk for
> "igb" while the PMD is fixed to advertise scatter.
> 

Thanks for the v2 Pablo.

Adding Eelco and Kevin as they had some comments on the v1.

FYI I'm investigating on the DPDK side to see how/when the flag should be set and used for igb and ixgbe as well as other drivers.

https://dpdk.org/ml/archives/dev/2018-April/097056.html

> Reported-by: Louis Peens <louis.peens@netronome.com>
> Signed-off-by: Pablo Cascón <pablo.cascon@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> ---
>  lib/netdev-dpdk.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ee39cbe..8f6a0a3
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -694,11 +694,19 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev,
> int n_rxq, int n_txq)
>      int diag = 0;
>      int i;
>      struct rte_eth_conf conf = port_conf;
> +    struct rte_eth_dev_info info;
> 
>      /* For some NICs (e.g. Niantic), scatter_rx mode needs to be
> explicitly
>       * enabled. */
>      if (dev->mtu > ETHER_MTU) {
> -        conf.rxmode.enable_scatter = 1;
> +        rte_eth_dev_info_get(dev->port_id, &info);
> +        if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) {
> +            conf.rxmode.enable_scatter = 1;
> +        } else if (!strcmp(info.driver_name, "igb")) {
> +            /* Quirk: as of DPDK 17.11.1 igb's PMD requires explicitly
> +               enabling scatter but fails to advertise it. */

I'm not sure this is acceptable. I'm worried it sets a precedent for code for specific devices and could lead to further instances of this in the future.

It could be argued the scatter approach up to now was specific to Niantic but it also worked for igb and i40e. I40e devices don’t require scatter but can handle it without issue if it is set.

In the past this type of solution has been rejected as the preferred approach was to keep netdev-dpdk code generic as possible.

That’s why I suggest deferring the patch in OVS until the required changes are made in DPDK to satisfy all cases. 17.11.2 is targeted for May 19th. We could have a solution in place for then.

I'm not trying to obstruct this but these cases do arise so interested to hear what others think?

Ian

> +            conf.rxmode.enable_scatter = 1;
> +        }
>      }
> 
>      conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &
> --
> 2.7.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Kevin Traynor April 13, 2018, 6:45 p.m. | #2
On 04/13/2018 04:20 PM, Stokes, Ian wrote:
>> Currently to RX jumbo packets fails for NICs not supporting scatter.
>> Scatter is not strictly needed for jumbo support on RX. This change fixes
>> the issue by only enabling scatter for NICs supporting it. Add a quirk for
>> "igb" while the PMD is fixed to advertise scatter.
>>
> 
> Thanks for the v2 Pablo.
> 
> Adding Eelco and Kevin as they had some comments on the v1.
> 
> FYI I'm investigating on the DPDK side to see how/when the flag should be set and used for igb and ixgbe as well as other drivers.
> 
> https://dpdk.org/ml/archives/dev/2018-April/097056.html
> 
>> Reported-by: Louis Peens <louis.peens@netronome.com>
>> Signed-off-by: Pablo Cascón <pablo.cascon@netronome.com>
>> Reviewed-by: Simon Horman <simon.horman@netronome.com>
>> ---
>>  lib/netdev-dpdk.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ee39cbe..8f6a0a3
>> 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -694,11 +694,19 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev,
>> int n_rxq, int n_txq)
>>      int diag = 0;
>>      int i;
>>      struct rte_eth_conf conf = port_conf;
>> +    struct rte_eth_dev_info info;
>>
>>      /* For some NICs (e.g. Niantic), scatter_rx mode needs to be
>> explicitly
>>       * enabled. */
>>      if (dev->mtu > ETHER_MTU) {
>> -        conf.rxmode.enable_scatter = 1;
>> +        rte_eth_dev_info_get(dev->port_id, &info);
>> +        if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) {
>> +            conf.rxmode.enable_scatter = 1;
>> +        } else if (!strcmp(info.driver_name, "igb")) {
>> +            /* Quirk: as of DPDK 17.11.1 igb's PMD requires explicitly
>> +               enabling scatter but fails to advertise it. */
> 

Can you check name for "nfp" and don't set enable_scatter? I don't think
most of the PMDs used these new offload defines in DPDK17.11.

> I'm not sure this is acceptable. I'm worried it sets a precedent for code for specific devices and could lead to further instances of this in the future.
> 
> It could be argued the scatter approach up to now was specific to Niantic but it also worked for igb and i40e. I40e devices don’t require scatter but can handle it without issue if it is set.
> 
> In the past this type of solution has been rejected as the preferred approach was to keep netdev-dpdk code generic as possible.
> 
> That’s why I suggest deferring the patch in OVS until the required changes are made in DPDK to satisfy all cases. 17.11.2 is targeted for May 19th. We could have a solution in place for then.
> 
> I'm not trying to obstruct this but these cases do arise so interested to hear what others think?
> 
> Ian
> 
>> +            conf.rxmode.enable_scatter = 1;
>> +        }
>>      }
>>
>>      conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Pablo Cascón April 18, 2018, 2:41 p.m. | #3
On 13/04/18 19:45, Kevin Traynor wrote:
> On 04/13/2018 04:20 PM, Stokes, Ian wrote:
>>> Currently to RX jumbo packets fails for NICs not supporting scatter.
>>> Scatter is not strictly needed for jumbo support on RX. This change fixes
>>> the issue by only enabling scatter for NICs supporting it. Add a quirk for
>>> "igb" while the PMD is fixed to advertise scatter.
>>>
>> Thanks for the v2 Pablo.
>>
>> Adding Eelco and Kevin as they had some comments on the v1.
>>
>> FYI I'm investigating on the DPDK side to see how/when the flag should be set and used for igb and ixgbe as well as other drivers.
>>
>> https://dpdk.org/ml/archives/dev/2018-April/097056.html
>>
>>> Reported-by: Louis Peens<louis.peens@netronome.com>
>>> Signed-off-by: Pablo Cascón<pablo.cascon@netronome.com>
>>> Reviewed-by: Simon Horman<simon.horman@netronome.com>
>>> ---
>>>   lib/netdev-dpdk.c | 10 +++++++++-
>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ee39cbe..8f6a0a3
>>> 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -694,11 +694,19 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev,
>>> int n_rxq, int n_txq)
>>>       int diag = 0;
>>>       int i;
>>>       struct rte_eth_conf conf = port_conf;
>>> +    struct rte_eth_dev_info info;
>>>
>>>       /* For some NICs (e.g. Niantic), scatter_rx mode needs to be
>>> explicitly
>>>        * enabled. */
>>>       if (dev->mtu > ETHER_MTU) {
>>> -        conf.rxmode.enable_scatter = 1;
>>> +        rte_eth_dev_info_get(dev->port_id, &info);
>>> +        if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) {
>>> +            conf.rxmode.enable_scatter = 1;
>>> +        } else if (!strcmp(info.driver_name, "igb")) {
>>> +            /* Quirk: as of DPDK 17.11.1 igb's PMD requires explicitly
>>> +               enabling scatter but fails to advertise it. */
> Can you check name for "nfp" and don't set enable_scatter? I don't think
> most of the PMDs used these new offload defines in DPDK17.11.

(sorry for the delay replying, was on PTO)

While it is technically possible to do that for the "nfp" it is not the 
preferred solution IMHO. The "nfp" PMD is doing the right thing (TM) by 
not advertising it supports scatter (it doesn't) and not requiring it 
for jumbo traffic. If we're to add a quirk it should be for the 
to-be-fixed PMD(s).

>> I'm not sure this is acceptable. I'm worried it sets a precedent for code for specific devices and could lead to further instances of this in the future.
>>
>> It could be argued the scatter approach up to now was specific to Niantic but it also worked for igb and i40e. I40e devices don’t require scatter but can handle it without issue if it is set.
>>
>> In the past this type of solution has been rejected as the preferred approach was to keep netdev-dpdk code generic as possible.

The principle makes sense in general to me. I think this should be a 
temporary exception.

>>
>> That’s why I suggest deferring the patch in OVS until the required changes are made in DPDK to satisfy all cases. 17.11.2 is targeted for May 19th. We could have a solution in place for then.

That would be fine when it comes to releases, not so fine for 
(potential) backports in distros and upstream consumers (interested on 
commits on between releases)

>>
>> I'm not trying to obstruct this but these cases do arise so interested to hear what others think?

+1

>>
>> Ian
>>
>>> +            conf.rxmode.enable_scatter = 1;
>>> +        }
>>>       }
>>>
>>>       conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &
>>> --
>>> 2.7.4
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Kevin Traynor April 18, 2018, 5:35 p.m. | #4
On 04/18/2018 03:41 PM, Pablo Cascón wrote:
> On 13/04/18 19:45, Kevin Traynor wrote:
>> On 04/13/2018 04:20 PM, Stokes, Ian wrote:
>>>> Currently to RX jumbo packets fails for NICs not supporting scatter.
>>>> Scatter is not strictly needed for jumbo support on RX. This change
>>>> fixes
>>>> the issue by only enabling scatter for NICs supporting it. Add a
>>>> quirk for
>>>> "igb" while the PMD is fixed to advertise scatter.
>>>>
>>> Thanks for the v2 Pablo.
>>>
>>> Adding Eelco and Kevin as they had some comments on the v1.
>>>
>>> FYI I'm investigating on the DPDK side to see how/when the flag
>>> should be set and used for igb and ixgbe as well as other drivers.
>>>
>>> https://dpdk.org/ml/archives/dev/2018-April/097056.html
>>>
>>>> Reported-by: Louis Peens<louis.peens@netronome.com>
>>>> Signed-off-by: Pablo Cascón<pablo.cascon@netronome.com>
>>>> Reviewed-by: Simon Horman<simon.horman@netronome.com>
>>>> ---
>>>>   lib/netdev-dpdk.c | 10 +++++++++-
>>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>>>> ee39cbe..8f6a0a3
>>>> 100644
>>>> --- a/lib/netdev-dpdk.c
>>>> +++ b/lib/netdev-dpdk.c
>>>> @@ -694,11 +694,19 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev,
>>>> int n_rxq, int n_txq)
>>>>       int diag = 0;
>>>>       int i;
>>>>       struct rte_eth_conf conf = port_conf;
>>>> +    struct rte_eth_dev_info info;
>>>>
>>>>       /* For some NICs (e.g. Niantic), scatter_rx mode needs to be
>>>> explicitly
>>>>        * enabled. */
>>>>       if (dev->mtu > ETHER_MTU) {
>>>> -        conf.rxmode.enable_scatter = 1;
>>>> +        rte_eth_dev_info_get(dev->port_id, &info);
>>>> +        if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) {
>>>> +            conf.rxmode.enable_scatter = 1;
>>>> +        } else if (!strcmp(info.driver_name, "igb")) {
>>>> +            /* Quirk: as of DPDK 17.11.1 igb's PMD requires explicitly
>>>> +               enabling scatter but fails to advertise it. */
>> Can you check name for "nfp" and don't set enable_scatter? I don't think
>> most of the PMDs used these new offload defines in DPDK17.11.
> 
> (sorry for the delay replying, was on PTO)
> 
> While it is technically possible to do that for the "nfp" it is not the
> preferred solution IMHO. The "nfp" PMD is doing the right thing (TM) by
> not advertising it supports scatter (it doesn't) and not requiring it
> for jumbo traffic. If we're to add a quirk it should be for the
> to-be-fixed PMD(s).
> 

I will agree with you...but only in the future :-)

You are considering that the other drivers are to-be-fixed but OVS
supports DPDK 17.11 LTS and it is not required for PMDs to support the
new offloads API (of which this define is associated with). So, Ian can
correct me if I'm wrong, but I don't think that other PMDs need to or
will set this flag in any 17.11.X stable release.

That's why I think it's better to make the exception for "nfp" at
present. It works for nfp, it works for Intel and it works for any other
NICs that currently work.

When OVS is updated to a future DPDK release where all the PMDs support
setting/not setting this flag, then I agree any workaround should be
made for NICs that are not properly reporting their status, or have quirks.

On a different note, and it may be irrelevant depending on the outcome
of the discussion, but this is mixing using defines introduced as part
of the new API and bitfields from the old API. It will work as both are
supported but whenever switching to the new API in OVS, we should
probably do it across the board.

Kevin.

>>> I'm not sure this is acceptable. I'm worried it sets a precedent for
>>> code for specific devices and could lead to further instances of this
>>> in the future.
>>>
>>> It could be argued the scatter approach up to now was specific to
>>> Niantic but it also worked for igb and i40e. I40e devices don’t
>>> require scatter but can handle it without issue if it is set.
>>>
>>> In the past this type of solution has been rejected as the preferred
>>> approach was to keep netdev-dpdk code generic as possible.
> 
> The principle makes sense in general to me. I think this should be a
> temporary exception.
> 
>>>
>>> That’s why I suggest deferring the patch in OVS until the required
>>> changes are made in DPDK to satisfy all cases. 17.11.2 is targeted
>>> for May 19th. We could have a solution in place for then.
> 
> That would be fine when it comes to releases, not so fine for
> (potential) backports in distros and upstream consumers (interested on
> commits on between releases)
> 
>>>
>>> I'm not trying to obstruct this but these cases do arise so
>>> interested to hear what others think?
> 
> +1
> 
>>>
>>> Ian
>>>
>>>> +            conf.rxmode.enable_scatter = 1;
>>>> +        }
>>>>       }
>>>>
>>>>       conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &
>>>> -- 
>>>> 2.7.4
>>>>
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Pablo Cascón April 19, 2018, 2:32 p.m. | #5
On 18/04/18 18:35, Kevin Traynor wrote:
> On 04/18/2018 03:41 PM, Pablo Cascón wrote:
>> On 13/04/18 19:45, Kevin Traynor wrote:
>>> On 04/13/2018 04:20 PM, Stokes, Ian wrote:
>>>>> Currently to RX jumbo packets fails for NICs not supporting scatter.
>>>>> Scatter is not strictly needed for jumbo support on RX. This change
>>>>> fixes
>>>>> the issue by only enabling scatter for NICs supporting it. Add a
>>>>> quirk for
>>>>> "igb" while the PMD is fixed to advertise scatter.
>>>>>
>>>> Thanks for the v2 Pablo.
>>>>
>>>> Adding Eelco and Kevin as they had some comments on the v1.
>>>>
>>>> FYI I'm investigating on the DPDK side to see how/when the flag
>>>> should be set and used for igb and ixgbe as well as other drivers.
>>>>
>>>> https://dpdk.org/ml/archives/dev/2018-April/097056.html
>>>>
>>>>> Reported-by: Louis Peens<louis.peens@netronome.com>
>>>>> Signed-off-by: Pablo Cascón<pablo.cascon@netronome.com>
>>>>> Reviewed-by: Simon Horman<simon.horman@netronome.com>
>>>>> ---
>>>>>    lib/netdev-dpdk.c | 10 +++++++++-
>>>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>>>>> ee39cbe..8f6a0a3
>>>>> 100644
>>>>> --- a/lib/netdev-dpdk.c
>>>>> +++ b/lib/netdev-dpdk.c
>>>>> @@ -694,11 +694,19 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev,
>>>>> int n_rxq, int n_txq)
>>>>>        int diag = 0;
>>>>>        int i;
>>>>>        struct rte_eth_conf conf = port_conf;
>>>>> +    struct rte_eth_dev_info info;
>>>>>
>>>>>        /* For some NICs (e.g. Niantic), scatter_rx mode needs to be
>>>>> explicitly
>>>>>         * enabled. */
>>>>>        if (dev->mtu > ETHER_MTU) {
>>>>> -        conf.rxmode.enable_scatter = 1;
>>>>> +        rte_eth_dev_info_get(dev->port_id, &info);
>>>>> +        if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) {
>>>>> +            conf.rxmode.enable_scatter = 1;
>>>>> +        } else if (!strcmp(info.driver_name, "igb")) {
>>>>> +            /* Quirk: as of DPDK 17.11.1 igb's PMD requires explicitly
>>>>> +               enabling scatter but fails to advertise it. */
>>> Can you check name for "nfp" and don't set enable_scatter? I don't think
>>> most of the PMDs used these new offload defines in DPDK17.11.
>> (sorry for the delay replying, was on PTO)
>>
>> While it is technically possible to do that for the "nfp" it is not the
>> preferred solution IMHO. The "nfp" PMD is doing the right thing (TM) by
>> not advertising it supports scatter (it doesn't) and not requiring it
>> for jumbo traffic. If we're to add a quirk it should be for the
>> to-be-fixed PMD(s).
>>
> I will agree with you...but only in the future :-)
>
> You are considering that the other drivers are to-be-fixed but OVS
> supports DPDK 17.11 LTS and it is not required for PMDs to support the
> new offloads API (of which this define is associated with). So, Ian can
> correct me if I'm wrong, but I don't think that other PMDs need to or
> will set this flag in any 17.11.X stable release.

I see, was assuming that PMDs were required to use the capabilities from 
the time they were added. If the PMDs are not required to then for "DPDK 
17.11.x stable release" the capability bit is not something to reliably 
check. Then it could make sense to rely on other information such as the 
driver name.

(note that the table at 
https://dpdk.org/doc/guides/nics/overview.html#id1 does not match the 
code at least for "igb" on master or tag 17.11, so might not be a 
reliable way to know either)


>
> That's why I think it's better to make the exception for "nfp" at
> present. It works for nfp, it works for Intel and it works for any other
> NICs that currently work.
There's some logic to it, something like "only add code for the device 
driver we're supporting in a better way or fixing as to avoid to 
potentially breaking others". The counter argument is that jumbo does 
not mean scatter and this patch is removing that link.

One way to look at it is that there are 2 different parts of the issue:
1) jumbo RX does not need scatter
2) there's no trivial way, without testing, to tell which NICs a) 
require scatter (and support it) even if it is not advertised and b) 
support jumbo with or without scatter

IMHO we should fix 1 with this patch as current code is wrongly linking 
the jumbo and scatter. And for 2 let NIC maintainers test it while the 
review process (or after) and add a quirk if need be (only for PMDs that 
won't RX jumbo otherwise, regardless of what they advertise). "igb" can 
be covered once tested and others will come if needed.


>
> When OVS is updated to a future DPDK release where all the PMDs support
> setting/not setting this flag, then I agree any workaround should be
> made for NICs that are not properly reporting their status, or have quirks.

Agree with that, once PMDs are required to report their caps for those 
features not working only because of the mis reporting then a per NIC 
workaround will be required.

>
> On a different note, and it may be irrelevant depending on the outcome
> of the discussion, but this is mixing using defines introduced as part
> of the new API and bitfields from the old API. It will work as both are
> supported but whenever switching to the new API in OVS, we should
> probably do it across the board.

Ah good to know, happy to adapt this patch if need be.

>
> Kevin.
>
>>>> I'm not sure this is acceptable. I'm worried it sets a precedent for
>>>> code for specific devices and could lead to further instances of this
>>>> in the future.
>>>>
>>>> It could be argued the scatter approach up to now was specific to
>>>> Niantic but it also worked for igb and i40e. I40e devices don’t
>>>> require scatter but can handle it without issue if it is set.
>>>>
>>>> In the past this type of solution has been rejected as the preferred
>>>> approach was to keep netdev-dpdk code generic as possible.
>> The principle makes sense in general to me. I think this should be a
>> temporary exception.
>>
>>>> That’s why I suggest deferring the patch in OVS until the required
>>>> changes are made in DPDK to satisfy all cases. 17.11.2 is targeted
>>>> for May 19th. We could have a solution in place for then.
>> That would be fine when it comes to releases, not so fine for
>> (potential) backports in distros and upstream consumers (interested on
>> commits on between releases)
>>
>>>> I'm not trying to obstruct this but these cases do arise so
>>>> interested to hear what others think?
>> +1
>>
>>>> Ian
>>>>
>>>>> +            conf.rxmode.enable_scatter = 1;
>>>>> +        }
>>>>>        }
>>>>>
>>>>>        conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &
>>>>> -- 
>>>>> 2.7.4
>>>>>
>>>>> _______________________________________________
>>>>> dev mailing list
>>>>> dev@openvswitch.org
>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Kevin Traynor April 19, 2018, 6:25 p.m. | #6
On 04/19/2018 03:32 PM, Pablo Cascón wrote:
> On 18/04/18 18:35, Kevin Traynor wrote:
>> On 04/18/2018 03:41 PM, Pablo Cascón wrote:
>>> On 13/04/18 19:45, Kevin Traynor wrote:
>>>> On 04/13/2018 04:20 PM, Stokes, Ian wrote:
>>>>>> Currently to RX jumbo packets fails for NICs not supporting scatter.
>>>>>> Scatter is not strictly needed for jumbo support on RX. This change
>>>>>> fixes
>>>>>> the issue by only enabling scatter for NICs supporting it. Add a
>>>>>> quirk for
>>>>>> "igb" while the PMD is fixed to advertise scatter.
>>>>>>
>>>>> Thanks for the v2 Pablo.
>>>>>
>>>>> Adding Eelco and Kevin as they had some comments on the v1.
>>>>>
>>>>> FYI I'm investigating on the DPDK side to see how/when the flag
>>>>> should be set and used for igb and ixgbe as well as other drivers.
>>>>>
>>>>> https://dpdk.org/ml/archives/dev/2018-April/097056.html
>>>>>
>>>>>> Reported-by: Louis Peens<louis.peens@netronome.com>
>>>>>> Signed-off-by: Pablo Cascón<pablo.cascon@netronome.com>
>>>>>> Reviewed-by: Simon Horman<simon.horman@netronome.com>
>>>>>> ---
>>>>>>    lib/netdev-dpdk.c | 10 +++++++++-
>>>>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>>>>>> ee39cbe..8f6a0a3
>>>>>> 100644
>>>>>> --- a/lib/netdev-dpdk.c
>>>>>> +++ b/lib/netdev-dpdk.c
>>>>>> @@ -694,11 +694,19 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk
>>>>>> *dev,
>>>>>> int n_rxq, int n_txq)
>>>>>>        int diag = 0;
>>>>>>        int i;
>>>>>>        struct rte_eth_conf conf = port_conf;
>>>>>> +    struct rte_eth_dev_info info;
>>>>>>
>>>>>>        /* For some NICs (e.g. Niantic), scatter_rx mode needs to be
>>>>>> explicitly
>>>>>>         * enabled. */
>>>>>>        if (dev->mtu > ETHER_MTU) {
>>>>>> -        conf.rxmode.enable_scatter = 1;
>>>>>> +        rte_eth_dev_info_get(dev->port_id, &info);
>>>>>> +        if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) {
>>>>>> +            conf.rxmode.enable_scatter = 1;
>>>>>> +        } else if (!strcmp(info.driver_name, "igb")) {
>>>>>> +            /* Quirk: as of DPDK 17.11.1 igb's PMD requires
>>>>>> explicitly
>>>>>> +               enabling scatter but fails to advertise it. */
>>>> Can you check name for "nfp" and don't set enable_scatter? I don't
>>>> think
>>>> most of the PMDs used these new offload defines in DPDK17.11.
>>> (sorry for the delay replying, was on PTO)
>>>
>>> While it is technically possible to do that for the "nfp" it is not the
>>> preferred solution IMHO. The "nfp" PMD is doing the right thing (TM) by
>>> not advertising it supports scatter (it doesn't) and not requiring it
>>> for jumbo traffic. If we're to add a quirk it should be for the
>>> to-be-fixed PMD(s).
>>>
>> I will agree with you...but only in the future :-)
>>
>> You are considering that the other drivers are to-be-fixed but OVS
>> supports DPDK 17.11 LTS and it is not required for PMDs to support the
>> new offloads API (of which this define is associated with). So, Ian can
>> correct me if I'm wrong, but I don't think that other PMDs need to or
>> will set this flag in any 17.11.X stable release.
> 
> I see, was assuming that PMDs were required to use the capabilities from
> the time they were added. If the PMDs are not required to then for "DPDK
> 17.11.x stable release" the capability bit is not something to reliably
> check. Then it could make sense to rely on other information such as the
> driver name.
> 
> (note that the table at
> https://dpdk.org/doc/guides/nics/overview.html#id1 does not match the
> code at least for "igb" on master or tag 17.11, so might not be a
> reliable way to know either)
> 
> 
>>
>> That's why I think it's better to make the exception for "nfp" at
>> present. It works for nfp, it works for Intel and it works for any other
>> NICs that currently work.
> There's some logic to it, something like "only add code for the device
> driver we're supporting in a better way or fixing as to avoid to
> potentially breaking others". The counter argument is that jumbo does
> not mean scatter and this patch is removing that link.
> 
> One way to look at it is that there are 2 different parts of the issue:
> 1) jumbo RX does not need scatter
> 2) there's no trivial way, without testing, to tell which NICs a)
> require scatter (and support it) even if it is not advertised and b)
> support jumbo with or without scatter
> 
> IMHO we should fix 1 with this patch as current code is wrongly linking
> the jumbo and scatter. And for 2 let NIC maintainers test it while the
> review process (or after) and add a quirk if need be (only for PMDs that
> won't RX jumbo otherwise, regardless of what they advertise). "igb" can
> be covered once tested and others will come if needed.
> 

I'm not totally against that approach. I'm just a little concerned that
the default is changing and other NIC vendors might not notice or test
for a long time, but you are right that it is technically the more
correct way to do things.

> 
>>
>> When OVS is updated to a future DPDK release where all the PMDs support
>> setting/not setting this flag, then I agree any workaround should be
>> made for NICs that are not properly reporting their status, or have
>> quirks.
> 
> Agree with that, once PMDs are required to report their caps for those
> features not working only because of the mis reporting then a per NIC
> workaround will be required.
> 
>>
>> On a different note, and it may be irrelevant depending on the outcome
>> of the discussion, but this is mixing using defines introduced as part
>> of the new API and bitfields from the old API. It will work as both are
>> supported but whenever switching to the new API in OVS, we should
>> probably do it across the board.
> 
> Ah good to know, happy to adapt this patch if need be.
> 

As you're not going to depend on the scatter define to check
capabilities (at least for now), I don't think changing to the new API
should be part of this patchset. It's not just the enable_scatter, you
would need to change the other bitfields in the port config as well.	

>>
>> Kevin.
>>
>>>>> I'm not sure this is acceptable. I'm worried it sets a precedent for
>>>>> code for specific devices and could lead to further instances of this
>>>>> in the future.
>>>>>
>>>>> It could be argued the scatter approach up to now was specific to
>>>>> Niantic but it also worked for igb and i40e. I40e devices don’t
>>>>> require scatter but can handle it without issue if it is set.
>>>>>
>>>>> In the past this type of solution has been rejected as the preferred
>>>>> approach was to keep netdev-dpdk code generic as possible.
>>> The principle makes sense in general to me. I think this should be a
>>> temporary exception.
>>>
>>>>> That’s why I suggest deferring the patch in OVS until the required
>>>>> changes are made in DPDK to satisfy all cases. 17.11.2 is targeted
>>>>> for May 19th. We could have a solution in place for then.
>>> That would be fine when it comes to releases, not so fine for
>>> (potential) backports in distros and upstream consumers (interested on
>>> commits on between releases)
>>>
>>>>> I'm not trying to obstruct this but these cases do arise so
>>>>> interested to hear what others think?
>>> +1
>>>
>>>>> Ian
>>>>>
>>>>>> +            conf.rxmode.enable_scatter = 1;
>>>>>> +        }
>>>>>>        }
>>>>>>
>>>>>>        conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &
>>>>>> -- 
>>>>>> 2.7.4
>>>>>>
>>>>>> _______________________________________________
>>>>>> dev mailing list
>>>>>> dev@openvswitch.org
>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Pablo Cascón April 20, 2018, 10:36 a.m. | #7
On 19/04/18 19:25, Kevin Traynor wrote:
> On 04/19/2018 03:32 PM, Pablo Cascón wrote:
>> On 18/04/18 18:35, Kevin Traynor wrote:
>>> On 04/18/2018 03:41 PM, Pablo Cascón wrote:
>>>> On 13/04/18 19:45, Kevin Traynor wrote:
>>>>> On 04/13/2018 04:20 PM, Stokes, Ian wrote:
>>>>>>> Currently to RX jumbo packets fails for NICs not supporting scatter.
>>>>>>> Scatter is not strictly needed for jumbo support on RX. This change
>>>>>>> fixes
>>>>>>> the issue by only enabling scatter for NICs supporting it. Add a
>>>>>>> quirk for
>>>>>>> "igb" while the PMD is fixed to advertise scatter.
>>>>>>>
>>>>>> Thanks for the v2 Pablo.
>>>>>>
>>>>>> Adding Eelco and Kevin as they had some comments on the v1.
>>>>>>
>>>>>> FYI I'm investigating on the DPDK side to see how/when the flag
>>>>>> should be set and used for igb and ixgbe as well as other drivers.
>>>>>>
>>>>>> https://dpdk.org/ml/archives/dev/2018-April/097056.html
>>>>>>
>>>>>>> Reported-by: Louis Peens<louis.peens@netronome.com>
>>>>>>> Signed-off-by: Pablo Cascón<pablo.cascon@netronome.com>
>>>>>>> Reviewed-by: Simon Horman<simon.horman@netronome.com>
>>>>>>> ---
>>>>>>>     lib/netdev-dpdk.c | 10 +++++++++-
>>>>>>>     1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>>>>>>> ee39cbe..8f6a0a3
>>>>>>> 100644
>>>>>>> --- a/lib/netdev-dpdk.c
>>>>>>> +++ b/lib/netdev-dpdk.c
>>>>>>> @@ -694,11 +694,19 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk
>>>>>>> *dev,
>>>>>>> int n_rxq, int n_txq)
>>>>>>>         int diag = 0;
>>>>>>>         int i;
>>>>>>>         struct rte_eth_conf conf = port_conf;
>>>>>>> +    struct rte_eth_dev_info info;
>>>>>>>
>>>>>>>         /* For some NICs (e.g. Niantic), scatter_rx mode needs to be
>>>>>>> explicitly
>>>>>>>          * enabled. */
>>>>>>>         if (dev->mtu > ETHER_MTU) {
>>>>>>> -        conf.rxmode.enable_scatter = 1;
>>>>>>> +        rte_eth_dev_info_get(dev->port_id, &info);
>>>>>>> +        if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) {
>>>>>>> +            conf.rxmode.enable_scatter = 1;
>>>>>>> +        } else if (!strcmp(info.driver_name, "igb")) {
>>>>>>> +            /* Quirk: as of DPDK 17.11.1 igb's PMD requires
>>>>>>> explicitly
>>>>>>> +               enabling scatter but fails to advertise it. */
>>>>> Can you check name for "nfp" and don't set enable_scatter? I don't
>>>>> think
>>>>> most of the PMDs used these new offload defines in DPDK17.11.
>>>> (sorry for the delay replying, was on PTO)
>>>>
>>>> While it is technically possible to do that for the "nfp" it is not the
>>>> preferred solution IMHO. The "nfp" PMD is doing the right thing (TM) by
>>>> not advertising it supports scatter (it doesn't) and not requiring it
>>>> for jumbo traffic. If we're to add a quirk it should be for the
>>>> to-be-fixed PMD(s).
>>>>
>>> I will agree with you...but only in the future :-)
>>>
>>> You are considering that the other drivers are to-be-fixed but OVS
>>> supports DPDK 17.11 LTS and it is not required for PMDs to support the
>>> new offloads API (of which this define is associated with). So, Ian can
>>> correct me if I'm wrong, but I don't think that other PMDs need to or
>>> will set this flag in any 17.11.X stable release.
>> I see, was assuming that PMDs were required to use the capabilities from
>> the time they were added. If the PMDs are not required to then for "DPDK
>> 17.11.x stable release" the capability bit is not something to reliably
>> check. Then it could make sense to rely on other information such as the
>> driver name.
>>
>> (note that the table at
>> https://dpdk.org/doc/guides/nics/overview.html#id1 does not match the
>> code at least for "igb" on master or tag 17.11, so might not be a
>> reliable way to know either)
>>
>>
>>> That's why I think it's better to make the exception for "nfp" at
>>> present. It works for nfp, it works for Intel and it works for any other
>>> NICs that currently work.
>> There's some logic to it, something like "only add code for the device
>> driver we're supporting in a better way or fixing as to avoid to
>> potentially breaking others". The counter argument is that jumbo does
>> not mean scatter and this patch is removing that link.
>>
>> One way to look at it is that there are 2 different parts of the issue:
>> 1) jumbo RX does not need scatter
>> 2) there's no trivial way, without testing, to tell which NICs a)
>> require scatter (and support it) even if it is not advertised and b)
>> support jumbo with or without scatter
>>
>> IMHO we should fix 1 with this patch as current code is wrongly linking
>> the jumbo and scatter. And for 2 let NIC maintainers test it while the
>> review process (or after) and add a quirk if need be (only for PMDs that
>> won't RX jumbo otherwise, regardless of what they advertise). "igb" can
>> be covered once tested and others will come if needed.
>>
> I'm not totally against that approach. I'm just a little concerned that
> the default is changing and other NIC vendors might not notice or test
> for a long time, but you are right that it is technically the more
> correct way to do things.

Yeah there is a small risk that there is another NIC other than igb that 
also requires scatter for jumbo RX, reckon it is small: would have to be 
a NIC/PMD being fine before the "jumbo equals scatter" was added 
(67fe6d63 Aug 2017) and after.

All right, will post a v3 summarizing the discussion:
- don't check PMD's reported capabilities as are not reliable (PMDs not 
required to use it) along with a comment
- keep the "igb" quirk


>
>>> When OVS is updated to a future DPDK release where all the PMDs support
>>> setting/not setting this flag, then I agree any workaround should be
>>> made for NICs that are not properly reporting their status, or have
>>> quirks.
>> Agree with that, once PMDs are required to report their caps for those
>> features not working only because of the mis reporting then a per NIC
>> workaround will be required.
>>
>>> On a different note, and it may be irrelevant depending on the outcome
>>> of the discussion, but this is mixing using defines introduced as part
>>> of the new API and bitfields from the old API. It will work as both are
>>> supported but whenever switching to the new API in OVS, we should
>>> probably do it across the board.
>> Ah good to know, happy to adapt this patch if need be.
>>
> As you're not going to depend on the scatter define to check
> capabilities (at least for now), I don't think changing to the new API
> should be part of this patchset. It's not just the enable_scatter, you
> would need to change the other bitfields in the port config as well.	

Good point, thx!
>>> Kevin.
>>>
>>>>>> I'm not sure this is acceptable. I'm worried it sets a precedent for
>>>>>> code for specific devices and could lead to further instances of this
>>>>>> in the future.
>>>>>>
>>>>>> It could be argued the scatter approach up to now was specific to
>>>>>> Niantic but it also worked for igb and i40e. I40e devices don’t
>>>>>> require scatter but can handle it without issue if it is set.
>>>>>>
>>>>>> In the past this type of solution has been rejected as the preferred
>>>>>> approach was to keep netdev-dpdk code generic as possible.
>>>> The principle makes sense in general to me. I think this should be a
>>>> temporary exception.
>>>>
>>>>>> That’s why I suggest deferring the patch in OVS until the required
>>>>>> changes are made in DPDK to satisfy all cases. 17.11.2 is targeted
>>>>>> for May 19th. We could have a solution in place for then.
>>>> That would be fine when it comes to releases, not so fine for
>>>> (potential) backports in distros and upstream consumers (interested on
>>>> commits on between releases)
>>>>
>>>>>> I'm not trying to obstruct this but these cases do arise so
>>>>>> interested to hear what others think?
>>>> +1
>>>>
>>>>>> Ian
>>>>>>
>>>>>>> +            conf.rxmode.enable_scatter = 1;
>>>>>>> +        }
>>>>>>>         }
>>>>>>>
>>>>>>>         conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &
>>>>>>> -- 
>>>>>>> 2.7.4
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> dev mailing list
>>>>>>> dev@openvswitch.org
>>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ee39cbe..8f6a0a3 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -694,11 +694,19 @@  dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int n_txq)
     int diag = 0;
     int i;
     struct rte_eth_conf conf = port_conf;
+    struct rte_eth_dev_info info;
 
     /* For some NICs (e.g. Niantic), scatter_rx mode needs to be explicitly
      * enabled. */
     if (dev->mtu > ETHER_MTU) {
-        conf.rxmode.enable_scatter = 1;
+        rte_eth_dev_info_get(dev->port_id, &info);
+        if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) {
+            conf.rxmode.enable_scatter = 1;
+        } else if (!strcmp(info.driver_name, "igb")) {
+            /* Quirk: as of DPDK 17.11.1 igb's PMD requires explicitly
+               enabling scatter but fails to advertise it. */
+            conf.rxmode.enable_scatter = 1;
+        }
     }
 
     conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &