Re: [dpdk-dev] [PATCH] ethdev: add flow tag

[Yongseok Koh <yskoh@mellanox.com>]

> On Jul 5, 2019, at 6:54 AM, Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> 
> On Thu, Jul 04, 2019 at 04:23:02PM -0700, Yongseok Koh wrote:
>> A tag is a transient data which can be used during flow match. This can be
>> used to store match result from a previous table so that the same pattern
>> need not be matched again on the next table. Even if outer header is
>> decapsulated on the previous match, the match result can be kept.
>> 
>> Some device expose internal registers of its flow processing pipeline and
>> those registers are quite useful for stateful connection tracking as it
>> keeps status of flow matching. Multiple tags are supported by specifying
>> index.
>> 
>> Example testpmd commands are:
>> 
>>  flow create 0 ingress pattern ... / end
>>    actions set_tag index 2 value 0xaa00bb mask 0xffff00ff /
>>            set_tag index 3 value 0x123456 mask 0xffffff /
>>            vxlan_decap / jump group 1 / end
>> 
>>  flow create 0 ingress pattern ... / end
>>    actions set_tag index 2 value 0xcc00 mask 0xff00 /
>>            set_tag index 3 value 0x123456 mask 0xffffff /
>>            vxlan_decap / jump group 1 / end
>> 
>>  flow create 0 ingress group 1
>>    pattern tag index is 2 value spec 0xaa00bb value mask 0xffff00ff /
>>            eth ... / end
>>    actions ... jump group 2 / end
>> 
>>  flow create 0 ingress group 1
>>    pattern tag index is 2 value spec 0xcc00 value mask 0xff00 /
>>            tag index is 3 value spec 0x123456 value mask 0xffffff /
>>            eth ... / end
>>    actions ... / end
>> 
>>  flow create 0 ingress group 2
>>    pattern tag index is 3 value spec 0x123456 value mask 0xffffff /
>>            eth ... / end
>>    actions ... / end
>> 
>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> 
> Hi Yongseok,
> 
> Only high level questions for now, while it unquestionably looks useful,
> from a user standpoint exposing the separate index seems redundant and not
> necessarily convenient. Using the following example to illustrate:
> 
> actions set_tag index 3 value 0x123456 mask 0xfffff
> 
> pattern tag index is 3 value spec 0x123456 value mask 0xffffff
> 
> I might be missing something, but why isn't this enough:
> 
> pattern tag index is 3 # match whatever is stored at index 3
> 
> Assuming it can work, then why bother with providing value spec/mask on
> set_tag? A flow rule pattern matches something, sets some arbitrary tag to
> be matched by a subsequent flow rule and that's it. It even seems like
> relying on the index only on both occasions is enough for identification.
> 
> Same question for the opposite approach; relying on the value, never
> mentioning the index.
> 
> I'm under the impression that the index is a hardware-specific constraint
> that shouldn't be exposed (especially since it's an 8-bit field). If so, a
> PMD could keep track of used indices without having them exposed through the
> public API.


Thank you for review, Adrien.
Hope you are doing well. It's been long since we talked each other. :-)

Your approach will work too in general but we have a request from customer that
they want to partition this limited tag storage. Assuming that HW exposes 32bit
tags (those are 'registers' in HW pipeline in mlx5 HW). Then, customers want to
store multiple data even in a 32-bit storage. For example, 16bit vlan tag, 8bit
table id and 8bit flow id. As they want to split one 32bit storage, I thought it
is better to provide mask when setting/matching the value. Even some customer
wants to store multiple flags bit by bit like ol_flags. They do want to alter
only partial bits.

And for the index, it is to reference an entry of tags array as HW can provide
larger registers than 32-bit. For example, mlx5 HW would provide 4 of 32b
storage which users can use for their own sake.
	tag[0], tag[1], tag[2], tag[3]


Thanks,
Yongseok