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

[Ori Kam <orika@mellanox.com>]

Hi Rory,

> -----Original Message-----
> From: Sexton, Rory <rory.sexton@intel.com>
> Sent: Tuesday, December 10, 2019 4:52 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,
> 
> > 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.

> Some more comments inline below.
> 
> Rory
> 
> >> diff --git a/lib/librte_ethdev/rte_flow.h
> >> b/lib/librte_ethdev/rte_flow.h index 452d359a1..5ee055c28 100644
> >> --- a/lib/librte_ethdev/rte_flow.h
> >> +++ b/lib/librte_ethdev/rte_flow.h
> >> @@ -510,6 +510,16 @@ enum rte_flow_item_type {
> >>  	 * See struct rte_flow_item_tag.
> >>  	 */
> >>  	RTE_FLOW_ITEM_TYPE_TAG,
> >> +
> >> +	/*
> >
> >Missing *. It should be /**
> >
> 
> Will correct this in another version of this patch.
> 
> >> +/**
> >> + * 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?

Best,
Ori