Re: [dpdk-dev] [PATCH] ethdev: add L2TPv3 header to flow API

[Ori Kam <orika@mellanox.com>]

Hi Rory,

PSB

> -----Original Message-----
> From: Sexton, Rory <rory.sexton@intel.com>
> Sent: Wednesday, December 11, 2019 1:36 PM
> To: Ori Kam <orika@mellanox.com>; dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> Adrien Mazarguil <adrien.mazarguil@6wind.com>; Jagus, DariuszX
> <dariuszx.jagus@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] ethdev: add L2TPv3 header to flow API
> 
> Hi Ori,
> 
> See my comments below.
> 
> Regards,
> Rory
> 
> >>
> >>> One general question why do we want to support only v3 and not also
> v2?
> >> l2tpv3 is more widely used as a tunneling protocol hence it's inclusion.
> >> A specific example is the cable industry where DOCSIS cable traffic is
> >> encapsulated using depi and uepi protocols which both make use of
> >> l2tpv3 session ids.
> >> Directing flows based on l2tpv3 can be very useful in these cases.
> >>
> >
> >I'm not saying that v3 is not used more, I just thought from looking at the
> spec that both can be supported and the only difference is the version, so
> matching on the version we will be able to support both versions.
> >Your decision.
> >
> 
> There is more difference between l2tpv2 and l2tpv3 than just the version
> number.
> In L2TPv2 there exists a 16 bit Tunnel ID and 16 bit Session ID. This is changed
> to a 32 bit Session ID in L2TPv3
> Please see difference in headers below for v2 and v3.
> Due to the differences between v2 and v3 I don't think both can be
> supported with same flow item.
> 
>                                                           L2TPv2
>     0...............................................15, 16......................................31
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |T|L|x|x|S|x|O|P|x|x|x|x|  Ver  |          Length (opt)                  |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |           Tunnel ID                               |           Session ID                     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |             Ns (opt)                               |             Nr (opt)                        |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |      Offset Size (opt)                        |    Offset pad... (opt)               |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
>                                                           L2TPv3
>     0...............................................15, 16......................................31
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                           Session ID                                                                  |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |               Cookie (optional, maximum 64 bits)...
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>                                                                                                                     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 

Since we have the version, they can by using union,
But like I said your call.

> >> >> +/**
> >> >> + * RTE_FLOW_ITEM_TYPE_L2TPV3.
> >> >> + *
> >> >> + * Matches a L2TPv3 header.
> >> >> + */
> >> >> +struct rte_flow_item_l2tpv3 {
> >> >> +	rte_be32_t session_id; /**< Session ID. */ };
> >> >
> >> >Does it make sense that in future we will want to match on the T / L
> >> >/ ver /
> >> Ns / Nr?
> >> >
> >> >Please also consider that any change will be ABI / API breakage,
> >> >which will
> >> be allowed only next year.
> >> >
> >>
> >> I don't foresee us wanting to match on any of the other fields, T / L
> >> / ver / Ns/ Nr.
> >> The session id would typically be the only field of interest in the
> >> l2tpv3 header.
> >
> > I think that adding all fields to the structure will solve the possible issue
> with adding matching later.
> > Also and even more important you will be able to use it for creating the
> raw_encap / raw_decap buffers.
> >
> >What do you think?
> 
> Based on the differences between v2 and v3 the only field of interest in
> l2tpv3 over IP is the Session ID.
> I agree it would make sense to add all fields if we were implementing l2tpv2
> however v2 would require a different implementation to v3 due to the
> differences between both Protocols.

Even if we just support v3, I think that it is a good idea to add all fields of v3.
This will allow the use of the raw_encap / raw_decap actions.
Please also note that you didn't add the new item to  cmd_set_raw_parsed function.
this function is used in order to create raw_encap/ raw_decap encapsulations.

Best,
Ori