RE: question about eth and vlan item in flow pattern

[Dariusz Sosnowski <dsosnowski@nvidia.com>]

> -----Original Message-----
> From: Ivan Malov <ivan.malov@arknetworks.am>
> Sent: Tuesday, April 23, 2024 13:44
> To: Jie Hai <haijie1@huawei.com>
> Cc: Yuying Zhang <yuying.zhang@intel.com>; Aman Singh
> <aman.deep.singh@intel.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; ferruh.yigit@amd.com >> Ferruh Yigit
> <ferruh.yigit@amd.com>; andrew.rybchenko@oktetlabs.ru;
> dekelp@nvidia.com; psatheesh@marvell.com; NBU-Contact-Adrien Mazarguil
> (EXTERNAL) <adrien.mazarguil@6wind.com>; fengchengwen
> <fengchengwen@huawei.com>; Huisong Li <lihuisong@huawei.com>;
> Dengdui Huang <huangdengdui@huawei.com>; dev@dpdk.org; Ori Kam
> <orika@nvidia.com>
> Subject: Re: question about eth and vlan item in flow pattern
> 
> Hi,
> 
> Please see below.
> 
> On Tue, 23 Apr 2024, Jie Hai wrote:
> 
> > Hi, Ivan Malov,
> > Sorry for the late reply.
> > On 2024/4/10 21:19, Ivan Malov wrote:
> >> Hi Jie,
> >>
> >> Consider the following examples:
> >> 1) flow create 0 ingress pattern eth / ipv4 proto is 17 / udp / end \
> >>     actions queue index 1 / end
> >> 2) flow create 0 ingress pattern eth / ipv4 / udp / end \
> >>     actions queue index 1 / end
> >>
> >> Generally speaking, these two rules might be equivalent, with "proto is 17"
> >> in
> >> the first one probably being redundant (the very presence of "udp"
> >> item might stand for the same match criterion -- the protocol ID
> >> being 17).
> > I can understand that.
> Very well.
> 
> >>
> >> And I'd be tempted to treat the "has_vlan" case similarly. Consider:
> >> 3) flow create 0 ingress pattern eth has_vlan is 1 / vlan / end \
> >>     actions queue index 1 / end
> >> 4) flow create 0 ingress pattern eth / vlan / end \
> >>     actions queue index 1 / end
> >>
> >> (rule (3) is similar to "rule-0" from your example)
> >>
> >> Rules (3) and (4) seem equivalent to me -- the "has_vlan is 1" bit
> >> might be redundant in flow (3) -- the presence of item "vlan" sort of
> >> means the same.
> >
> >> Rule (3) and (4) should probably match both single-tagged and
> >> double-tagged packets because, in both cases, the pattern does not
> >> clarify which protocol follows the outermost VLAN tag. If one needs
> >> to exclude double-tag match, they should clarify the rule in one of the
> possible ways, in example:
> >>
> >> - pattern eth / vlan has_more_vlan is 0 / end
> >> - pattern eth / vlan tpid is 0x0800 / end
> >> - pattern eth / vlan / ipv4 / end
> >>
> >> (the "tpid" goes to the "hdr.eth_proto" of "struct rte_flow_item_vlan").
> >>
> > As mentioned above, `has_vlan is 1` and `vlan` are the same, which
> > means `vlan` stands for any-tagged vlan, Then, is the introduction of
> > `has_vlan` and `has_more_vlan` unnecessary?
> It's not clear why you believe it's unnecessary. Some vendors support boolean
> match on "is outer VLAN present" and "is inner VLAN present"
> in addition to exact match on TPID/TCI. It may be convenient to expose these
> match criteria as sub-fields in ETH and VLAN items.
> This should allow for faster parsing as one can drop item VLAN from pattern in
> case there's no intention to match on TPID/TCI.

To expand the argument on convenience by providing an example,
 `has_vlan` allows to construct a following logic:

1) flow create 0 group X ingress pattern eth has_vlan is 1 / end actions jump group Y / end
2) flow create 0 group X ingress pattern eth has_vlan is 0 / end actions jump group Z / end

