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

[Dekel Peled <dekelp@mellanox.com>]

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