Message ID | 20180517112750.10424-1-lucasagomes@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev] datapath: RHEL 7.5 ndo_change_mtu backward compatibility | expand |
On Thu, May 17, 2018 at 12:27 PM, <lucasagomes@gmail.com> wrote: > From: Lucas Alvares Gomes <lucasagomes@gmail.com> > > 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 <lucasagomes@gmail.com> Sorry, forgot it to append it to the commit message before submitting the patch.
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 <dalvarez@redhat.com> Tested-by: Daniel Alvarez <dalvarez@redhat.com> On Thu, May 17, 2018 at 1:27 PM, <lucasagomes@gmail.com> wrote: > From: Lucas Alvares Gomes <lucasagomes@gmail.com> > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, May 17, 2018 at 4:27 AM, <lucasagomes@gmail.com> wrote: > From: Lucas Alvares Gomes <lucasagomes@gmail.com> > > 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 = &internal_dev_netdev_ops; Thanks, -Yi-Hung
lucasagomes@gmail.com writes: > From: Lucas Alvares Gomes <lucasagomes@gmail.com> > > 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, > };
Daniel Alvarez Sanchez <dalvarez@redhat.com> 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 <dalvarez@redhat.com> > Tested-by: Daniel Alvarez <dalvarez@redhat.com> > > On Thu, May 17, 2018 at 1:27 PM, <lucasagomes@gmail.com> wrote: > >> From: Lucas Alvares Gomes <lucasagomes@gmail.com> >> >> 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Aaron Conole <aconole@redhat.com> writes: > lucasagomes@gmail.com writes: > >> From: Lucas Alvares Gomes <lucasagomes@gmail.com> >> >> 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, >> };
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 = &internal_dev_netdev_ops; Cool! I will give this a go and see if it works. Cheers, Lucas
On Fri, May 18, 2018 at 1:41 AM, Lucas Alvares Gomes <lucasagomes@gmail.com> 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 = &internal_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
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
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, };
From: Lucas Alvares Gomes <lucasagomes@gmail.com> 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(-)