Hi, Kind reminder, please respond on the recent correspondence so we can conclude this issue.
Regards, Dekel > -----Original Message----- > From: Dekel Peled <dek...@mellanox.com> > Sent: Wednesday, June 3, 2020 3:11 PM > To: Ori Kam <or...@mellanox.com>; Adrien Mazarguil > <adrien.mazarg...@6wind.com> > Cc: Andrew Rybchenko <arybche...@solarflare.com>; > ferruh.yi...@intel.com; john.mcnam...@intel.com; > marko.kovace...@intel.com; Asaf Penso <as...@mellanox.com>; Matan > Azrad <ma...@mellanox.com>; Eli Britstein <el...@mellanox.com>; > dev@dpdk.org; Ivan Malov <ivan.ma...@oktetlabs.ru> > Subject: RE: [RFC] ethdev: add fragment attribute to IPv6 item > > Hi, PSB. > > > -----Original Message----- > > From: Ori Kam <or...@mellanox.com> > > Sent: Wednesday, June 3, 2020 11:16 AM > > To: Adrien Mazarguil <adrien.mazarg...@6wind.com> > > Cc: Andrew Rybchenko <arybche...@solarflare.com>; Dekel Peled > > <dek...@mellanox.com>; ferruh.yi...@intel.com; > > john.mcnam...@intel.com; marko.kovace...@intel.com; Asaf Penso > > <as...@mellanox.com>; Matan Azrad <ma...@mellanox.com>; Eli > Britstein > > <el...@mellanox.com>; dev@dpdk.org; Ivan Malov > > <ivan.ma...@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.mazarg...@6wind.com> > > > Sent: Tuesday, June 2, 2020 10:04 PM > > > To: Ori Kam <or...@mellanox.com> > > > Cc: Andrew Rybchenko <arybche...@solarflare.com>; Dekel Peled > > > <dek...@mellanox.com>; ferruh.yi...@intel.com; > > > john.mcnam...@intel.com; marko.kovace...@intel.com; Asaf Penso > > > <as...@mellanox.com>; Matan Azrad <ma...@mellanox.com>; Eli > > Britstein > > > <el...@mellanox.com>; dev@dpdk.org; Ivan Malov > > > <ivan.ma...@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