3) flow create 0 group Y ingress pattern eth / vlan vid is N / end actions /* actions for tagged traffic */
4) flow create 0 group Z ingress pattern eth / end actions /* actions for non-tagged traffic */

Traffic can be split between different groups - one group for tagged and another for non-tagged packets.
`has_vlan` gives more flexibility in the API.

> >
> > There are drivers seeing `vlan` an one vlan layer and match 'eth/vlan/end'
> > with single-tagged packets, e.g.bnxt, hns3, etc.
> > I think this meaning makes it easier to distinguish the functions of the two.
> >
> > The `has_more_vlan is 1` is defined and used by some drivers, like mlx5.
> > While the `has_more_vlan is 0` is not defined.
> What do you mean by saying "not defined"? Not defined where?
> Having "has_more_vlan is 0" is perfectly legit, isn't it?
> 
> > Here you give the meaning as `having no vlan after this vlan`.
> > Then, What should the follow rule mean? single-tagged or double tagged?
> >
> > - pattern eth / vlan tpid is 0x8100 has_more_vlan is 0 / end
> Item VLAN here is supposed to describe the header _immediately_ following
> the ETH one, isn't it? If so, the rule means single-tagged. Doesn't it?

I also think this rule means single-tagged, but on the other hand I think it's also ill-defined.
tpid field in VLAN item defines the inner packet type,
so this pattern should match the following packet:

dmac=<any>
smac=<any>
ether_type=0x8100  // match single-tagged packet 
pcp=<any>
vid=<any>
ether_type=0x8100  // from tpid VLAN item 

Which is an incorrect packet.

> Thank you.
> 
> >
> >> With regard to the question about VLAN ID match, "pattern eth / vlan
> >> vid is 10", as well as "pattern eth has_vlan is 1 / vlan vid is 10",
> >> should probably match only on the outermost tag ID, -- that is, not
> >> on the inner one and not on both.
> > I agree with it.
> >> That is because the "vid is 10" criterion relates to the specific
> >> header, as per the match item it sits in. If the user wants to match
> >> on the inner VLAN ID (say, 11), they should clarify specify it as follows:
> >>
> >> - pattern eth / vlan vid is 10 / vlan vid is 11 / end
> >> - pattern eth / vlan / vlan vid is 11 / end
> >>
> >
> >> Hopefully, the rest of DPDK vendor community can correct me in case I
> >> got that wrong. Should you have more questions, please feel free to ask
> such.
> >>
> >> Thank you.
> >>
> >> On Wed, 10 Apr 2024, Jie Hai wrote:
> >>
> >>> Hi, all,
> >>>
> >>> I have some questions about the sub-options for ``VLAN`` and ``ETH``
> item.
> >>>
> >>> According to the documentation, ``has_vlan`` is sub-option of
> >>> ``ETH`` item and it means that the pattern contains at least one vlan.
> >>> The ``VLAN`` item is used to match tagged packets and have some
> >>> sub-options such as ``vid``, ``tci``, etc.
> >>>
> >>> If we combine them, what should the effect be?
> >>> For instance,
> >>>
> >>> rule-0: flow create 0 ingress pattern  eth  has_vlan is 1 / vlan  /
> >>> end actions queue index 2 / end
> >>> rule-1: flow create 0 ingress pattern  eth  has_vlan is 1 / vlan vid
> >>> is 10 / end actions queue index 2 / end
> >>>
> >>> For rule-0, should it match single-tagged packets only or
> >>> multi-tagged only or both?
> >>> That is to say, which one will take effect, `has_vlan is 1`  or
> >>> `vlan` or both?
> >>>
> >>> For rule-2, which packets should it match, with inner VLAN id 10, or
> >>> outer VLAN id 10, or both 10?
> >>>
> >>> The hns3 driver supports only the exact matching of VLAN numer.
> >>> And it is planned to adapt ``has_vlan`` and ``has_more_vlan`` to the
> >>> meaning of one VLAN for hns3 driver. Therefore, if the preceding
> >>> combinations are supported, we need to confirm the exact meanings.
> >>>
> >>> So, what are your views on the above question?
> >>>
> >>>
> >>>
> >>>
> >> .
> >