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

["Sexton, Rory" <rory.sexton@intel.com>]

Hi Ori,

Let me rework the patch to make it clearer that this is supporting new flow type for l2tpv3 over IP, rather than l2tpv2/v3 over UDP which is how you interpreted it.
Will take into account all your feedback. Please review v2 once I submit.

Regards,
Rory

-----Original Message-----
From: dev <dev-bounces@dpdk.org> On Behalf Of Sexton, Rory
Sent: Wednesday, December 11, 2019 4:31 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 comments below.

Regards,
Rory

> > > > > > +/**
> > > > > > + * 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.

I think the confusion here is based on the fact that there are 2 separate types of l2tpv3.
- l2tpv3 over UDP, which is very similar to l2tpv2 with only change being 16 bit Tunnel ID
  and 16 bit Session ID changing to 32 bit Session ID. All other fields remain (T/L/Ver/Ns/Nr).
- l2tpv3 over IP is another type of l2tpv3 that is carried over IP rather than UDP and as such
  the message format is entirely different and consists of just a 32 bit Session ID. The other
  fields mentioned above do not exist at all in this l2tpv3 header.

This patch was targeted at creating a flow to handle l2tpv3 over IP exclusively. This is why the Session ID is the only field in the flow item.

I can add the item to cmd_set_raw_parsed function.