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 Fri, Jan 24, 2020 at 03:47:37PM -0500, Willem de Bruijn wrote:
> On Fri, Jan 24, 2020 at 9:47 AM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > 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.
> 
> Sounds great. Yes, these functions are better in a .c file.

Keeping these functions in ipv4/route.c and ipv6/ip6_output.c along with ip_route_output_flow & ip6_dst_lookup_flow
These functions will be named ip_route_output_tunnel & ip6_dst_lookup_tunnel
 

Keeping these functions in ip_tunnels.c & ip6_tunnels.c is a bad idea as the bareudp module has no
Dependency with ipv4 & ipv6 tunnel module as of now and there is no need to create one.


Do you see any issues
If the above approach is not feasible we should fall back to ip_tunnel.h ip6_tunnel.h with proper #ifdef protection