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. 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 >>>> [email protected] >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
