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.pe...@netronome.com>
>>>>>> Signed-off-by: Pablo Cascón<pablo.cas...@netronome.com>
>>>>>> Reviewed-by: Simon Horman<simon.hor...@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
>>>>>> d...@openvswitch.org
>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to