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

2018-04-13 Thread Pablo Cascón 

On 12/04/18 14:05, Stokes, Ian wrote:

On 10/04/18 21:08, 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.

Reported-by: Louis Peens 
Signed-off-by: Pablo Cascón 
Reviewed-by: Simon Horman 
---
   lib/netdev-dpdk.c | 5 -
   1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
ee39cbe..28b20b5
100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -694,11 +694,14 @@ 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, );
+if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER)
+conf.rxmode.enable_scatter = 1;

Thanks for this, quick note:

conf.rxmode.enable_scatter = 1; should be enclosed in braces as per OVS

coding style.

With an ixgbe device (a Niantic in this case),  it never hits the offload 
condition above so that it can set scatter.
Currently DEV_RX_OFFLOAD_SCATTER is not set for ixgbe or igb devices as part of 
info.rx_offload_capa.

Surprisingly MTU still worked for the ixgbe device.

Digging a little deeper it seems the need to set scatter explicitly for ixgbe 
was modified by commit

net/ixgbe: remove MTU setting limitation (c009c6b1)

"This patch allows setting this special MTU when device is stopped, because 
scattered_rx will be re-configured during next port start and driver may enable scattered 
receive according new MTU value."

In the ixgbe case it's ok because the device is stopped at the time we call set 
mtu.

However the patch breaks existing functionality for igb devices as they do not 
have a flag set for DEV_RX_OFFLOAD_SCATTER either and still explicitly require 
scatter to be set regardless of being stopped. Otherwise they fail to 
reconfigure and remain in a down state.

I think a patch is needed for DPDK to set the DEV_RX_OFFLOAD_SCATTER flag for 
ixgbe and igb devices. I can look into that.


Thanks for the testing and investigation. Sorry for unearthing this bug :)

If setting the DEV_RX_OFFLOAD_SCATTER flag in the igb and ixgbe PMD 
fixes the issue (when this patch is applied to OVS) please warm other 
PMDs in the DPDK's mailing list




In an effort to avoid breaking existing functionality in OVS I suggest we defer 
this patch until that support is in place for DPDK 17.11.2.


Unfortunately deferring this patch would hurt Netronome's use case, this 
jumbo/scatter bug needs to be fixed. Will post a v2 including your style 
feedback and a check for driver_name being "igb" to set scatter 
regardless of the capability. That will make OVS-DPDK to work with "igb" 
before and after your fix. We can comment in that v2 if other extra 
checks are needed




We could combine the current patch with the incremental below to fix the style 
issue and comment.

Thoughts?

-/* For some NICs (e.g. Niantic), scatter_rx mode needs to be explicitly
- * enabled. */
+/* For some NICs scatter_rx mode needs to be explicitly enabled. */
  if (dev->mtu > ETHER_MTU) {
  rte_eth_dev_info_get(dev->port_id, );
-if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER)
+if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) {
  conf.rxmode.enable_scatter = 1;
+}
  }

Thanks
Ian


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

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

2018-04-12 Thread Stokes, Ian 
> On 10/04/18 21:08, 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.
> >>
> >> Reported-by: Louis Peens 
> >> Signed-off-by: Pablo Cascón 
> >> Reviewed-by: Simon Horman 
> >> ---
> >>   lib/netdev-dpdk.c | 5 -
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >> ee39cbe..28b20b5
> >> 100644
> >> --- a/lib/netdev-dpdk.c
> >> +++ b/lib/netdev-dpdk.c
> >> @@ -694,11 +694,14 @@ 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, );
> >> +if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER)
> >> +conf.rxmode.enable_scatter = 1;
> > Thanks for this, quick note:
> >
> > conf.rxmode.enable_scatter = 1; should be enclosed in braces as per OVS
> coding style.

With an ixgbe device (a Niantic in this case),  it never hits the offload 
condition above so that it can set scatter.
Currently DEV_RX_OFFLOAD_SCATTER is not set for ixgbe or igb devices as part of 
info.rx_offload_capa.

