Re: [ovs-dev] [PATCH] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Lucas Alvares Gomes]

Hi Yi-Hung,

> I format the patch and post in here:
> https://patchwork.ozlabs.org/patch/927995/ Could you help to take a
> look?

Oh great! Sorry for the delay on this, I will take a look!

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

Re: [ovs-dev] [PATCH] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Yi-Hung Wei]

On Fri, May 18, 2018 at 1:41 AM, Lucas Alvares Gomes
 wrote:
>> But as mentioned in ovs commit 6c0bf091 ("datapath: use core MTU range
>> checking in core net infra"), it might be the case that my commit [0]
>> does not set max_mtu correctly.  How about the fix in the following?
>> From what I tested, without the fix, min_mtu: 64, max_mtu: 1500, with
>> that fix, min_mtu:64 and max_mtu: 65535.
>>
>>> As pointed out by commit [0], the ndo_change_mtu function pointer has been
>>> moved from 'struct net_device_ops' to 'struct net_device_ops_extended'
>>> on RHEL 7.5.
>>>
>>> So this patch fixes the backport issue by setting the
>>> .extended.ndo_change_mtu when necessary.
>>>
>>> [0] 39ca338374abe367e28a2247bac9159695f19710
>>
>> --- a/datapath/vport-internal_dev.c
>> +++ b/datapath/vport-internal_dev.c
>> @@ -169,6 +169,8 @@ static void do_setup(struct net_device *netdev)
>>
>>  #ifdef HAVE_NET_DEVICE_WITH_MAX_MTU
>> netdev->max_mtu = ETH_MAX_MTU;
>> +#elif HAVE_RHEL7_MAX_MTU
>> +   netdev->extended->max_mtu = ETH_MAX_MTU;
>>  #endif
>> netdev->netdev_ops = _dev_netdev_ops;
>
> Cool! I will give this a go and see if it works.
>
> Cheers,
> Lucas

Hi Lucas,

I format the patch and post in here:
https://patchwork.ozlabs.org/patch/927995/ Could you help to take a
look?

Thanks,

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

Re: [ovs-dev] [PATCH] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Lucas Alvares Gomes]

Hi Yi-Hung Wei,

> But as mentioned in ovs commit 6c0bf091 ("datapath: use core MTU range
> checking in core net infra"), it might be the case that my commit [0]
> does not set max_mtu correctly.  How about the fix in the following?
> From what I tested, without the fix, min_mtu: 64, max_mtu: 1500, with
> that fix, min_mtu:64 and max_mtu: 65535.
>
>> As pointed out by commit [0], the ndo_change_mtu function pointer has been
>> moved from 'struct net_device_ops' to 'struct net_device_ops_extended'
>> on RHEL 7.5.
>>
>> So this patch fixes the backport issue by setting the
>> .extended.ndo_change_mtu when necessary.
>>
>> [0] 39ca338374abe367e28a2247bac9159695f19710
>
> --- a/datapath/vport-internal_dev.c
> +++ b/datapath/vport-internal_dev.c
> @@ -169,6 +169,8 @@ static void do_setup(struct net_device *netdev)
>
>  #ifdef HAVE_NET_DEVICE_WITH_MAX_MTU
> netdev->max_mtu = ETH_MAX_MTU;
> +#elif HAVE_RHEL7_MAX_MTU
> +   netdev->extended->max_mtu = ETH_MAX_MTU;
>  #endif
> netdev->netdev_ops = _dev_netdev_ops;

Cool! I will give this a go and see if it works.

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

Re: [ovs-dev] [PATCH] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Aaron Conole]

Aaron Conole  writes:

