Re: [dpdk-dev] [RFC] ethdev: add fragment attribute to IPv6 item

[Dekel Peled <dekelp@mellanox.com>]

Hi,

Kind reminder, please respond on the recent correspondence so we can conclude this issue.

Regards,
Dekel

> -----Original Message-----
> From: Dekel Peled <dekelp@mellanox.com>
> Sent: Wednesday, June 3, 2020 3:11 PM
> To: Ori Kam <orika@mellanox.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>;
> ferruh.yigit@intel.com; john.mcnamara@intel.com;
> marko.kovacevic@intel.com; Asaf Penso <asafp@mellanox.com>; Matan
> Azrad <matan@mellanox.com>; Eli Britstein <elibr@mellanox.com>;
> dev@dpdk.org; Ivan Malov <Ivan.Malov@oktetlabs.ru>
> Subject: RE: [RFC] ethdev: add fragment attribute to IPv6 item
> 
> Hi, PSB.
> 
> > -----Original Message-----
> > From: Ori Kam <orika@mellanox.com>
> > Sent: Wednesday, June 3, 2020 11:16 AM
> > To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Cc: Andrew Rybchenko <arybchenko@solarflare.com>; Dekel Peled
> > <dekelp@mellanox.com>; ferruh.yigit@intel.com;
> > john.mcnamara@intel.com; marko.kovacevic@intel.com; Asaf Penso
> > <asafp@mellanox.com>; Matan Azrad <matan@mellanox.com>; Eli
> Britstein
> > <elibr@mellanox.com>; dev@dpdk.org; Ivan Malov
> > <Ivan.Malov@oktetlabs.ru>
> > Subject: RE: [RFC] ethdev: add fragment attribute to IPv6 item
> >
> > Hi Adrien,
> >
> > Great to hear from you again.
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Sent: Tuesday, June 2, 2020 10:04 PM
> > > To: Ori Kam <orika@mellanox.com>
> > > Cc: Andrew Rybchenko <arybchenko@solarflare.com>; Dekel Peled
> > > <dekelp@mellanox.com>; ferruh.yigit@intel.com;
> > > john.mcnamara@intel.com; marko.kovacevic@intel.com; Asaf Penso
> > > <asafp@mellanox.com>; Matan Azrad <matan@mellanox.com>; Eli
> > Britstein
> > > <elibr@mellanox.com>; dev@dpdk.org; Ivan Malov
> > > <Ivan.Malov@oktetlabs.ru>
> > > Subject: Re: [RFC] ethdev: add fragment attribute to IPv6 item
> > >
> > > Hi Ori, Andrew, Delek,
> 
> It's Dekel, not Delek ;-)
> 
> > >
> > > (been a while eh?)
> > >
> > > On Tue, Jun 02, 2020 at 06:28:41PM +0000, Ori Kam wrote:
> > > > Hi Andrew,
> > > >
> > > > PSB,
> > > [...]
> > > > > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > > > > b/lib/librte_ethdev/rte_flow.h index b0e4199..3bc8ce1 100644
> > > > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > > > @@ -787,6 +787,8 @@ struct rte_flow_item_ipv4 {
> > > > > >   */
> > > > > >  struct rte_flow_item_ipv6 {
> > > > > >  	struct rte_ipv6_hdr hdr; /**< IPv6 header definition. */
> > > > > > +	uint32_t is_frag:1; /**< Is IPv6 packet fragmented/non-
> > fragmented. */
> > > > > > +	uint32_t reserved:31; /**< Reserved, must be zero. */
> > > > >
> > > > > The solution is simple, but hardly generic and adds an example
> > > > > for the future extensions. I doubt that it is a right way to go.
> > > > >
> > > > I agree with you that this is not the most generic way possible,
> > > > but the IPV6 extensions are very unique. So the solution is also unique.
> > > > In general, I'm always in favor of finding the most generic way,
> > > > but
> > > sometimes
> > > > it is better to keep things simple, and see how it goes.
> > >
> > > Same feeling here, it doesn't look right.
> > >
> > > > > May be we should add 256-bit string with one bit for each IP
> > > > > protocol number and apply it to extension headers only?
> > > > > If bit A is set in the mask:
> > > > >  - if bit A is set in spec as well, extension header with
> > > > >    IP protocol (1 << A) number must present
> > > > >  - if bit A is clear in spec, extension header with
> > > > >    IP protocol (1 << A) number must absent If bit is clear in
> > > > > the mask, corresponding extension header may present and may
> > > > > absent (i.e. don't care).
> > > > >
> > > > There are only 12 possible extension headers and currently none of
> > > > them are supported in rte_flow. So adding a logic to parse the 256
> > > > just to get a max
> > > of 12
> > > > possible values is an overkill. Also, if we disregard the case of
> > > > the extension, the application must select only one next proto.
> > > > For example, the application can't select udp + tcp. There is the
> > > > option to add a flag for each of the possible extensions, does it
> > > > makes more
> > sense to you?
> > >
> > > Each of these extension headers has its own structure, we first need
> > > the ability to match them properly by adding the necessary pattern items.
> > >
> > > > > The RFC indirectly touches IPv6 proto (next header) matching
> > > > > logic.
> > > > >
> > > > > If logic used in ETH+VLAN is applied on IPv6 as well, it would
> > > > > make pattern specification and handling complicated. E.g.:
> > > > >   eth / ipv6 / udp / end
> > > > > should match UDP over IPv6 without any extension headers only.
> > > > >
> > > > The issue with VLAN I agree is different since by definition VLAN
> > > > is layer 2.5. We can add the same logic also to the VLAN case,
> > > > maybe it will be easier.
> > > > In any case, in your example above and according to the RFC we
> > > > will get all ipv6 udp traffic with and without extensions.
> > > >
> > > > > And how to specify UPD over IPv6 regardless extension headers?
> > > >
> > > > Please see above the rule will be eth / ipv6 /udp.
> > > >
> > > > >   eth / ipv6 / ipv6_ext / udp / end with a convention that
> > > > > ipv6_ext is optional if spec and mask are NULL (or mask is empty).
> > > > >
> > > > I would guess that this flow should match all ipv6 that has one
> > > > ext and the
> > > next
> > > > proto is udp.
> > >
> > > In my opinion RTE_FLOW_ITEM_TYPE_IPV6_EXT is a bit useless on its
> own.
> > > It's only for matching packets that contain some kind of extension
> > > header, not a specific one, more about that below.
> > >
> > > > > I'm wondering if any driver treats it this way?
> > > > >
> > > > I'm not sure, we can support only the frag ext by default, but if
> > > > required we
> > > can support other
> > > > ext.
> > > >
> > > > > I agree that the problem really comes when we'd like match
> > > > > IPv6 frags or even worse not fragments.
> > > > >
> > > > > Two patterns for fragments:
> > > > >   eth / ipv6 (proto=FRAGMENT) / end
> > > > >   eth / ipv6 / ipv6_ext (next_hdr=FRAGMENT) / end
> > > > >
> > > > > Any sensible solution for not-fragments with any other extension
> > > > > headers?
> > > > >
> > > > The one propose in this mail 😊
> > > >
> > > > > INVERT exists, but hardly useful, since it simply says that
> > > > > patches which do not match pattern without INVERT matches the
> > > > > pattern with INVERT and
> > > > >   invert / eth / ipv6 (proto=FRAGMENT) / end will match ARP,
> > > > > IPv4,
> > > > > IPv6 with an extension header before fragment header and so on.
> > > > >
> > > > I agree with you, INVERT in this doesn’t help.
> > > > We were considering adding some kind of not mask / item per item.
> > > > some think around this line:
> > > > user request ipv6 unfragmented udp packets. The flow would look
> > > > something like this:
> > > > Eth / ipv6 / Not (Ipv6.proto = frag_proto) / udp But it makes the
> > > > rules much harder to use, and I don't think that there is any HW
> > > > that support not, and adding such feature to all items is overkill.
> > > >
> > > >
> > > > > Bit string suggested above will allow to match:
> > > > >  - UDP over IPv6 with any extension headers:
> > > > >     eth / ipv6 (ext_hdrs mask empty) / udp / end
> > > > >  - UDP over IPv6 without any extension headers:
> > > > >     eth / ipv6 (ext_hdrs mask full, spec empty) / udp / end
> > > > >  - UDP over IPv6 without fragment header:
> > > > >     eth / ipv6 (ext.spec & ~FRAGMENT, ext.mask | FRAGMENT) / udp
> > > > > / end
> > > > >  - UDP over IPv6 with fragment header
> > > > >     eth / ipv6 (ext.spec | FRAGMENT, ext.mask | FRAGMENT) / udp
> > > > > / end
> > > > >
> > > > > where FRAGMENT is 1 << IPPROTO_FRAGMENT.
> > > > >
> > > > Please see my response regarding this above.
> > > >
> > > > > Above I intentionally keep 'proto' unspecified in ipv6 since
> > > > > otherwise it would specify the next header after IPv6 header.
> > > > >
> > > > > Extension headers mask should be empty by default.
> > >
> > > This is a deliberate design choice/issue with rte_flow: an empty
> > > pattern matches everything; adding items only narrows the selection.
> > > As Andrew said there is currently no way to provide a specific item
> > > to reject, it can only be done globally on a pattern through INVERT
> > > that no
> > PMD implements so far.
> > >
> > > So we have two requirements here: the ability to specifically match
> > > IPv6 fragment headers and the ability to reject them.
> > >
> > > To match IPv6 fragment headers, we need a dedicated pattern item.
> > > The generic RTE_FLOW_ITEM_TYPE_IPV6_EXT is useless for that on its
> > > own, it must be completed with RTE_FLOW_ITEM_TYPE_IPV6_EXT_FRAG
> and
> > associated
> > > object
> >
> > Yes, we must add EXT_FRAG to be able to match on the FRAG bits.
> >
> 
> Please see previous RFC I sent.
> [RFC] ethdev: add IPv6 fragment extension header item
> http://mails.dpdk.org/archives/dev/2020-March/160255.html
> It is complemented by this RFC.
> 
> > > to match individual fields if needed (like all the others
> > > protocols/headers).
> > >
> > > Then to reject a pattern item... My preference goes to a new "NOT"
> > > meta item affecting the meaning of the item coming immediately after
> > > in the pattern list. That would be ultra generic, wouldn't break any
> > > ABI/API and like INVERT, wouldn't even require a new object
> > > associated
> > with it.
> > >
> > > To match UDPv6 traffic when there is no fragment header, one could
> > > then do something like:
> > >
> > >  eth / ipv6 / not / ipv6_ext_frag / udp
> > >
> > > PMD support would be trivial to implement (I'm sure!)
> > >
> > I agree with you as I said above. The issue is not PMD, the issues are:
> > 1. think about the rule you stated above from logic point there is
> > some contradiction, you are saying ipv6 next proto udp but you also
> > say not frag, this is logic only for IPV6 ext.
> > 2. HW issue, I don't know of HW that knows how to support not on an item.
> > So adding something for all items for only one case is overkill.
> >
> >
> >
> > > We may later implement other kinds of "operator" items as Andrew
> > > suggested, for bit-wise stuff and so on. Let's keep adding features
> > > on a needed basis though.
> > >
> > > --
> > > Adrien Mazarguil
> > > 6WIND
> >
> > Best,
> > Ori