Surprisingly MTU still worked for the ixgbe device.

Digging a little deeper it seems the need to set scatter explicitly for ixgbe 
was modified by commit 

net/ixgbe: remove MTU setting limitation (c009c6b1)

"This patch allows setting this special MTU when device is stopped, because 
scattered_rx will be re-configured during next port start and driver may enable 
scattered receive according new MTU value."

In the ixgbe case it's ok because the device is stopped at the time we call set 
mtu.

However the patch breaks existing functionality for igb devices as they do not 
have a flag set for DEV_RX_OFFLOAD_SCATTER either and still explicitly require 
scatter to be set regardless of being stopped. Otherwise they fail to 
reconfigure and remain in a down state.

I think a patch is needed for DPDK to set the DEV_RX_OFFLOAD_SCATTER flag for 
ixgbe and igb devices. I can look into that.

In an effort to avoid breaking existing functionality in OVS I suggest we defer 
this patch until that support is in place for DPDK 17.11.2.

We could combine the current patch with the incremental below to fix the style 
issue and comment.

Thoughts?

-/* For some NICs (e.g. Niantic), scatter_rx mode needs to be explicitly
- * enabled. */
+/* For some NICs scatter_rx mode needs to be explicitly enabled. */
 if (dev->mtu > ETHER_MTU) {
 rte_eth_dev_info_get(dev->port_id, );
-if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER)
+if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER) {
 conf.rxmode.enable_scatter = 1;
+}
 }

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

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

2018-04-11 Thread Stokes, Ian 
> > On 10/04/18 21:08, 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.
> >>>
> >>> Reported-by: Louis Peens 
> >>> Signed-off-by: Pablo Cascón 
> >>> Reviewed-by: Simon Horman 
> >>> ---
> >>>   lib/netdev-dpdk.c | 5 -
> >>>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >>> ee39cbe..28b20b5
> >>> 100644
> >>> --- a/lib/netdev-dpdk.c
> >>> +++ b/lib/netdev-dpdk.c
> >>> @@ -694,11 +694,14 @@ 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, );
> >>> +if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER)
> 
> Hi, this was not added until DPDK 17.11, so it's not relevant for OVS
> 2.8 branch.
> 
> Kevin.

Good catch, cheers for the heads up.

Ian
> 
> >>> +conf.rxmode.enable_scatter = 1;
> >> Thanks for this, quick note:
> >>
> >> conf.rxmode.enable_scatter = 1; should be enclosed in braces as per
> >> OVS coding style.
> >
> > Thanks for the feedback, sorry about the lack of braces. Let me know
> > if a v2 is needed
> >>
> >> I'll have some time to test this tomorrow, I take it this should be
> >> backported to OVS2.9 and OVS 2.8 also?
> >
> > Yes please, the change is a welcome one for both 2.8 and 2.9. Also it
> > applies nicely on both. Let me know if a per branch patch is needed.
> >
> > Thanks
> >
> > Pablo
> >
> >>
> >> Ian
> >>
> >>>   }
> >>>
> >>>   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

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

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

2018-04-11 Thread Stokes, Ian 
> >> 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.
> >>
> >> Reported-by: Louis Peens 
> >> Signed-off-by: Pablo Cascón 
> >> Reviewed-by: Simon Horman 
> >> ---
> >>   lib/netdev-dpdk.c | 5 -
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >> ee39cbe..28b20b5
> >> 100644
> >> --- a/lib/netdev-dpdk.c
> >> +++ b/lib/netdev-dpdk.c
> >> @@ -694,11 +694,14 @@ 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, );
> >> +if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER)
> >> +conf.rxmode.enable_scatter = 1;
> > Thanks for this, quick note:
> >
> > conf.rxmode.enable_scatter = 1; should be enclosed in braces as per OVS
> coding style.
> 
> Thanks for the feedback, sorry about the lack of braces. Let me know if a
> v2 is needed

No need, I can add an incremental on commit.

