[ovs-dev] [PATCH v1 1/2] userspace datapath: Add GTP-U tunnel support

Joe Stringer joe at ovn.org
Mon Mar 5 03:40:49 UTC 2018


On 4 March 2018 at 18:48, Yang, Yi <yi.y.yang at intel.com> wrote:
> On Mon, Mar 05, 2018 at 04:19:58AM +0800, Joe Stringer wrote:
>> ?On 2 March 2018 at 04:32, Yi Yang <yi.y.yang at intel.com> wrote:
>> >
>> > Signed-off-by: Feng Yang <feng.yang at intel.com>
>> > Signed-off-by: Yi Yang <yi.y.yang at intel.com>
>> > ---
>>
>> Hi Yi, thanks for this work. Some minor comments below, but this is
>> just a "drive-by" review. I hope that others with more experience with
>> GTP will take a look as well.
>>
>> I see the comment that there is locking on the device whenever a GTP
>> header is built, I assume you intend to address that in a followup
>> iteration.
>
> Joe, thank for your comments, can you elaborate "there is locking on the
> device whenever a GTP  header is built" with more details? I don't know
> this, or there is any existing document for this? We'll fix it in next
> iteration.

See the comment " /* XXX: RCUfy tnl_cfg. */ " and related locking in the patch.

>>
>> If you haven't already, it may be interesting to sync with Jiannan
>> (CC'd) who has previously done some work on GTP support in OVS for
>> kernel, to see whether the API (OpenFlow, OVSDB) makes sense for a
>> cross-datapath (ie kernel AND userspace) implementation perspective.
>
> Yes, sync is necessary, we contacted Jiannan before, I'll ping him to
> check the status of his kernel patch, we'll make sure they can work well
> together.

Awesome.

>> > +    }
>> > +    tnl->gtpu_msgtype = gtph->msgtype;
>> > +    tnl->tun_id = htonll(ntohl(get_16aligned_be32(&gtph->teid)));
>> > +
>> > +    /*Figure out whether the inner packet is IPv4, IPv6 or a GTP-U message.*/
>> > +    if (tnl->gtpu_msgtype == GTPU_MSGTYPE_GPDU) {
>> > +        struct ip_header *ip;
>> > +
>> > +        ip = (struct ip_header *)(gtph + 1);
>> > +        if (IP_VER(ip->ip_ihl_ver) == 4) {
>> > +            packet->packet_type = htonl(PT_IPV4);
>> > +        } else if (IP_VER(ip->ip_ihl_ver) == 6) {
>> > +            packet->packet_type = htonl(PT_IPV6);
>> > +        } else {
>> > +            goto err;
>> > +        }
>> > +    } else {
>> > +        /* tnl->gtpu_msgtype can identify if it is GTP-U message,
>> > +         * miniflow_extract won't parse it if tnl->gtpu_msgtype
>> > +         * isn't equal to GTPU_MSGTYPE_GPDU.
>> > +         */
>> > +        packet->packet_type = htonl(PT_UNKNOWN);
>>
>> This is where the limits of my GTP understanding will show, I was
>> under the impression that GTP-U provides encapsulation for user
>> traffic, so would commonly encapsulate IPv4/IPv6 inside. Whereas for
>> GTP-C, it's signalling traffic so would not have such inner headers -
>> it's supposed to be handled by a GTP implementation. Why is it
>> important to be able to decapsulate GTP-C? Furthermore, what's
>> ensuring that we don't end up spitting inner GTP-C packet data onto
>> the wire without headers post-translation?
>>
> GTP-C is for control path, so I don't think it makes sense for OVS to
> handle this, in addition GTP-C used different UDP port from GTP-U's, so
> we must have different tunnel ports to handle them respectively.

Re: GTP-C in OVS, I'm inclined to agree that it doesn't make sense to
be in OVS. There's a lot of complexity there and existing FOSS
projects already attempt to support this, such as those under Osmocom.

The thing I was trying to highlight with the above comment is that
this function appears to be attempting to handle both GTP-U and GTP-C,
but if it doesn't make sense for OVS to handle it then I'm not sure
why this code attempts to do so. I don't think that expecting people
to just send the right kind of traffic on the designated ports (eg,
GTP-U on the GTP-U port) is enough, these paths need to be resilient
to unexpected input. Perhaps it's enough to restrict installation of
flows with pop GTP actions based on presence of the appropriate GTP
msgtype in the flow key.

> I don't know if Jiannan has implemented GTP-C support in kernel data
> path, we can add GTP-C support if it is really needed, but I think it is
> a nice-to-have thing.

I believe there is some kind of implementation already in the kernel,
but you'd have to take a look to see whether its design fits with what
you're trying to do.

Cheers,
Joe


More information about the dev mailing list