[ovs-dev] [PATCH v2 1/1] netdev-dpdk: fix RX jumbo for NICs not supporting scatter
Pablo Cascón
pablo.cascon at netronome.com
Fri Apr 20 10:36:30 UTC 2018
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 at netronome.com>
>>>>>>> Signed-off-by: Pablo Cascón<pablo.cascon at netronome.com>
>>>>>>> Reviewed-by: Simon Horman<simon.horman at 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 at openvswitch.org
>>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list