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.

>
> 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?

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?
>>> 
>>> 
>>> 
>>> 
>> .
>