Hi Rory,
PSB
> -----Original Message-----
> From: Sexton, Rory <rory.sex...@intel.com>
> Sent: Wednesday, December 11, 2019 1:36 PM
> To: Ori Kam <or...@mellanox.com>; dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zh...@intel.com>; Xing, Beilei <beilei.x...@intel.com>;
> Adrien Mazarguil <adrien.mazarg...@6wind.com>; Jagus, DariuszX
> <dariuszx.ja...@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
