Re: [PATCH net-next v5 1/2] net: UDP tunnel encapsulation module for tunnelling different protocols like MPLS,IP,NSH etc.

[Martin Varghese <martinvarghesenokia@gmail.com>]

On Thu, Jan 23, 2020 at 05:42:25PM -0500, Willem de Bruijn wrote:
> On Thu, Jan 23, 2020 at 1:04 PM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > From: Martin Varghese <martin.varghese@nokia.com>
> >
> > The Bareudp tunnel module provides a generic L3 encapsulation
> > tunnelling module for tunnelling different protocols like MPLS,
> > IP,NSH etc inside a UDP tunnel.
> >
> > Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
> 
> > diff --git a/include/net/ip6_tunnel.h b/include/net/ip6_tunnel.h
> > index 028eaea..8215d1b 100644
> > --- a/include/net/ip6_tunnel.h
> > +++ b/include/net/ip6_tunnel.h
> > @@ -165,5 +165,55 @@ static inline void ip6tunnel_xmit(struct sock *sk, struct sk_buff *skb,
> >                 iptunnel_xmit_stats(dev, pkt_len);
> >         }
> >  }
> > +
> > +static inline struct dst_entry *ip6tunnel_get_dst(struct sk_buff *skb,
> > +                                                 struct net_device *dev,
> > +                                                 struct net *net,
> > +                                                 struct socket *sock,
> > +                                                 struct flowi6 *fl6,
> > +                                                 const struct ip_tunnel_info *info,
> > +                                                 bool use_cache)
> > +{
> > +       struct dst_entry *dst = NULL;
> > +#ifdef CONFIG_DST_CACHE
> > +       struct dst_cache *dst_cache;
> > +#endif
> 
> I just noticed these ifdefs are absent in Geneve. On closer look,
> CONFIG_NET_UDP_TUNNEL selects CONFIG_NET_IP_TUNNEL selects
> CONFIG_DST_CACHE. So they are indeed not needed.
> 
> Sorry, should have noticed that in v4. It could conceivably be fixed
> up later, but seems worth one more round to get it right from the
> start.
> 
But unlike geneve i have placed this definition in ip_tunnels.h &
ip6_tunnels.h which doesnt come under NET_IP_TUNNEL.Hence build 
will fail in cases where NET_UDP_TUNNEL is disabled
Kbuild robot has shown that in v3.

Even with  #ifdef CONFIG_DST_CACHE Kbuild robot reported another issue.
when ip6_tunnel.h included in ip4_tunnel_core.c.
dst_cache_get_ipv6 comes under ipv6 flag  and hence the compilation of 
ip4_tunnel_core.c fails when IPV6 is disabled.

Ideally this functions should be defined in ip_tunnel.c & ip6_tunnel.c
as these function has no significance if IP Tunnel is disabled.


> Glad you found the previous reviews helpful. I will also miss a lot.
> For more assurance and also as regression test, it might be
> worth looking into adding a bareudp mode to
> tools/testing/selftests/net/pmtu.sh. That looks like it exercises a
> variety of tunnel types already. Extending it might be little work
> (once ip supports bareudp).
> 
> To be clear, not for this patch set. Let's not delay that further.
> Just a thought.