> -----Original Message----- > From: Ferruh Yigit <ferruh.yi...@amd.com> > Sent: Wednesday, August 16, 2023 6:08 PM > To: Morten Brørup <m...@smartsharesystems.com>; Dumitrescu, Cristian > <cristian.dumitre...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>; Ori > Kam <or...@nvidia.com>; Jerin Jacob <jerinjac...@gmail.com> > Cc: NBU-Contact-Thomas Monjalon (EXTERNAL) <tho...@monjalon.net>; > david.march...@redhat.com; Richardson, Bruce > <bruce.richard...@intel.com>; jer...@marvell.com; techbo...@dpdk.org; > Mcnamara, John <john.mcnam...@intel.com>; Zhang, Helin > <helin.zh...@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.dumitre...@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. > > > > > > > > >
