Re: [PATCH v3 1/3] ethdev: support metadata as flow rule criteria

[Yongseok Koh <yskoh@mellanox.com>]

> On Oct 5, 2018, at 6:39 AM, Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> 
> On 10/5/18 4:31 PM, Ferruh Yigit wrote:
>> On 9/27/2018 2:57 PM, Dekel Peled wrote:
>>> As described in [1], a new rte_flow item is added to support metadata
>>> to use as flow rule match pattern.
>>> The metadata is an opaque item, fully controlled by the application.
>>> 
>>> The use of metadata is relevant for egress rules only.
>>> It can be set in the flow rule using the RTE_FLOW_ITEM_META.
>>> 
>>> In order to avoid change in mbuf API, exisitng field buf.hash.fdir.hi
>>> is used to carry the metadata item. This field is used only in
>>> ingress packets, so using it for egress metadata will not cause
>>> conflicts.
>>> 
>>> Application should set the packet metadata in the mbuf dedicated field,
>>> and set the PKT_TX_METADATA flag in the mbuf->ol_flags.
>>> The NIC will use the packet metadata as match criteria for relevant
>>> flow rules.
>>> 
>>> This patch introduces metadata item type for rte_flow RTE_FLOW_ITEM_META,
>>> along with corresponding struct rte_flow_item_meta and ol_flag
>>> PKT_TX_METADATA.
>>> 
>>> [1] "[RFC,v2] ethdev: support metadata as flow rule criteria"
>>>     https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmails.dpdk.org%2Farchives%2Fdev%2F2018-August%2F110194.html&amp;data=02%7C01%7Cyskoh%40mellanox.com%7Cefd5b454b5ac4c84947108d62ac82a8c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636743436575292609&amp;sdata=2WbhDkF89hUV16zER4sfhvD5qDjw6geFQqE0kJQgdyM%3D&amp;reserved=0
>>> 
>>> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
>> <...>
>> 
>>> @@ -526,6 +532,12 @@ struct rte_mbuf {
>>>  			uint32_t hi;
>>>  			/**< First 4 flexible bytes or FD ID, dependent on
>>>  			     PKT_RX_FDIR_* flag in ol_flags. */
>>> +			/**
>>> +			 * Above member has optional use on egress:
>>> +			 * Application specific metadata value
>>> +			 * for flow rule match.
>>> +			 * Valid if PKT_TX_METADATA is set.
>>> +			 */
>>>  		} fdir;           /**< Filter identifier if FDIR enabled */
>> Any objection/comment to use hash.fdir.hi for this new "metadata" meaning? Olivier?
> 
> As for me, I'd prefer to see dedicated union member something like
> it was suggested in [1].
> 
> Andrew.
> 
> [1] https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmails.dpdk.org%2Farchives%2Fdev%2F2018-September%2F111954.html&amp;data=02%7C01%7Cyskoh%40mellanox.com%7Cefd5b454b5ac4c84947108d62ac82a8c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636743436575292609&amp;sdata=QbrBkMWIWVMlKb8839FG3U72VfLhRo%2BjGSHkTXT8ocQ%3D&amp;reserved=0

FYI, I also mentioned that when I reviewed mlx5 part of this patchset and Dekel acked.
He'll make the change and submit a new version once he's back to the office.

Thanks,
Yongseok