Thanks
Ian
> >
> > I'll have some time to test this tomorrow, I take it this should be
> backported to OVS2.9 and OVS 2.8 also?
> 
> Yes please, the change is a welcome one for both 2.8 and 2.9. Also it
> applies nicely on both. Let me know if a per branch patch is needed.
> 
> Thanks
> 
> Pablo
> 
> >
> > Ian
> >
> >>   }
> >>
> >>   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

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

2018-04-11 Thread Kevin Traynor 
On 04/11/2018 12:10 PM, Pablo Cascón wrote:
> On 10/04/18 21:08, 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.
>>>
>>> Reported-by: Louis Peens 
>>> Signed-off-by: Pablo Cascón 
>>> Reviewed-by: Simon Horman 
>>> ---
>>>   lib/netdev-dpdk.c | 5 -
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>>> ee39cbe..28b20b5
>>> 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -694,11 +694,14 @@ 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, );
>>> +if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER)

Hi, this was not added until DPDK 17.11, so it's not relevant for OVS
2.8 branch.

Kevin.

>>> +conf.rxmode.enable_scatter = 1;
>> Thanks for this, quick note:
>>
>> conf.rxmode.enable_scatter = 1; should be enclosed in braces as per
>> OVS coding style.
> 
> Thanks for the feedback, sorry about the lack of braces. Let me know if
> a v2 is needed
>>
>> I'll have some time to test this tomorrow, I take it this should be
>> backported to OVS2.9 and OVS 2.8 also?
> 
> Yes please, the change is a welcome one for both 2.8 and 2.9. Also it
> applies nicely on both. Let me know if a per branch patch is needed.
> 
> Thanks
> 
> Pablo
> 
>>
>> Ian
>>
>>>   }
>>>
>>>   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

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

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

2018-04-11 Thread Pablo Cascón 

On 10/04/18 21:08, 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.

Reported-by: Louis Peens 
Signed-off-by: Pablo Cascón 
Reviewed-by: Simon Horman 
---
  lib/netdev-dpdk.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ee39cbe..28b20b5
100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -694,11 +694,14 @@ 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, );
+if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER)
+conf.rxmode.enable_scatter = 1;

Thanks for this, quick note:

conf.rxmode.enable_scatter = 1; should be enclosed in braces as per OVS coding 
style.


Thanks for the feedback, sorry about the lack of braces. Let me know if 
a v2 is needed


I'll have some time to test this tomorrow, I take it this should be backported 
to OVS2.9 and OVS 2.8 also?


Yes please, the change is a welcome one for both 2.8 and 2.9. Also it 
applies nicely on both. Let me know if a per branch patch is needed.


Thanks

Pablo



Ian


  }

  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

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

2018-04-10 Thread Stokes, Ian 
> 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.
> 
> Reported-by: Louis Peens 
> Signed-off-by: Pablo Cascón 
> Reviewed-by: Simon Horman 
> ---
>  lib/netdev-dpdk.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ee39cbe..28b20b5
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -694,11 +694,14 @@ 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, );
> +if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER)
> +conf.rxmode.enable_scatter = 1;

Thanks for this, quick note:

conf.rxmode.enable_scatter = 1; should be enclosed in braces as per OVS coding 
style.

I'll have some time to test this tomorrow, I take it this should be backported 
to OVS2.9 and OVS 2.8 also?

Ian

>  }
> 
>  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

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

2018-04-10 Thread Eelco Chaudron 

On 10/04/18 11:36, Pablo Cascón 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.

Reported-by: Louis Peens 
Signed-off-by: Pablo Cascón 
Reviewed-by: Simon Horman 
---
  lib/netdev-dpdk.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ee39cbe..28b20b5 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -694,11 +694,14 @@ 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, );
+if (info.rx_offload_capa & DEV_RX_OFFLOAD_SCATTER)
+conf.rxmode.enable_scatter = 1;
  }
  
  conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &


Change looks good to me.


Acked-by: Eelco Chaudron 


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