> lucasago...@gmail.com writes:
>
>> From: Lucas Alvares Gomes 
>>
>> The commit [0] partially fixed the problem but in RHEL 7.5 neither
>> .{min, max}_mtu or 'ndo_change_mtu' were being set/implemented for
>> vport-internal_dev.c.
>>
>> As pointed out by commit [0], the ndo_change_mtu function pointer has been
>> moved from 'struct net_device_ops' to 'struct net_device_ops_extended'
>> on RHEL 7.5.
>>
>> So this patch fixes the backport issue by setting the
>> .extended.ndo_change_mtu when necessary.
>>
>> [0] 39ca338374abe367e28a2247bac9159695f19710
>
> Which commit is this?  Can you also include the commit subject?  I can't
> find it in either the ovs, linux, or even rhel kernel repository.

Disregard.  Found it in OvS.  Apparently I didn't search in the correct
tree.

>> ---
>>  datapath/vport-internal_dev.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
>> index 3cb8d06b2..16f4aaeee 100644
>> --- a/datapath/vport-internal_dev.c
>> +++ b/datapath/vport-internal_dev.c
>> @@ -88,7 +88,7 @@ static const struct ethtool_ops internal_dev_ethtool_ops = 
>> {
>>  .get_link   = ethtool_op_get_link,
>>  };
>>  
>> -#if !defined(HAVE_NET_DEVICE_WITH_MAX_MTU) && !defined(HAVE_RHEL7_MAX_MTU)
>> +#ifndef HAVE_NET_DEVICE_WITH_MAX_MTU
>>  static int internal_dev_change_mtu(struct net_device *dev, int new_mtu)
>>  {
>>  if (new_mtu < ETH_MIN_MTU) {
>> @@ -155,6 +155,8 @@ static const struct net_device_ops 
>> internal_dev_netdev_ops = {
>>  .ndo_set_mac_address = eth_mac_addr,
>>  #if !defined(HAVE_NET_DEVICE_WITH_MAX_MTU) && !defined(HAVE_RHEL7_MAX_MTU)
>>  .ndo_change_mtu = internal_dev_change_mtu,
>> +#elif   !defined(HAVE_NET_DEVICE_WITH_MAX_MTU) && 
>> defined(HAVE_RHEL7_MAX_MTU)
>> +.extended.ndo_change_mtu = internal_dev_change_mtu,
>>  #endif
>>  .ndo_get_stats64 = (void *)internal_get_stats,
>>  };
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Aaron Conole]

Daniel Alvarez Sanchez  writes:

> Thanks Lucas, this makes sense. There is something that this patch is
> fixing and I'm not sure why.
> Maybe someone can shed some light:
>
> Using datapath from OVS master, and a setup where we have a physical
> interface connected to
> an OVS bridge (br-ex) connected to another OVS bridge (br-int) through a
> patch port, there's a lot
> of retransmissions of TCP packets when connecting from the host to a VM
> connected to br-int.
> The retransmissions seem to be due to a wrong checksum from the VM to the
> host and only after
> a few attempts, the checksum is correct and the host sends the ACK back.
> The packets I am
> sending using netcat are very small so there shouldn't be a problem with
> the MTU. However, could
> it be a side effect of this patch that the checksum gets now correctly
> received at the host?
>
> As a side not: if instead from connecting to the VM from the host I do it
> from a namespace where
> I have an OVS internal port connected to br-ex, then I don't see the
> checksum problems.

I'm concerned - I don't see why this checksum issue would occur.
Additionally, I see no reason for this patch to clear it up.

Do you have a clear reproducer?

> Acked-by: Daniel Alvarez 
> Tested-by: Daniel Alvarez 
>
> On Thu, May 17, 2018 at 1:27 PM,  wrote:
>
>> From: Lucas Alvares Gomes 
>>
>> The commit [0] partially fixed the problem but in RHEL 7.5 neither
>> .{min, max}_mtu or 'ndo_change_mtu' were being set/implemented for
>> vport-internal_dev.c.
>>
>> As pointed out by commit [0], the ndo_change_mtu function pointer has been
>> moved from 'struct net_device_ops' to 'struct net_device_ops_extended'
>> on RHEL 7.5.
>>
>> So this patch fixes the backport issue by setting the
>> .extended.ndo_change_mtu when necessary.
>>
>> [0] 39ca338374abe367e28a2247bac9159695f19710
>> ---
>>  datapath/vport-internal_dev.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
>> index 3cb8d06b2..16f4aaeee 100644
>> --- a/datapath/vport-internal_dev.c
>> +++ b/datapath/vport-internal_dev.c
>> @@ -88,7 +88,7 @@ static const struct ethtool_ops internal_dev_ethtool_ops
>> = {
>> .get_link   = ethtool_op_get_link,
>>  };
>>
>> -#if!defined(HAVE_NET_DEVICE_WITH_MAX_MTU) &&
>> !defined(HAVE_RHEL7_MAX_MTU)
>> +#ifndef HAVE_NET_DEVICE_WITH_MAX_MTU
>>  static int internal_dev_change_mtu(struct net_device *dev, int new_mtu)
>>  {
>> if (new_mtu < ETH_MIN_MTU) {
>> @@ -155,6 +155,8 @@ static const struct net_device_ops
>> internal_dev_netdev_ops = {
>> .ndo_set_mac_address = eth_mac_addr,
>>  #if!defined(HAVE_NET_DEVICE_WITH_MAX_MTU) &&
>> !defined(HAVE_RHEL7_MAX_MTU)
>> .ndo_change_mtu = internal_dev_change_mtu,
>> +#elif  !defined(HAVE_NET_DEVICE_WITH_MAX_MTU) &&
>> defined(HAVE_RHEL7_MAX_MTU)
>> +   .extended.ndo_change_mtu = internal_dev_change_mtu,
>>  #endif
>> .ndo_get_stats64 = (void *)internal_get_stats,
>>  };
>> --
>> 2.17.0
>>
>> ___
>> 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] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Aaron Conole]

lucasago...@gmail.com writes:

> From: Lucas Alvares Gomes 
>
> The commit [0] partially fixed the problem but in RHEL 7.5 neither
> .{min, max}_mtu or 'ndo_change_mtu' were being set/implemented for
> vport-internal_dev.c.
>
> As pointed out by commit [0], the ndo_change_mtu function pointer has been
> moved from 'struct net_device_ops' to 'struct net_device_ops_extended'
> on RHEL 7.5.
>
> So this patch fixes the backport issue by setting the
> .extended.ndo_change_mtu when necessary.
>
> [0] 39ca338374abe367e28a2247bac9159695f19710

Which commit is this?  Can you also include the commit subject?  I can't
find it in either the ovs, linux, or even rhel kernel repository.

> ---
>  datapath/vport-internal_dev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
> index 3cb8d06b2..16f4aaeee 100644
> --- a/datapath/vport-internal_dev.c
> +++ b/datapath/vport-internal_dev.c
> @@ -88,7 +88,7 @@ static const struct ethtool_ops internal_dev_ethtool_ops = {
>   .get_link   = ethtool_op_get_link,
>  };
>  
> -#if  !defined(HAVE_NET_DEVICE_WITH_MAX_MTU) && !defined(HAVE_RHEL7_MAX_MTU)
> +#ifndef HAVE_NET_DEVICE_WITH_MAX_MTU
>  static int internal_dev_change_mtu(struct net_device *dev, int new_mtu)
>  {
>   if (new_mtu < ETH_MIN_MTU) {
> @@ -155,6 +155,8 @@ static const struct net_device_ops 
> internal_dev_netdev_ops = {
>   .ndo_set_mac_address = eth_mac_addr,
>  #if  !defined(HAVE_NET_DEVICE_WITH_MAX_MTU) && !defined(HAVE_RHEL7_MAX_MTU)
>   .ndo_change_mtu = internal_dev_change_mtu,
> +#elif!defined(HAVE_NET_DEVICE_WITH_MAX_MTU) && 
> defined(HAVE_RHEL7_MAX_MTU)
> + .extended.ndo_change_mtu = internal_dev_change_mtu,
>  #endif
>   .ndo_get_stats64 = (void *)internal_get_stats,
>  };
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Yi-Hung Wei]

On Thu, May 17, 2018 at 4:27 AM,   wrote:
> From: Lucas Alvares Gomes 
>
> The commit [0] partially fixed the problem but in RHEL 7.5 neither
> .{min, max}_mtu or 'ndo_change_mtu' were being set/implemented for
> vport-internal_dev.c.
>
Thanks Lucas for finding this bug and the fix.   I take a look RHEL
7.5 kernel again, if neither ndo_change_mtu() or ndo_change_mtu_rh74()
is not implemented it should use dev_set_mtu() in ./net/core/dev.c. I
also found that the {min,max}_mtu is set in ./net/ethernet/eth.c

void ether_setup_rh(struct net_device *dev)
{
ether_setup(dev);
dev->extended->min_mtu= ETH_MIN_MTU;
dev->extended->max_mtu= ETH_DATA_LEN;
}

But as mentioned in ovs commit 6c0bf091 ("datapath: use core MTU range
checking in core net infra"), it might be the case that my commit [0]
does not set max_mtu correctly.  How about the fix in the following?
>From what I tested, without the fix, min_mtu: 64, max_mtu: 1500, with
that fix, min_mtu:64 and max_mtu: 65535.

> As pointed out by commit [0], the ndo_change_mtu function pointer has been
> moved from 'struct net_device_ops' to 'struct net_device_ops_extended'
> on RHEL 7.5.
>
> So this patch fixes the backport issue by setting the
> .extended.ndo_change_mtu when necessary.
>
> [0] 39ca338374abe367e28a2247bac9159695f19710

--- a/datapath/vport-internal_dev.c
+++ b/datapath/vport-internal_dev.c
@@ -169,6 +169,8 @@ static void do_setup(struct net_device *netdev)

 #ifdef HAVE_NET_DEVICE_WITH_MAX_MTU
netdev->max_mtu = ETH_MAX_MTU;
+#elif HAVE_RHEL7_MAX_MTU
+   netdev->extended->max_mtu = ETH_MAX_MTU;
 #endif
netdev->netdev_ops = _dev_netdev_ops;


Thanks,

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

Re: [ovs-dev] [PATCH] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Daniel Alvarez Sanchez]

Thanks Lucas, this makes sense. There is something that this patch is
fixing and I'm not sure why.
Maybe someone can shed some light:

Using datapath from OVS master, and a setup where we have a physical
interface connected to
an OVS bridge (br-ex) connected to another OVS bridge (br-int) through a
patch port, there's a lot
of retransmissions of TCP packets when connecting from the host to a VM
connected to br-int.
The retransmissions seem to be due to a wrong checksum from the VM to the
host and only after
a few attempts, the checksum is correct and the host sends the ACK back.
The packets I am
sending using netcat are very small so there shouldn't be a problem with
the MTU. However, could
it be a side effect of this patch that the checksum gets now correctly
received at the host?

As a side not: if instead from connecting to the VM from the host I do it
from a namespace where
I have an OVS internal port connected to br-ex, then I don't see the
checksum problems.

Acked-by: Daniel Alvarez 
Tested-by: Daniel Alvarez 

On Thu, May 17, 2018 at 1:27 PM,  wrote:

> From: Lucas Alvares Gomes 
>
> The commit [0] partially fixed the problem but in RHEL 7.5 neither
> .{min, max}_mtu or 'ndo_change_mtu' were being set/implemented for
> vport-internal_dev.c.
>
> As pointed out by commit [0], the ndo_change_mtu function pointer has been
> moved from 'struct net_device_ops' to 'struct net_device_ops_extended'
> on RHEL 7.5.
>
> So this patch fixes the backport issue by setting the
> .extended.ndo_change_mtu when necessary.
>
> [0] 39ca338374abe367e28a2247bac9159695f19710
> ---
>  datapath/vport-internal_dev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
> index 3cb8d06b2..16f4aaeee 100644
> --- a/datapath/vport-internal_dev.c
> +++ b/datapath/vport-internal_dev.c
> @@ -88,7 +88,7 @@ static const struct ethtool_ops internal_dev_ethtool_ops
> = {
> .get_link   = ethtool_op_get_link,
>  };
>
> -#if!defined(HAVE_NET_DEVICE_WITH_MAX_MTU) &&
> !defined(HAVE_RHEL7_MAX_MTU)
> +#ifndef HAVE_NET_DEVICE_WITH_MAX_MTU
>  static int internal_dev_change_mtu(struct net_device *dev, int new_mtu)
>  {
> if (new_mtu < ETH_MIN_MTU) {
> @@ -155,6 +155,8 @@ static const struct net_device_ops
> internal_dev_netdev_ops = {
> .ndo_set_mac_address = eth_mac_addr,
>  #if!defined(HAVE_NET_DEVICE_WITH_MAX_MTU) &&
> !defined(HAVE_RHEL7_MAX_MTU)
> .ndo_change_mtu = internal_dev_change_mtu,
> +#elif  !defined(HAVE_NET_DEVICE_WITH_MAX_MTU) &&
> defined(HAVE_RHEL7_MAX_MTU)
> +   .extended.ndo_change_mtu = internal_dev_change_mtu,
>  #endif
> .ndo_get_stats64 = (void *)internal_get_stats,
>  };
> --
> 2.17.0
>
> ___
> 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] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Lucas Alvares Gomes]

On Thu, May 17, 2018 at 12:27 PM,   wrote:
> From: Lucas Alvares Gomes 
>
> The commit [0] partially fixed the problem but in RHEL 7.5 neither
> .{min, max}_mtu or 'ndo_change_mtu' were being set/implemented for
> vport-internal_dev.c.
>
> As pointed out by commit [0], the ndo_change_mtu function pointer has been
> moved from 'struct net_device_ops' to 'struct net_device_ops_extended'
> on RHEL 7.5.
>
> So this patch fixes the backport issue by setting the
> .extended.ndo_change_mtu when necessary.
>
> [0] 39ca338374abe367e28a2247bac9159695f19710

Signed-off-by: Lucas Alvares Gomes 

Sorry, forgot it to append it to the commit message before submitting the patch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[lucasagomes]

From: Lucas Alvares Gomes 

The commit [0] partially fixed the problem but in RHEL 7.5 neither
.{min, max}_mtu or 'ndo_change_mtu' were being set/implemented for
vport-internal_dev.c.

As pointed out by commit [0], the ndo_change_mtu function pointer has been
moved from 'struct net_device_ops' to 'struct net_device_ops_extended'
on RHEL 7.5.

So this patch fixes the backport issue by setting the
.extended.ndo_change_mtu when necessary.

[0] 39ca338374abe367e28a2247bac9159695f19710
---
 datapath/vport-internal_dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
index 3cb8d06b2..16f4aaeee 100644
--- a/datapath/vport-internal_dev.c
+++ b/datapath/vport-internal_dev.c
@@ -88,7 +88,7 @@ static const struct ethtool_ops internal_dev_ethtool_ops = {
.get_link   = ethtool_op_get_link,
 };
 
-#if!defined(HAVE_NET_DEVICE_WITH_MAX_MTU) && !defined(HAVE_RHEL7_MAX_MTU)
+#ifndef HAVE_NET_DEVICE_WITH_MAX_MTU
 static int internal_dev_change_mtu(struct net_device *dev, int new_mtu)
 {
if (new_mtu < ETH_MIN_MTU) {
@@ -155,6 +155,8 @@ static const struct net_device_ops internal_dev_netdev_ops 
= {
.ndo_set_mac_address = eth_mac_addr,
 #if!defined(HAVE_NET_DEVICE_WITH_MAX_MTU) && !defined(HAVE_RHEL7_MAX_MTU)
.ndo_change_mtu = internal_dev_change_mtu,
+#elif  !defined(HAVE_NET_DEVICE_WITH_MAX_MTU) && defined(HAVE_RHEL7_MAX_MTU)
+   .extended.ndo_change_mtu = internal_dev_change_mtu,
 #endif
.ndo_get_stats64 = (void *)internal_get_stats,
 };
-- 
2.17.0

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