Re: [PATCH] ethdev: introduce generic flow item and action

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 8/16/2023 3:20 PM, Morten Brørup wrote:
>> From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
>> Sent: Wednesday, 16 August 2023 15.23
>>
>> Hi Morten,
>>
>> <snip>
>>
>>>>>
>>>>> In order to avoid conflicts between P4 and non-P4 generic flow
>>>> items/actions,
>>>>> the generic type should include information about how to interpret the
>>>>> information, which is why I suggest making it a Vendor-Specific type,
>> with
>>>>> vendor-specific TLV's (managed by the vendor), like the RADIUS Vendor-
>>>>> Specific attributes I compared to, instead of just an opaque blob.
>>>>
>>>> I like this idea, but it is not necessary to introduce a vendor-specific
>> type,
>>>> it could be considered a device-specific type (or port-specific in the
>> context
>>>> of DPDK).
>>>>
>>>> However, the PMD can manage a dictionary, enabling users to query about
>>> the
>>>> format of each generic item or action if we can expose a set of query
>> APIs.
>>>>
>>>> But I guess we don't need vendor-id / vendor-type as RADIUS does, as we
>>> have
>>>> port_id here.
>>>
>>> If the flow item itself doesn't have a "type" field (identifying how to
>> interpret
>>> the blob), you might have two different NICs using each their own blob
>>> format. The NIC must be able to determine if a given flow item is of a type
>> it
>>> can understand, before it tries to parse the blob in it.
>>>
>>> This is why the "struct rte_flow_item" has a "type" field. It tells the HW
>> how
>>> to interpret the values in a flow item.
>>>
>>> If we introduce a "generic" flow item type, it can only be used for multiple
>>> purposes (i.e. both P4, but also other purposes than P4) if it has a "sub-
>> type"
>>> field. Otherwise, someone will create a "generic" flow item containing a P4
>>> program and send it to a NIC, which uses the "generic" flow item type for
>>> other program types, e.g. a cBPF program. And this cBPF capable NIC has no
>>> way to detect that the blob in the flow item is not a cBPF program, but a P4
>>> program. The P4 capable NIC will accept the P4 program, but will be confused
>>> when sent the cBPF program understood by the first NIC.
>>>
>>> So I am suggesting that the "generic" flow items and actions follow an
>> existing
>>> and well known design patterns for how to identify the meaning of blobs:
>>> Include a Vendor-ID followed by vendor-specific TLV formatted data.
>>>
>>> As I wrote initially, I am opposed to introducing uninterpretable blobs into
>>> DPDK. Flow items/actions need to be well defined. Allowing "Vendor-Specific"
>>> flow items/actions is a workaround that allows you to bypass the normal
>>> standardization process.
>>>
>>
>> I would be happy to add mechanisms to describe the user-defined flow items
>> and actions in greater detail. Would you be able to provide some examples for
>> your proposal for a flow item and a flow action of your choice, please?
>> Thanks!
>>
>> One thing I would want to stress here: the flow items and flow actions are
>> defined exclusively by the user (through their P4 program) without any
>> knowledge or intervention from the HW vendor, so any TLVs / helper fields
>> must be populated by the user as opposed to the HW vendor.
> 
> Perhaps I have completely misunderstood this patch...
> 
> I thought the purpose is for the user to define some generic flow items and actions, which are not in the list of DPDK standardized (and fully documented) RTE_FLOW items/actions, but are understood by a variety of programmable NICs from various HW vendors. In this case, each blob needs to be prefixed with a "type" field, so the HW can determine which of its processing engines needs to parse the blob. E.g. a NIC could have both a P4 processing engine and a BPF processing engine, so the blob needs to indicate which of the two engines to use for the provided flow item/action.
> 
> But maybe the purpose is completely different. Is the purpose of this patch to introduce flow items and flow actions, which each make the HW perform a "callback" to the user application? In this case, only the user application (handling the "callbacks") can understand them, and thus they are completely opaque to everything else.
> 

@Morten, I agree that this is more "opaque" than "generic" and it is
open to abuse.

As far as I understand, purpose is NOT to implement unsupported RTE_FLOW
items/actions, if that is the case I agree with @Ori to figure out gaps
and implement them.


Purpose seems to provide control path for P4 pipelines.

Each P4 pipeline implementation can have set of tables, used for match
action for the pipeline, and these tables can be for anything, doesn't
have to be standard net functions or protocols etc..

To be able to dynamically program the pipeline, someone needs the
ability to program the tables, since these tables not limited to net
functions it is not possible to address these tables by rte_flow patterns.
Hence this opaque rte_flow pattern/action seems designed to dynamically
update/control the pipeline.


If above understanding is correct, I suggest using a middle ground,
instead of having wide open key/value based rte_flow item, rename it to
RTE_FLOW_ITEM_TYPE_P4[SOMETHING],

and have "struct rte_flow_item_p4[something]" to include all relevant
fields to program a P4 pipeline, like table_id/table_name, value etc..

This way new rte_flow item/action remains scoped and focused, but still
flexible enough to program any type of P4 pipeline.


Also we don't need to maintain a unique vendor ID etc, although what is
the table name/id, what it is for and what is the relevant action is HW
(even pipeline) specific, there is nothing to collide per vendor to manage.
With this approach code can be portable, same application can be used on
top of different HW that implements exact same P4 pipelines (assuming
driver implementations are complete.)