Re: [dpdk-dev] [PATCH] net/tap: fix zeroed flow mask configurations

[Matan Azrad <matan@mellanox.com>]

Hi Adrien

From: Adrien Mazarguil
> On Sun, Aug 05, 2018 at 06:10:55AM +0000, Matan Azrad wrote:
> > Hi Adrien
> >
> > From: Adrien Mazarguil
> > > Hi Matan,
> > >
> > > On Thu, Aug 02, 2018 at 05:52:18PM +0000, Matan Azrad wrote:
> > > > Hi Adrien
> > > >
> > > > From: Adrien Mazarguil
> > > > > On Thu, Aug 02, 2018 at 10:33:00AM +0000, Matan Azrad wrote:
> > > > > > The rte_flow meaning of zero flow mask configuration is to
> > > > > > match all the range of the item value.
> > > > > > For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask
> > > > > > 0.0.0.0 should much all the ipv4 traffic from the rte_flow API
> perspective.
> > > > > >
> > > > > > From some kernel perspectives the above rule means to ignore
> > > > > > all the
> > > > > > ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).
> > > > > >
> > > > > > Due to the fact that the tap PMD should provide the rte_flow
> > > > > > meaning, it is necessary to ignore the spec in case the mask
> > > > > > is zero when it forwards such like flows to the kernel.
> > > > > > So, the above rule should be translated to eth / ipv4 to get
> > > > > > the correct meaning.
> > > > > >
> > > > > > Ignore spec configurations when the mask is zero.
> > > > >
> > > > > I would go further, one should be able to match IP address
> > > > > 0.0.0.0 for
> > > instance.
> > > > > The PMD should only trust the mask on all fields without looking at
> spec.
> > > >
> > > > The PMD should convert the RTE flow API to the device
> > > > configuration, So I can think on scenarios that the PMD should look on
> spec.
> > >
> > > Obviously the PMD needs to take spec into account. What I meant is
> > > that for each field, spec must be taken into account according to mask
> only.
> > >
> > > For any given field, when mask is empty, don't look at spec, it's like a
> wildcard.
> > > When mask is full, take spec as is, even if spec only contains zeroed bits.
> > >
> > > User intent in that case is to match a zero value exactly, so it
> > > must not result in a wildcard match. If supported, when mask is
> > > partial, masked bits are also matched exactly, even if these turn
> > > out to be a zero value. Unmasked bits are considered wildcards.
> > >
> >
> > Yes I understand your point Adrien, but I mean that maybe sometimes
> some spec values should be converted to another spec values to get the
> correct translation of rte_flow for a special device.
> >
> > Here, maybe IP_spec=0.0.0.0 is a special case that should be taken into
> account, so we must validate what's happen in Tap for this case to apply your
> suggestion too, Maybe there was some intentions for spec=0 cases from the
> current code author.
> 
> I understand that's a lot of maybes :)
> 
> I've checked the code and I'am sure it's a mistake made by the original
> author. See tap_flow_create_eth() for instance:
> 
>  if (!is_zero_ether_addr(&spec->dst)) {
> 
> Followed by:
> 
>  if (!is_zero_ether_addr(&mask->src))
> 
> This lack of consistency doesn't make any sense, it cannot be on purpose.
> 
> To my credentials I wrote a very similar code which uses TC flower in mlx5
> and relies on mask (only) in order to retrieve spec. Have a look at
> drivers/net/mlx5/mlx5_nl_flow.c. I validated that traffic where addresses
> were all zeroes could be successfully matched.
>

I will check the spec zero cases and will update.

Thanks Adrien!