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<[email protected]> >>>>>> Signed-off-by: Pablo Cascón<[email protected]> >>>>>> Reviewed-by: Simon Horman<[email protected]> >>>>>> --- >>>>>> 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 >>>>>> [email protected] >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
