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

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Wednesday, August 16, 2023 6:08 PM
> To: Morten Brørup <mb@smartsharesystems.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Ori
> Kam <orika@nvidia.com>; Jerin Jacob <jerinjacobk@gmail.com>
> Cc: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> david.marchand@redhat.com; Richardson, Bruce
> <bruce.richardson@intel.com>; jerinj@marvell.com; techboard@dpdk.org;
> Mcnamara, John <john.mcnamara@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; dev@dpdk.org
> Subject: Re: [PATCH] ethdev: introduce generic flow item and action
> 
> 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.
> 

Exactly.

> 
> Purpose seems to provide control path for P4 pipelines.
> 

+1

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

Yes.

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

If this approach would be acceptable, we'd be glad to go for it.

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

Yes, flow items and actions are defined by the user, the vendor is happily
unaware about what the users are doing in their P4 programs.
> 
> 
> 
> 
> 
> 
> 
> 
>