Re: [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item

[Maxime Leroy <maxime.leroy@6wind.com>]

Hi Dekel,

First, I don't understand the initial change [1] done on RTE_FLOW API.
Before this change, it was possible to match any packets with or
without vlan encapsulations.
At least, it's not anymore the case with the mlx5 pmd.

For example, if I want to match any ssh packets whatever if it's
encapsulated with no vlan or N vlan headers:
testpmd> flow create 0 ingress  pattern  eth type spec 0 type mask 0 /
ipv4 / udp dst is 22  / end actions  mark id 2 / queue index 0 / end

By setting the ethernet type mask to 0x0, it means that ethernet type
should be ignored. It means if ethernet type is 0x800 (i.e. ipv4) or
0x8100 (i.e. vlan) or 0x88A8 (qinq), the packet should be matched.

But if you wanted to match only ethernet packets (and not vlan/qinq
one), you can create the following flows:
testpmd> flow create 0 ingress  pattern  eth type spec 0x800 type mask
0xffff / ipv4 / udp dst is 22  / end actions  mark id 2 / queue index
0 / end

With your new RFC, first I don't understand the needs of the num_of_vlans field.

You can create the following follow if you want to match any qinq /
ipv4 packets (i.e. 2 vlan level) for example:
> flow create 0 ingress  pattern  eth type spec 0x88A8 type mask 0xffff / vlan inner_type spec is 0x8100 mask is 0xFFFF / vlan  / end actions  mark id 2 / queue index 0 / end

Could you explain to me the utility of this new field ?

The cvlan_exist, and svlan_exist seems useless for me. For me, you can
already do the same thing with type field. Because, by setting the
type mask to 0, you can already give the notion of any ethertype.

Let's take some example:

1. with svlan_exists=0, cvlan_exists=0, it can already be configured like that:
> flow  create 0 ingress  pattern  eth type spec 0x800 type mask 0xFFFF / ipv4 / end actions  mark id 2 / queue index 0 / end

2. With svlan_exists=0, cvlan_exists=1:
> flow  create 0 ingress  pattern eth type spec 0x8100 type mask 0xFFFF / vlan inner_type is 0x800 mask is 0xFFFF / ipv4 / end actions  mark id 2 / queue index 0 / end

3. With svlan_exists=1, cvlan_exists=0: it doesn't seem to be a real
use case. Anyway, you could have:
> flow create 0 ingress  pattern  eth type spec 0x88A8 type mask 0xffff / vlan inner_type spec is 0x800 mask is 0xFFFF / ipv4  / end actions  mark id 2 / queue index 0 / end

4. With svlan_exists=1, cvlan_exists=1:
 > flow create 0 ingress  pattern  eth type spec 0x88A8 type mask
0xffff / vlan inner_type spec is 0x8100 mask is 0xFFFF / vlan spec is
0x800 mask 0xFFFF / ipv4 / end actions  mark id 2 / queue index 0 /
end

Could you please explain to me what you try to achieve with this RFC ?

I would like to know why ether_type value setted by the user is
ignored when I create the following rule:
testpmd> flow create 0 ingress  pattern  eth type spec 0 type mask 0 /
ipv4 / udp dst is 22  / end actions  mark id 2 / queue index 0 / end
with the mlx5 pmd ? (i.e. why this change [1].)

[1] http://mails.dpdk.org/archives/dev/2020-May/166257.html

Best Regards,

Maxime

On Wed, Aug 5, 2020 at 9:04 AM Matan Azrad <matan@mellanox.com> wrote:
>
> But if the user want to force only one vlan and don't care about others?
>
>
> > -----Original Message-----
> > From: Dekel Peled <dekelp@mellanox.com>
> > Sent: Wednesday, August 5, 2020 9:54 AM
> > To: Eli Britstein <elibr@mellanox.com>; ferruh.yigit@intel.com;
> > arybchenko@solarflare.com; Ori Kam <orika@mellanox.com>; Thomas
> > Monjalon <thomas@monjalon.net>
> > Cc: Asaf Penso <asafp@mellanox.com>; Matan Azrad
> > <matan@mellanox.com>; dev@dpdk.org
> > Subject: RE: [RFC] ethdev: add VLAN attributes to ETH item
> >
> > Thanks, PSB.
> >
> > > -----Original Message-----
> > > From: Eli Britstein <elibr@mellanox.com>
> > > Sent: Tuesday, August 4, 2020 6:47 PM
> > > To: Dekel Peled <dekelp@mellanox.com>; ferruh.yigit@intel.com;
> > > arybchenko@solarflare.com; Ori Kam <orika@mellanox.com>; Thomas
> > > Monjalon <thomas@monjalon.net>
> > > Cc: Asaf Penso <asafp@mellanox.com>; Matan Azrad
> > <matan@mellanox.com>;
> > > dev@dpdk.org
> > > Subject: Re: [RFC] ethdev: add VLAN attributes to ETH item
> > >
> > >
> > > On 8/4/2020 6:36 PM, Dekel Peled wrote:
> > > > In existing code the match on tagged/untagged packets is not explicit.
> > > > Recent documentation update [1] describes the different patterns and
> > > > clarifies the intended use of different patterns.
> > > >
> > > > This patch proposes an update to ETH item struct, to clearly define
> > > > the required characteristic of a packet, and enable precise match criteria.
> > > >
> > > > [1] http://mails.dpdk.org/archives/dev/2020-May/166257.html
> > > >
> > > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > > > ---
> > > >   lib/librte_ethdev/rte_flow.h | 9 +++++++++
> > > >   1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > > b/lib/librte_ethdev/rte_flow.h index cf0eccb..345feb5 100644
> > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > @@ -726,11 +726,20 @@ struct rte_flow_item_raw {
> > > >    * If the @p ETH item is the only item in the pattern, and the @p type
> > field
> > > >    * is not specified, then both tagged and untagged packets will match
> > the
> > > >    * pattern.
> > > > + * The fields @p cvlan_exist and @p svlan_exist can be used to
> > > > + match specific
> > > > + * packet types, instead of using the @p type field. This can be
> > > > + used to match
> > > > + * on packets that do/don't contain either cvlan, svlan, or both.
> > > > + * The field @p num_of_vlans can be used to match packets by the
> > > > + exact number
> > > > + * of VLANs in header.
> > > >    */
> > > >   struct rte_flow_item_eth {
> > > >           struct rte_ether_addr dst; /**< Destination MAC. */
> > > >           struct rte_ether_addr src; /**< Source MAC. */
> > > >           rte_be16_t type; /**< EtherType or TPID. */
> > > > + uint32_t cvlan_exist:1; /**< C-tag VLAN exist in header. */
> > > > + uint32_t svlan_exist:1; /**< S-tag VLAN exist in header. */
> > > > + uint32_t reserved:14; /**< Reserved, must be zero. */
> > > > + uint32_t num_of_vlans:16; /**< Number of VLANs in header. */
> > > We can deduct from num_of_vlans the values of cvlan_exist/svlan_exist,
> > > so those are redundant fields. Keeping them introduce a conflicting
> > > match. For example num_of_vlans=0 and cvlan_exist=1.
> >
> > Such conflict is simple to validate and reject.
> > Even if num_of_vlans is removed, we can still get conflict svlan_exist=1,
> > cvlan_exist=0.
> > The different fields are proposed to allow flexible match on different VLAN
> > attributes.
> > Every PMD can choose to support any or none of them.
> >
> > > >   };
> > > >
> > > >   /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */