Message-ID: <CAOrHB_Drah9B5vqO9DvSRQUNs7X98AFy8uKxiK_vx5THXH0hPQ@mail.gmail.com>
Date:   Mon, 14 Dec 2020 20:00:03 -0800
From:   Pravin Shelar <pravin.ovn@...il.com>
To:     Jonas Bonn <jonas@...rbonn.se>
Cc:     Pravin B Shelar <pbshelar@...com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Pablo Neira Ayuso <pablo@...filter.org>, laforge@...monks.org
Subject: Re: [PATCH net-next v2] GTP: add support for flow based tunneling API

On Mon, Dec 14, 2020 at 12:29 AM Jonas Bonn <jonas@...rbonn.se> wrote:
>
> Hi Pravin,
>
> On 13/12/2020 20:32, Pravin Shelar wrote:
> > On Sat, Dec 12, 2020 at 11:56 PM Jonas Bonn <jonas@...rbonn.se> wrote:
> >>
> >> Hi Pravin,
> >>
> >> I've been thinking a bit about this and find it more and more
> >> interesting.  Could you post a bit of information about the ip-route
> >> changes you'll make in order to support GTP LWT encapsulation?  Could
> >> you provide an example command line?
> >>
> > This is done as part of the magma core project
> > (https://www.magmacore.org/) that needs OVS GTP support.
> > I have started with OVS integration first, there are unit tests that
> > validate the GTP support. This is datapath related test, that has the
> > setup commands:
> > https://github.com/pshelar/ovs/blob/6ec6a2a86adc56c7c9dcab7b3a7b70bb6dad35c9/tests/system-layer3-tunnels.at#L158
>
> That link just shows the classic setup using gtp-link and gtp-tunnel
> from libgtpnl.  It doesn't exercise LWT at all.
>
OVS add-port (ref ADD_OVS_TUNNEL) creates LWT tunnel.

> > Once OVS patches are upstream I can post patches for ip-route command.
>
> No, you should do it the other way around, please.  Post the ip-route
> changes along with this so we can see where this is going.
>
Currently we are using OVS for GTP tunnel. So I added support for OVS
first. You can see how it is used from OVS code. In the past when
adding support for other LWT tunnel protocols we have never followed
any ordering wrt OVS vs ip-route.
ip-route does not even support regular GTP tunnel devices. so this
should not be a blocker for this patch.

Anyways, I have a patch for iproute:
https://github.com/pshelar/iproute2/commit/d6e99f8342672e6e9ce0b71e153296f8e2b41cfc

> >>> +#include <net/dst_metadata.h>
> >>>    #include <net/net_namespace.h>
> >>>    #include <net/protocol.h>
> >>>    #include <net/ip.h>
> >>> @@ -73,6 +74,9 @@ struct gtp_dev {
> >>>        unsigned int            hash_size;
> >>>        struct hlist_head       *tid_hash;
> >>>        struct hlist_head       *addr_hash;
> >>> +     /* Used by flow based tunnel. */
> >>> +     bool                    collect_md;
> >>> +     struct socket           *collect_md_sock;
> >>
> >> I'm not convinced that you need to special-case LWT in this way.  It
> >> should be possible to just use the regular sk1u socket.  I know that the
> >> sk1u socket is created in userspace and might be set up to listen on the
> >> wrong address, but that's a user error if they try to use that device
> >> with LWT.  You could easily make the sk1u socket an optional parameter
> >> and create it (as you do in your patch) if it's not provided.  Then
> >> ip_tunnel_collect_metadata() would tell you whether to get the
> >> encapsulaton details from the tunnel itself or whether to look up a PDP
> >> context.  That should suffice, right?
> >>
> > Sounds good. I have added it as part of v3.
> > Just to be clear, I still need collect_md_sock to keep reference to
> > the socket that is created as part of the newlink in kernel space.
>
> Why?  I don't see that there's anything special enough about that socket
> that you can't just use it as sk1u.  You might need to massage the types
> a bit, but that doesn't seem like a big problem.  What am I missing?
>

If you look at the code you can see I do use sk1u for all datapath
processing, collect_md_sock reference to socket object is only kept
for destroying the socket object.

Powered by blists - more mailing lists