| Message ID | 1578346594-26279-1-git-send-email-gvrose8192@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev] compat: Include confirm_neigh parameter if needed | expand |
On Mon, Jan 06, 2020 at 01:36:34PM -0800, Greg Rose wrote: > A change backported to the Linux 4.14.162 LTS kernel requires > a boolean parameter. Check for the presence of the parameter > and adjust the caller in that case. > > Passes check-kmod test with no regressions. > > Passes Travis build here: > https://travis-ci.org/gvrose8192/ovs-experimental/builds/633461320 > > Signed-off-by: Greg Rose <gvrose8192@gmail.com> Thanks Greg, I have pushed this to master with a view to quickly resolving the build problem there. I also plan to push the change to to branch-2.10 ... 2.12 once I've confirmed the problem exists there and testing of those backports is complete: branch-2.12: https://travis-ci.org/horms2/ovs/builds/633651396 branch-2.12 + backport: https://travis-ci.org/horms2/ovs/builds/633674666 branch-2.11: https://travis-ci.org/horms2/ovs/builds/633651456 branch-2.11 + backport: https://travis-ci.org/horms2/ovs/builds/633676376 branch-2.10: https://travis-ci.org/horms2/ovs/builds/633676916 branch-2.10 + backport: https://travis-ci.org/horms2/ovs/builds/633677490 > --- > acinclude.m4 | 2 ++ > datapath/linux/compat/ip6_gre.c | 4 ++++ > datapath/linux/compat/ip_tunnel.c | 5 +++++ > 3 files changed, 11 insertions(+) > > diff --git a/acinclude.m4 b/acinclude.m4 > index 542637a..18264c4 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -1065,6 +1065,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ > [OVS_DEFINE([HAVE_IP_GRE_CALC_HLEN])]) > OVS_GREP_IFELSE([$KSRC/include/linux/rbtree.h], [rb_link_node_rcu], > [OVS_DEFINE([HAVE_RBTREE_RB_LINK_NODE_RCU])]) > + OVS_GREP_IFELSE([$KSRC/include/net/dst_ops.h], [bool confirm_neigh], > + [OVS_DEFINE([HAVE_DST_OPS_CONFIRM_NEIGH])]) > > if cmp -s datapath/linux/kcompat.h.new \ > datapath/linux/kcompat.h >/dev/null 2>&1; then > diff --git a/datapath/linux/compat/ip6_gre.c b/datapath/linux/compat/ip6_gre.c > index afff817..7fd3453 100644 > --- a/datapath/linux/compat/ip6_gre.c > +++ b/datapath/linux/compat/ip6_gre.c > @@ -1089,7 +1089,11 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb, > > /* TooBig packet may have updated dst->dev's mtu */ > if (!t->parms.collect_md && dst && dst_mtu(dst) > dst->dev->mtu) > +#ifndef HAVE_DST_OPS_CONFIRM_NEIGH > dst->ops->update_pmtu(dst, NULL, skb, dst->dev->mtu); > +#else > + dst->ops->update_pmtu(dst, NULL, skb, dst->dev->mtu, false); > +#endif Did you consider using skb_dst_update_pmtu() unconditionally? That may be cleaner going forwards. > > err = ip6_tnl_xmit(skb, dev, dsfield, &fl6, encap_limit, &mtu, > NEXTHDR_GRE); > diff --git a/datapath/linux/compat/ip_tunnel.c b/datapath/linux/compat/ip_tunnel.c > index 7dd57fe..e7a0393 100644 > --- a/datapath/linux/compat/ip_tunnel.c > +++ b/datapath/linux/compat/ip_tunnel.c > @@ -267,7 +267,12 @@ static int rpl_tnl_update_pmtu(struct net_device *dev, struct sk_buff *skb, > mtu = skb_valid_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu; > > if (skb_valid_dst(skb)) > +#ifndef HAVE_DST_OPS_CONFIRM_NEIGH > skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu); > +#else > + skb_dst(skb)->ops->update_pmtu(skb_dst(skb), > + NULL, skb, mtu, false); > +#endif > > if (skb->protocol == htons(ETH_P_IP)) { > if (!skb_is_gso(skb) && > -- > 1.8.3.1 >
On 1/7/2020 1:34 AM, Simon Horman wrote: > On Mon, Jan 06, 2020 at 01:36:34PM -0800, Greg Rose wrote: >> A change backported to the Linux 4.14.162 LTS kernel requires >> a boolean parameter. Check for the presence of the parameter >> and adjust the caller in that case. >> >> Passes check-kmod test with no regressions. >> >> Passes Travis build here: >> https://travis-ci.org/gvrose8192/ovs-experimental/builds/633461320 >> >> Signed-off-by: Greg Rose <gvrose8192@gmail.com> > Thanks Greg, > > I have pushed this to master with a view to quickly resolving > the build problem there. > > I also plan to push the change to to branch-2.10 ... 2.12 > once I've confirmed the problem exists there and testing of those backports > is complete: > > branch-2.12: https://travis-ci.org/horms2/ovs/builds/633651396 > branch-2.12 + backport: https://travis-ci.org/horms2/ovs/builds/633674666 > branch-2.11: https://travis-ci.org/horms2/ovs/builds/633651456 > branch-2.11 + backport: https://travis-ci.org/horms2/ovs/builds/633676376 > branch-2.10: https://travis-ci.org/horms2/ovs/builds/633676916 > branch-2.10 + backport: https://travis-ci.org/horms2/ovs/builds/633677490 Oh good, thanks! >> --- >> acinclude.m4 | 2 ++ >> datapath/linux/compat/ip6_gre.c | 4 ++++ >> datapath/linux/compat/ip_tunnel.c | 5 +++++ >> 3 files changed, 11 insertions(+) >> >> diff --git a/acinclude.m4 b/acinclude.m4 >> index 542637a..18264c4 100644 >> --- a/acinclude.m4 >> +++ b/acinclude.m4 >> @@ -1065,6 +1065,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ >> [OVS_DEFINE([HAVE_IP_GRE_CALC_HLEN])]) >> OVS_GREP_IFELSE([$KSRC/include/linux/rbtree.h], [rb_link_node_rcu], >> [OVS_DEFINE([HAVE_RBTREE_RB_LINK_NODE_RCU])]) >> + OVS_GREP_IFELSE([$KSRC/include/net/dst_ops.h], [bool confirm_neigh], >> + [OVS_DEFINE([HAVE_DST_OPS_CONFIRM_NEIGH])]) >> >> if cmp -s datapath/linux/kcompat.h.new \ >> datapath/linux/kcompat.h >/dev/null 2>&1; then >> diff --git a/datapath/linux/compat/ip6_gre.c b/datapath/linux/compat/ip6_gre.c >> index afff817..7fd3453 100644 >> --- a/datapath/linux/compat/ip6_gre.c >> +++ b/datapath/linux/compat/ip6_gre.c >> @@ -1089,7 +1089,11 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb, >> >> /* TooBig packet may have updated dst->dev's mtu */ >> if (!t->parms.collect_md && dst && dst_mtu(dst) > dst->dev->mtu) >> +#ifndef HAVE_DST_OPS_CONFIRM_NEIGH >> dst->ops->update_pmtu(dst, NULL, skb, dst->dev->mtu); >> +#else >> + dst->ops->update_pmtu(dst, NULL, skb, dst->dev->mtu, false); >> +#endif > Did you consider using skb_dst_update_pmtu() unconditionally? > That may be cleaner going forwards. Yes, but it defaults to true for the boolean parameter to confirm the neighbor entry and I didn't want to change behavior for older kernels. If you think that's not a concern then I'd be happy to respin the patch using skb_dst_update_pmtu(). - Greg
On Tue, Jan 07, 2020 at 10:34:37AM +0100, Simon Horman wrote: > On Mon, Jan 06, 2020 at 01:36:34PM -0800, Greg Rose wrote: > > A change backported to the Linux 4.14.162 LTS kernel requires > > a boolean parameter. Check for the presence of the parameter > > and adjust the caller in that case. > > > > Passes check-kmod test with no regressions. > > > > Passes Travis build here: > > https://travis-ci.org/gvrose8192/ovs-experimental/builds/633461320 > > > > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > > Thanks Greg, > > I have pushed this to master with a view to quickly resolving > the build problem there. > > I also plan to push the change to to branch-2.10 ... 2.12 > once I've confirmed the problem exists there and testing of those backports > is complete: > > branch-2.12: https://travis-ci.org/horms2/ovs/builds/633651396 > branch-2.12 + backport: https://travis-ci.org/horms2/ovs/builds/633674666 > branch-2.11: https://travis-ci.org/horms2/ovs/builds/633651456 > branch-2.11 + backport: https://travis-ci.org/horms2/ovs/builds/633676376 > branch-2.10: https://travis-ci.org/horms2/ovs/builds/633676916 > branch-2.10 + backport: https://travis-ci.org/horms2/ovs/builds/633677490 Empirically it appears the backport is only needed for branch-2.12, I have pushed it there.
On 07.01.2020 18:08, Simon Horman wrote: > On Tue, Jan 07, 2020 at 10:34:37AM +0100, Simon Horman wrote: >> On Mon, Jan 06, 2020 at 01:36:34PM -0800, Greg Rose wrote: >>> A change backported to the Linux 4.14.162 LTS kernel requires >>> a boolean parameter. Check for the presence of the parameter >>> and adjust the caller in that case. >>> >>> Passes check-kmod test with no regressions. >>> >>> Passes Travis build here: >>> https://travis-ci.org/gvrose8192/ovs-experimental/builds/633461320 >>> >>> Signed-off-by: Greg Rose <gvrose8192@gmail.com> >> >> Thanks Greg, >> >> I have pushed this to master with a view to quickly resolving >> the build problem there. >> >> I also plan to push the change to to branch-2.10 ... 2.12 >> once I've confirmed the problem exists there and testing of those backports >> is complete: >> >> branch-2.12: https://travis-ci.org/horms2/ovs/builds/633651396 >> branch-2.12 + backport: https://travis-ci.org/horms2/ovs/builds/633674666 >> branch-2.11: https://travis-ci.org/horms2/ovs/builds/633651456 >> branch-2.11 + backport: https://travis-ci.org/horms2/ovs/builds/633676376 >> branch-2.10: https://travis-ci.org/horms2/ovs/builds/633676916 >> branch-2.10 + backport: https://travis-ci.org/horms2/ovs/builds/633677490 > > Empirically it appears the backport is only needed > for branch-2.12, I have pushed it there. > We might still consider backporting. Travis doesn't fail on branches before 2.12 only because commit c94e2d64f05e ("travis: Test with latest stable kernel releases.") was introduced in 2.12. On earlier branches we're using older versions of stable kernels. These branches could still fail to build if we'll try to build them with 4.14.162. Best regards, Ilya Maximets.
On Tue, Jan 07, 2020 at 08:35:57AM -0800, Gregory Rose wrote: > On 1/7/2020 1:34 AM, Simon Horman wrote: > > On Mon, Jan 06, 2020 at 01:36:34PM -0800, Greg Rose wrote: ... > > > --- a/datapath/linux/compat/ip6_gre.c > > > +++ b/datapath/linux/compat/ip6_gre.c > > > @@ -1089,7 +1089,11 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb, > > > /* TooBig packet may have updated dst->dev's mtu */ > > > if (!t->parms.collect_md && dst && dst_mtu(dst) > dst->dev->mtu) > > > +#ifndef HAVE_DST_OPS_CONFIRM_NEIGH > > > dst->ops->update_pmtu(dst, NULL, skb, dst->dev->mtu); > > > +#else > > > + dst->ops->update_pmtu(dst, NULL, skb, dst->dev->mtu, false); > > > +#endif > > Did you consider using skb_dst_update_pmtu() unconditionally? > > That may be cleaner going forwards. > > Yes, but it defaults to true for the boolean parameter to confirm the > neighbor entry > and I didn't want to change behavior for older kernels. If you think that's > not a concern > then I'd be happy to respin the patch using skb_dst_update_pmtu(). Thanks Greg, after reviewing things another time I agree that what you have done is correct.
On Tue, Jan 07, 2020 at 06:13:32PM +0100, Ilya Maximets wrote: > On 07.01.2020 18:08, Simon Horman wrote: > > On Tue, Jan 07, 2020 at 10:34:37AM +0100, Simon Horman wrote: > >> On Mon, Jan 06, 2020 at 01:36:34PM -0800, Greg Rose wrote: > >>> A change backported to the Linux 4.14.162 LTS kernel requires > >>> a boolean parameter. Check for the presence of the parameter > >>> and adjust the caller in that case. > >>> > >>> Passes check-kmod test with no regressions. > >>> > >>> Passes Travis build here: > >>> https://travis-ci.org/gvrose8192/ovs-experimental/builds/633461320 > >>> > >>> Signed-off-by: Greg Rose <gvrose8192@gmail.com> > >> > >> Thanks Greg, > >> > >> I have pushed this to master with a view to quickly resolving > >> the build problem there. > >> > >> I also plan to push the change to to branch-2.10 ... 2.12 > >> once I've confirmed the problem exists there and testing of those backports > >> is complete: > >> > >> branch-2.12: https://travis-ci.org/horms2/ovs/builds/633651396 > >> branch-2.12 + backport: https://travis-ci.org/horms2/ovs/builds/633674666 > >> branch-2.11: https://travis-ci.org/horms2/ovs/builds/633651456 > >> branch-2.11 + backport: https://travis-ci.org/horms2/ovs/builds/633676376 > >> branch-2.10: https://travis-ci.org/horms2/ovs/builds/633676916 > >> branch-2.10 + backport: https://travis-ci.org/horms2/ovs/builds/633677490 > > > > Empirically it appears the backport is only needed > > for branch-2.12, I have pushed it there. > > > > We might still consider backporting. Travis doesn't fail on branches > before 2.12 only because commit c94e2d64f05e ("travis: Test with latest > stable kernel releases.") was introduced in 2.12. On earlier branches > we're using older versions of stable kernels. These branches could still > fail to build if we'll try to build them with 4.14.162. Thanks, I tested that locally by building branch-2.10 and branch-2.11 locally against v4.14.161 and 4.14.162. Accordingly I have pushed the backport to those branches. FWIIW, I noticed that branch-2.9 does not build against newer v4.14 releases. But the problem discussed in this thread does not seem relevant there.
On 1/8/2020 12:57 AM, Simon Horman wrote: > On Tue, Jan 07, 2020 at 08:35:57AM -0800, Gregory Rose wrote: >> On 1/7/2020 1:34 AM, Simon Horman wrote: >>> On Mon, Jan 06, 2020 at 01:36:34PM -0800, Greg Rose wrote: > ... > >>>> --- a/datapath/linux/compat/ip6_gre.c >>>> +++ b/datapath/linux/compat/ip6_gre.c >>>> @@ -1089,7 +1089,11 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb, >>>> /* TooBig packet may have updated dst->dev's mtu */ >>>> if (!t->parms.collect_md && dst && dst_mtu(dst) > dst->dev->mtu) >>>> +#ifndef HAVE_DST_OPS_CONFIRM_NEIGH >>>> dst->ops->update_pmtu(dst, NULL, skb, dst->dev->mtu); >>>> +#else >>>> + dst->ops->update_pmtu(dst, NULL, skb, dst->dev->mtu, false); >>>> +#endif >>> Did you consider using skb_dst_update_pmtu() unconditionally? >>> That may be cleaner going forwards. >> Yes, but it defaults to true for the boolean parameter to confirm the >> neighbor entry >> and I didn't want to change behavior for older kernels. If you think that's >> not a concern >> then I'd be happy to respin the patch using skb_dst_update_pmtu(). > Thanks Greg, > > after reviewing things another time I agree that what you have done is > correct. Good deal, thanks Simon! - Greg
diff --git a/acinclude.m4 b/acinclude.m4 index 542637a..18264c4 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -1065,6 +1065,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ [OVS_DEFINE([HAVE_IP_GRE_CALC_HLEN])]) OVS_GREP_IFELSE([$KSRC/include/linux/rbtree.h], [rb_link_node_rcu], [OVS_DEFINE([HAVE_RBTREE_RB_LINK_NODE_RCU])]) + OVS_GREP_IFELSE([$KSRC/include/net/dst_ops.h], [bool confirm_neigh], + [OVS_DEFINE([HAVE_DST_OPS_CONFIRM_NEIGH])]) if cmp -s datapath/linux/kcompat.h.new \ datapath/linux/kcompat.h >/dev/null 2>&1; then diff --git a/datapath/linux/compat/ip6_gre.c b/datapath/linux/compat/ip6_gre.c index afff817..7fd3453 100644 --- a/datapath/linux/compat/ip6_gre.c +++ b/datapath/linux/compat/ip6_gre.c @@ -1089,7 +1089,11 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb, /* TooBig packet may have updated dst->dev's mtu */ if (!t->parms.collect_md && dst && dst_mtu(dst) > dst->dev->mtu) +#ifndef HAVE_DST_OPS_CONFIRM_NEIGH dst->ops->update_pmtu(dst, NULL, skb, dst->dev->mtu); +#else + dst->ops->update_pmtu(dst, NULL, skb, dst->dev->mtu, false); +#endif err = ip6_tnl_xmit(skb, dev, dsfield, &fl6, encap_limit, &mtu, NEXTHDR_GRE); diff --git a/datapath/linux/compat/ip_tunnel.c b/datapath/linux/compat/ip_tunnel.c index 7dd57fe..e7a0393 100644 --- a/datapath/linux/compat/ip_tunnel.c +++ b/datapath/linux/compat/ip_tunnel.c @@ -267,7 +267,12 @@ static int rpl_tnl_update_pmtu(struct net_device *dev, struct sk_buff *skb, mtu = skb_valid_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu; if (skb_valid_dst(skb)) +#ifndef HAVE_DST_OPS_CONFIRM_NEIGH skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu); +#else + skb_dst(skb)->ops->update_pmtu(skb_dst(skb), + NULL, skb, mtu, false); +#endif if (skb->protocol == htons(ETH_P_IP)) { if (!skb_is_gso(skb) &&
A change backported to the Linux 4.14.162 LTS kernel requires a boolean parameter. Check for the presence of the parameter and adjust the caller in that case. Passes check-kmod test with no regressions. Passes Travis build here: https://travis-ci.org/gvrose8192/ovs-experimental/builds/633461320 Signed-off-by: Greg Rose <gvrose8192@gmail.com> --- acinclude.m4 | 2 ++ datapath/linux/compat/ip6_gre.c | 4 ++++ datapath/linux/compat/ip_tunnel.c | 5 +++++ 3 files changed, 11 insertions(+)