[PATCH v7] ethdev: add special flags when creating async transfer table

[Rongwei Liu]

HI Ivan:

BR
Rongwei

> -----Original Message-----
> From: Ivan Malov <ivan.malov at arknetworks.am>
> Sent: Tuesday, January 31, 2023 13:30
> To: Rongwei Liu <rongweil at nvidia.com>
> Cc: Matan Azrad <matan at nvidia.com>; Slava Ovsiienko
> <viacheslavo at nvidia.com>; Ori Kam <orika at nvidia.com>; NBU-Contact-
> Thomas Monjalon (EXTERNAL) <thomas at monjalon.net>; Aman Singh
> <aman.deep.singh at intel.com>; Yuying Zhang <yuying.zhang at intel.com>;
> Ferruh Yigit <ferruh.yigit at amd.com>; Andrew Rybchenko
> <andrew.rybchenko at oktetlabs.ru>; dev at dpdk.org; Raslan Darawsheh
> <rasland at nvidia.com>
> Subject: RE: [PATCH v7] ethdev: add special flags when creating async transfer
> table
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Rongwei,
> 
> OK, I hear ya. Thanks for persevering.
> 
> I still hope community will comment on the possibility to provide a hint
> mechanism for always-the-same match items, with the perspective of
> becoming more versatile. Other than that, your current patch might be OK,
> but, again, I think other reviewers' comments (if any) shall be addressed. But
> no strong objections from me.
>
Good to hear the confirmation from you.
> By the way, for this "specialise" field, in your opinion, which extra flags could
> emerge in future / would be nice to have? I mean, is there any concept of
> what can be added to this field's namespace and what can't be?
> 
IMO, flags that helps to reduce the resource or speed up insertion/deletion should be considered as a candidate.
> Thank you.
> 
> On Tue, 31 Jan 2023, Rongwei Liu wrote:
> 
> > HI Ivan
> >
> > BR
> > Rongwei
> >
> >> -----Original Message-----
> >> From: Ivan Malov <ivan.malov at arknetworks.am>
> >> Sent: Tuesday, January 31, 2023 07:00
> >> To: Rongwei Liu <rongweil at nvidia.com>
> >> Cc: Matan Azrad <matan at nvidia.com>; Slava Ovsiienko
> >> <viacheslavo at nvidia.com>; Ori Kam <orika at nvidia.com>; NBU-Contact-
> >> Thomas Monjalon (EXTERNAL) <thomas at monjalon.net>; Aman Singh
> >> <aman.deep.singh at intel.com>; Yuying Zhang <yuying.zhang at intel.com>;
> >> Ferruh Yigit <ferruh.yigit at amd.com>; Andrew Rybchenko
> >> <andrew.rybchenko at oktetlabs.ru>; dev at dpdk.org; Raslan Darawsheh
> >> <rasland at nvidia.com>
> >> Subject: RE: [PATCH v7] ethdev: add special flags when creating async
> >> transfer table
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> Hi Rongwei,
> >>
> >> Thanks for the professional attitude.
> >> Hope this discussion gets us on the
> >> same page. Please see below.
> > Thanks for the suggestion and comments. Hope everything goes well.
> >>
> >> On Mon, 30 Jan 2023, Rongwei Liu wrote:
> >>
> >>> HI Ivan
> >>>
> >>> BR
> >>> Rongwei
> >>>
> >>>> -----Original Message-----
> >>>> From: Ivan Malov <ivan.malov at arknetworks.am>
> >>>> Sent: Monday, January 30, 2023 15:40
> >>>> To: Rongwei Liu <rongweil at nvidia.com>
> >>>> Cc: Matan Azrad <matan at nvidia.com>; Slava Ovsiienko
> >>>> <viacheslavo at nvidia.com>; Ori Kam <orika at nvidia.com>; NBU-Contact-
> >>>> Thomas Monjalon (EXTERNAL) <thomas at monjalon.net>; Aman Singh
> >>>> <aman.deep.singh at intel.com>; Yuying Zhang <yuying.zhang at intel.com>;
> >>>> Ferruh Yigit <ferruh.yigit at amd.com>; Andrew Rybchenko
> >>>> <andrew.rybchenko at oktetlabs.ru>; dev at dpdk.org; Raslan Darawsheh
> >>>> <rasland at nvidia.com>
> >>>> Subject: RE: [PATCH v7] ethdev: add special flags when creating
> >>>> async transfer table
> >>>>
> >>>> External email: Use caution opening links or attachments
> >>>>
> >>>>
> >>>> Hi Rongwei,
> >>>>
> >>>> For my responses, PSB.
> >>>>
> >>>> By the way, now you mention things like wasting memory and
> >>>> insertion optimisastions, are there any comparative figures to see
> >>>> the effect of this hint on insertion performance / memory footprint?
> >>>> Some "before" / "after" examples would really be helpful.
> >>>>
> >>> Good to hear we reach agreement almost.
> >>
> >> Very well.
> >>
> >> The key point here is that one may agree that some optimisations are
> >> indeed needed, yes. I don't deny the fact that some vendors might
> >> have issues with how the API maps to the HW capabilities.
> >> Yes, some undesirable resource overhead shall be avoided, but the
> >> high level hints for that have to be designed with care.
> >>
> > Totally agree. That' why we emphasize "optional for PMD" and "application
> should take care of hint"
> >>> First, the hint has nothing related to matching, only affects PMD
> >>> resource
> >> management.
> >>
> >> You say "PMD resource management". For the flow management, that's
> >> mostly vendor-specific, I take it. Let me explain. The application,
> >> for instance, can control the number of Tx descriptors in the queue during
> setup stage.
> >> Tx descriptors are a common type of HW resource, hence the explicit
> >> control for it available to applications. For flow library, it's not
> >> like that. Different vendors have different "underlayer"
> >> representations, they may vary drastically.
> > The resource I mentioned is about "steering logic" not SW datapath.
> > With flow rules offloading, hardware should store the steering logic in its
> reachable memory no matter embedded in or mapping from host OS.
> >>
> >> I take it, in the case of the HW you're working with, this hint
> >> indeed maps to something that is entirely resource-related and which
> >> does not belong in this specific vendor's match criteria. I 100%
> >> understand that, in your case, these are separate. But the point is
> >> that, on the high-level programming level (vendor-neutral), such a
> >> hint is in fact a match criterion. Because it tells the driver to
> >> limit the scope of matching to just "from net"/"from vport", the same way
> other metadata items do (represented_port).
> >> The only difference is that it refers to a group of unspecified ports
> >> which have something in common.
> >>
> > " a group of unspecified ports" means dynamic and flexible, right. IMO it's
> valid and fits sync flow perfectly.
> > But in async, when allocating resources (table creation), the group info is
> still unknown. We don't want to scatter it into each rule insertion.
> >> So, although I don't strongly object having some hints like this one
> >> in the generic API, I nevertheless disagree with describing this as
> >> just "resource- specific" and not being a match criterion. It's just
> >> not always the case. It might not be valid for *all* NIC vendors.
> >>
> > Agree, not valid for *all* NIC vendors.
> >>> In my local test, it can save around 50% memory in the VxLAN
> >>> encap/decap
> >> example case.
> >>
> >> Forgive me in case this has been already discussed; where's that memory?
> >> I mean, is it some sort of general-purpose memory? Or some
> >> HW-specific table capacity overhead? I'm trying to understand how the
> >> feature will be useful to other vendors, or how common this problem is.
> >>
> > See above. HW always needs memory to store offloaded rules no matter
> embedded in chip or borrowed from OS.
> >>> Insertion rate has very very few improvements.
> >>>> After all, I'm not objecting this patch. But I believe that other reviewers'
> >>>> concerns should nevertheless be addressed anyway.
> >>> Let me try to show the hint is useful.
> >>>>
> >>>> On Mon, 30 Jan 2023, Rongwei Liu wrote:
> >>>>
> >>>>> Hi Ivan,
> >>>>>
> >>>>> BR
> >>>>> Rongwei
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Ivan Malov <ivan.malov at arknetworks.am>
> >>>>>> Sent: Monday, January 30, 2023 08:00
> >>>>>> To: Rongwei Liu <rongweil at nvidia.com>
> >>>>>> Cc: Matan Azrad <matan at nvidia.com>; Slava Ovsiienko
> >>>>>> <viacheslavo at nvidia.com>; Ori Kam <orika at nvidia.com>;
> >>>>>> NBU-Contact- Thomas Monjalon (EXTERNAL) <thomas at monjalon.net>;
> >>>>>> Aman Singh <aman.deep.singh at intel.com>; Yuying Zhang
> >>>>>> <yuying.zhang at intel.com>; Ferruh Yigit <ferruh.yigit at amd.com>;
> >>>>>> Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>; dev at dpdk.org;
> >>>>>> Raslan Darawsheh <rasland at nvidia.com>
> >>>>>> Subject: Re: [PATCH v7] ethdev: add special flags when creating
> >>>>>> async transfer table
> >>>>>>
> >>>>>> External email: Use caution opening links or attachments
> >>>>>>
> >>>>>>
> >>>>>> Hi Rongwei,
> >>>>>>
> >>>>>> Thanks for persevering. I have no strong opinion, but, at least,
> >>>>>> the fact that the new flags are no longer meant for use in
> >>>>>> rte_flow_attr, which is clearly not the right place for such, is
> >>>>>> an
> >> improvement.
> >>>>>>
> >>>>> Thanks for the suggestion, move it to rte_flow_table_attr now and it'
> >>>> dedicated to async API.
> >>>>>> However, let's take a closer look at the current patch, shall we?
> >>>>>>
> >>>>>> But, before we get to that, I'd like to kindly request that you
> >>>>>> provide a more concrete example of how this feature is supposed
> >>>>>> to be used. Are there some real-life application examples?
> >>>>>>
> >>>>> Sure.
> >>>>>> Also, to me, it's still unclear how an application can obtain the
> >>>>>> knowledge of this hint in the first instance. For example, can
> >>>>>> Open vSwitch somehow tell ethdevs representing physical ports
> >>>>>> from ones representing "vports" (host endpoints)?
> >>>>>> How does it know which attribute to specify?
> >>>>>>
> >>>>> Hint should be initiated by application and application knows it'
> >>>>> traffic
> >>>> pattern which highly relates to deployment.
> >>>>> Let' use VxLAN encap/decap as an example:
> >>>>> 1. Traffic from wire should be VxLAN pattern and do the decap,
> >>>>> then send to
> >>>> different vports.
> >>>>> flow pattern_template 0 create transfer relaxed no
> >>>>> pattern_template_id
> >>>>> 4 template represented_port ethdev_port_id is 0 / eth / ipv4 / udp
> >>>>> / vxlan / tag index is 0 data is 0x33 / end flow actions_template
> >>>>> 0 create transfer actions_template_id 4 template raw_decap index 0
> >>>>> / represented_port ethdev_port_id 1 / end mask raw_decap index 0 /
> >>>>> represented_port ethdev_port_id 1 / end flow template_table 0
> >>>>> create group 1 priority 0 transfer wire_orig table_id 4
> >>>>> rules_number 128 pattern_template 4 actions_template 4
> >>>>>
> >>>>> 2. Traffic from vports should be encap with different VxLAN header
> >>>>> and send
> >>>> to wire.
> >>>>> flow actions_template 1 create transfer actions_template_id 5
> >>>>> template raw_encap index 0 / represented_port ethdev_port_id 0 /
> >>>>> end mask raw_encap index 0 / represented_port ethdev_port_id 0 /
> >>>>> end flow template_table 0 create group 1 priority 0 transfer
> >>>>> vport_orig table_id 5 rules_number 128 pattern_template 4
> >>>>> actions_template 5
> >>>>>
> >>>>>> For the rest of my notes, PSB.
> >>>>>>
> >>>>>> On Mon, 14 Nov 2022, Rongwei Liu wrote:
> >>>>>>
> >>>>>>> In case flow rules match only one kind of traffic in a flow
> >>>>>>> table, then optimization can be done via allocation of this table.
> >>>>>>
> >>>>>> This wording might confuse readers. Consider rephrasing it, please:
> >>>>>> If multiple flow rules share a common set of match masks, then
> >>>>>> they might belong in a flow table which can be pre-allocated.
> >>>>>>
> >>>>>>> Such optimization is possible only if the application gives a
> >>>>>>> hint about its usage of the table during initial configuration.
> >>>>>>>
> >>>>>>> The transfer domain rules may process traffic from wire or
> >>>>>>> vport, which may correspond to two kinds of underlayer resources.
> >>>>>>
> >>>>>> Why name it a "vport"? Why not "host"?
> >>>>>>
> >>>>>> host = packets generated by any of the host's "vport"s wire =
> >>>>>> packets arriving at the NIC from the network
> >>>>> Vport is "virtual port" for short and contains "VF/SF" for now.
> >>>>> Per my thoughts, it' clearer and maps to DPDK port
> probing/management.
> >>>>
> >>>> I understand that "host" might not be a brilliant name.
> >>>>
> >>>> If "vport" stands for every port of the NIC that is not a network
> >>>> port, then this name might be OK to me, but why doesn't it cover PFs?
> >>>> A PF is clearly not a network / physical port. Why just VF/SF then?
> >>>> Where
> >> does that "for now"
> >>>> decision come from? Just wondering.
> >>>>
> >>> "For now" stands for my understanding. DPDK is always in evolution, right?
> >>> You are right, PF should be included in 'vport" concept.
> >>>>>>
> >>>>>>> That's why the first two hints introduced in this patch are
> >>>>>>> about wire and vport traffic specialization.
> >>>>>>> Wire means traffic arrives from the uplink port while vport
> >>>>>>> means traffic initiated from VF/SF.
> >>>>>>
> >>>>>> By the sound of it, the meaning is confined to just VFs/SFs.
> >>>>>> What if the user wants to match packets coming from PFs?
> >>>>>>
> >>>>> It should be "wire_orig".
> >>>>
> >>>> Forgive me, but that does not sound correct. Say, there's an
> >>>> application and it has a PF plugged into it: ethdev index 0. And
> >>>> the application transmits packets using rte_eth_tx_burst() from that port.
> >>>> You say that these packets can be matched via "wire_orig".
> >>>> But they do not come from the wire. They come from PF...
> >>> Hmm. My mistake.
> >>> This may highly depend on PMD implementation. Basically, PFs'
> >>> traffic may contain "from wire"/"wire_orig" and '"from
> local"/"vport_orig".
> >>> That' why we emphasize it' optional for PMD.
> >>>>
> >>>>>>>
> >>>>>>> There are two possible approaches for providing the hints.
> >>>>>>> Using IPv4 as an example:
> >>>>>>> 1. Use pattern item in both template table and flow rules.
> >>>>>>>
> >>>>>>>  pattern_template: pattern ANY_VPORT / eth / ipv4 is 1.1.1.1 /
> >>>>>>> end async flow create: pattern ANY_VPORT / eth / ipv4 is 1.1.1.2
> >>>>>>> / end
> >>>>>>>
> >>>>>>>  "ANY_VPORT" needs to be present in each flow rule even if it's
> >>>>>>> just a hint. No value to match because matching is already done
> >>>>>>> by
> >>>>>>>  IPv4 item.
> >>>>>>
> >>>>>> Why no value to match on? How does it prevent rogue tenants from
> >>>>>> spoofing network headers? If the application receives a packet on
> >>>>>> a particular vport's representor, then it may strictly specify
> >>>>>> item represented_port pointing to that vport so that only packets
> >>>>>> from that vport
> >>>> match.
> >>>>>>
> >>>>>> Why isn't security a consideration?
> >>>>>>
> >>>>> There is some misunderstanding here.  "ANY_VPORT" is the approach
> >>>>> (new
> >>>> matching item without value)  suggested by you.
> >>>> I'm not talking about ANY_VPORT in this particular paragraph.
> >>>>
> >>>> There's item "represented_port" mentioned over there. I'm just
> >>>> asking about this "already done by IPv4 item" bit. Yes, it matches
> >>>> on the header but not on the true origin of the packet (the logical
> >>>> port of the NIC). If the app knows which logical port the packet
> >>>> ingresses the NIC, why not match on it for security?
> >>>>
> >>> Hint is not a matching and it implies how to manage underlayer
> >>> steering
> >> resource.
> >>> If "vport_orig" is present, PMD will only apply the steering logic
> >>> to vport
> >> traffic.
> >>> The resource is allocated in the async table before each rule.
> >>> Already cover
> >> security considerations.
> >>> Matching on "represented_port" needs to program each rule,
> >>> considering a
> >> port range like index "5-10".
> >>> Hint tells PMD only to take care of traffic from vport regardless
> >>> the port
> >> index.
> >>>
> >>>>> I was explaining we need to apply it to each flow rule even if
> >>>>> it's only a flag
> >>>> and no value.
> >>>>
> >>>> That's clear. But PSB.
> >>>>
> >>>>>>>
> >>>>>>> 2. Add special flags into table_attr.
> >>>>>>>
> >>>>>>>  template_table 0 create table_id 0 group 1 transfer vport_orig
> >>>>>>>
> >>>>>>> Approach 1 needs to specify the pattern in each flow rule which
> >>>>>>> wastes memory and is not user friendly.
> >>>>>>
> >>>>>> What if the user has to insert a group of rules which not only
> >>>>>> have the same set of match masks but also share exactly the same
> >>>>>> match spec values for a limited subset of network items (for
> >>>>>> example, those of an encap. header)? This way, a subset of
> >>>>>> network item specs can remain fixed across many rules. Does that
> >>>>>> count as wasting
> >> memory?
> >>>>>>
> >>>>> Per my understanding, you are talking "multiple spec and mask mixing".
> >>>>
> >>>> Say, there's a group of rules, and each of them matches on exactly
> >>>> the same encap. header (the same in all rules), but different
> >>>> internal match
> >> field values.
> >>>> So, why don't these "fixed"
> >>>> encap. header items deserve being "optimised" somehow, the same way
> >>>> as this "wire_orig" does?
> >>> We are back to original point. Async approach is trying to
> >>> pre-allocate
> >> resources and speed up the insertion.
> >>> Resource is allocated in async table stage and we only have mask
> >> information.
> >>> In each rule, the matching value passes in. I guess you are saying
> >>> to optimize
> >> per different matching values, right?
> >>> This needs dynamic calculations per each rule and wastes the
> >>> resource in
> >> async table(table allocates resource for all possible values).
> >>>>
> >>>> If the application knows for sure that there will be packets with
> >>>> exactly the same encap. header, - that forms this special knowledge
> >>>> that can be used during init times to help the PMD optimise
> >>>> resource
> >> allocation.
> >>>> Isn't that so? Don't these items deserve some form of a "hint"?
> >>>>
> >>> It can deserve some kinds of "hint". But see above, these hints are
> >>> per rule
> >> and resource allocation happens before rules.
> >>
> >> That's not per rule. Perhaps I should've worded it differently.
> >>
> >> Suppose, an application has to insert many flow rules, each of which
> >> has match items A and B. Item A not only has the same mask in *all*
> >> rule instances, but also the same spec.
> >> On the other hand, item B only has the same mask in all the rules,
> >> but its spec is different for each rule.
> >>
> >> In this example, the application can allocate a template with items A
> >> and B, but that only provides a fixed mask for them. And the
> >> application will HAVE to provide item A with exactly the same spec in
> >> all rule instances. The PMD, in turn, will HAVE to process this item
> >> every time, being unable to see it's in fact the same at all times.
> >>
> >> To me, this sounds very similar to how you described the need to
> >> always provide item ANY_VPORT in each rule thus facing some waste of
> >> memory and parsing difficulties.
> >>
> >> If the application knows that a certain item (or a certain fraction
> >> of items) is going to be entirely the same (mask +
> >> spec) across all the rules, why shouldn't it be able to express this
> >> as a hint to the PMD? Why shouldn't it be able to avoid providing
> >> such items in every new flow rule instance? The same way the "vport_orig"
> works.
> >>
> >> I'm not demanding that you re-implement or re-design this.
> >> Just trying to find out whether such a problem can indeed be acknowledged.
> >> Or has it been solved already? If not, then perhaps it pays to just
> >> discuss whether solving it can be combined with this "vport_orig" solution.
> >>
> >> What do you think? What do others think?
> >>
> >>>>> We provide a hint in this patch and no assumption on the matching
> >> patterns.
> >>>>
> >>>> So I understand. My point is, certain portions of matching patterns
> >>>> may be "fixed" = entirely the same (masks and specs) in all rules
> >>>> of a table. Why not give PMD a "hint" about them, too?
> >>>>
> >>>>> I think matching pattern is totally controlled by application layer.
> >>>>
> >>>> So is the "direction" spec: the app layer has item represented_port
> >>>> to control that. But, still, we're here to discuss a hint for that.
> >>>> Why does the new hint aim exclusively at optimising out this
> >>>> specific meta item? Why isn't it possible to care about a generic
> >>>> portion of "know in advance" all-the-same items?
> >>> " generic portion of know in advance" is some still kind of dynamic
> >>> approach,
> >> right?
> >>> Imagine a situation. DPDK has 10 VFs, each VF may have different
> >>> VxLAN
> >> encap headers.
> >>> This hint approach can work for 10 VFs once.
> >>> In public cloud deployments, each VF/SF may map to different users,
> >>> but
> >> underlay is almost same(GRE/VxLAN... differ in filed values).
> >>>>
> >>>>> "wasting memory " because your approach needs to scatter in each
> >>>>> rule
> >>>> while this patch only needs to set table_attr once.
> >>>>> No relation with matching patter totally.
> >>>>
> >>>> The slight problem with your proposal is that for some reason only
> >>>> one type of a match criterion deserves a hint moved to the attrs.
> >>>> Whilst in reality the applicaction may know in advance that certain
> >>>> subsets of items will not only have the same masks in all rules but
> >>>> also totally the same specs. If that is a valid use case, why
> >>>> doesn't it deserve the same (more
> >>>> generic) optimisation / a hint? Just wondering...
> >>>> Or has that been addressed already somehow?
> >>>>
> >>> Believe me, the hint helps us to save significant resources already.
> >>
> >> I'm not arguing it can be helpful. You're working round the clock to
> >> offer a solution, - that's fine and is greatly appreciated.
> >> But what I'm trying to say is that it looks like the problem might
> >> manifest itself for other type of knowledge that also may deserve a hint.
> Hence the questions.
> >> Hence the offer to think of covering more match criteria, not just net/vport.
> >>
> >>> Per my view, your proposal is totally valid in sync approach, but
> >>> please check my response, Async is trying to allocate resources in
> >>> advance
> >> and speed up insertion ASAP.
> >>
> >> So if it's valid in sync approach, then why can't it be valid in the async one?
> >> And I guess it can reflect positively on the insertion rate, too. Why
> >> limit this "hint" approach to just one aspect then?
> >>
> >> I'm sure we're close to understanding each other here.
> >> Yes, "orig_vport" is just a one-bit knowledge, and seems innocent to
> >> add as a hint, but why not make it possible to have a hint for an
> >> arbitrary set of always- the-same match criteria?
> >>
> >> In this case, nobody will ever argue of whether a hint is a match
> >> criterion or if it's not. It will be quite a generic instrument, potentially
> useful to vendors.
> >> I'm afraid I can't think of an immediate example of such usefulness,
> >> but at least it will appear as generic as possible from the API perspective.
> >>
> >>>>>> If yes, then the problem does not concern just a single pair of
> >>>>>> attributes, but rather deserves a more versatile solution like
> >>>>>> some sort of indirect grouping of constant item specs.
> >>>>>> Have you considered such options?
> >>>>> See above.
> >>>>>>
> >>>>>>> This patch takes the 2nd approach and introduces one new member
> >>>>>>> "specialize" into rte_flow_table_attr to indicate possible flow
> >>>>>>> table optimization.
> >>>>>>
> >>>>>> The name "specialize" might have some drawbacks:
> >>>>>> - spelling difference (specialise/specialize)
> >>>>>> - in grep output, will mix with flows' "spec"
> >>>>>> - quite long
> >>>>>> - not a noun
> >>>>>>
> >>>>>> Why not "scope"? Or something like that?
> >>>>>>
> >>>>> It means special optimization to PMD. "scope" is more rogue.
> >>>>
> >>>> Why is it "rogue"? Scope is something limiting the point of view.
> >>>> So are the suggested flags. Flag "wire_origin" (or whatever it can
> >>>> be named
> >>>> eventually) limits the scope of matching. No?
> >>>>
> >>> Hint won't interfere with matching. It has no knowledge of matching.
> >>
> >> Does specifying "orig_vport" actually provide a *choice* for the packet
> origin?
> >> Does it filter out everything else? If yes, then, alas, it *is*
> >> matching. Because matching is choosing something of interest. Let's face it.
> >>
> >> As I said above, I do acknowledge the fact that, for some vendors,
> >> this match criterion, internally goes to a different HW aspect that
> >> is separate from matching on, for example, IPv4 addresses.
> >> That's OK. But for some vendors, this might be just a regular match
> >> criterion internally. So let's describe it with care.
> >>
> >>> Instead, it only controls matching resources.  "wire_orig" tells PMD
> >>> to
> >> allocate HW resource for traffic from wire only.
> >>
> >> If it controls "matching resources", it's indeed affiliated with matching then.
> >> Look. When the application creates a template, it tells the PMD that
> >> it is going to match on this, this and this.
> >> Masks... No exact values; they will come at a later stage. But, with
> >> this "wire_orig", the application tells the PMD that not only it will
> >> match on
> >> *some* "direction", but it actually provides a SPEC for that. If it
> >> indicates bit "wire_orig", that equals to setting a "mask" for the "direction
> enum"
> >> AND a "spec" (WIRE). Isn't that the case?
> >>
> >> If it is, then please see my above concerns about possibly having
> >> similar need to provide exact-spec hints for other items as well.
> >>
> >>> Then traffic from vport is sliently ignored. Hint doesn't know what
> >>> are
> >> matched and how many fields are involves.
> >>>>>>>
> >>>>>>> By default, there is no hint, so the behavior of the transfer
> >>>>>>> domain doesn't change.
> >>>>>>> There is no guarantee that the hint will be used by the PMD.
> >>>>>>>
> >>>>>>> Signed-off-by: Rongwei Liu <rongweil at nvidia.com>
> >>>>>>> Acked-by: Ori Kam <orika at nvidia.com>
> >>>>>>>
> >>>>>>> v2: Move the new field to template table attribute.
> >>>>>>> v4: Mark it as optional and clear the concept.
> >>>>>>> v5: Change specialize type to uint32_t.
> >>>>>>> v6: Change the flags to macros and re-construct the commit log.
> >>>>>>> v7: Fix build failure.
> >>>>>>> ---
> >>>>>>> app/test-pmd/cmdline_flow.c                 | 26 +++++++++++++++++++
> >>>>>>> doc/guides/prog_guide/rte_flow.rst          | 15 +++++++++++
> >>>>>>> doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
> >>>>>>> lib/ethdev/rte_flow.h                       | 28 +++++++++++++++++++++
> >>>>>>> 4 files changed, 71 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/app/test-pmd/cmdline_flow.c
> >>>>>>> b/app/test-pmd/cmdline_flow.c index 88108498e0..62197f2618
> >>>>>>> 100644
> >>>>>>> --- a/app/test-pmd/cmdline_flow.c
> >>>>>>> +++ b/app/test-pmd/cmdline_flow.c
> >>>>>>> @@ -184,6 +184,8 @@ enum index {
> >>>>>>>       TABLE_INGRESS,
> >>>>>>>       TABLE_EGRESS,
> >>>>>>>       TABLE_TRANSFER,
> >>>>>>> +     TABLE_TRANSFER_WIRE_ORIG,
> >>>>>>> +     TABLE_TRANSFER_VPORT_ORIG,
> >>>>>>>       TABLE_RULES_NUMBER,
> >>>>>>>       TABLE_PATTERN_TEMPLATE,
> >>>>>>>       TABLE_ACTIONS_TEMPLATE,
> >>>>>>> @@ -1158,6 +1160,8 @@ static const enum index next_table_attr[] =
> {
> >>>>>>>       TABLE_INGRESS,
> >>>>>>>       TABLE_EGRESS,
> >>>>>>>       TABLE_TRANSFER,
> >>>>>>> +     TABLE_TRANSFER_WIRE_ORIG,
> >>>>>>> +     TABLE_TRANSFER_VPORT_ORIG,
> >>>>>>>       TABLE_RULES_NUMBER,
> >>>>>>>       TABLE_PATTERN_TEMPLATE,
> >>>>>>>       TABLE_ACTIONS_TEMPLATE,
> >>>>>>> @@ -2933,6 +2937,18 @@ static const struct token token_list[] = {
> >>>>>>>               .next = NEXT(next_table_attr),
> >>>>>>>               .call = parse_table,
> >>>>>>>       },
> >>>>>>> +     [TABLE_TRANSFER_WIRE_ORIG] = {
> >>>>>>> +             .name = "wire_orig",
> >>>>>>> +             .help = "affect rule direction to transfer",
> >>>>>>> +             .next = NEXT(next_table_attr),
> >>>>>>> +             .call = parse_table,
> >>>>>>> +     },
> >>>>>>> +     [TABLE_TRANSFER_VPORT_ORIG] = {
> >>>>>>> +             .name = "vport_orig",
> >>>>>>> +             .help = "affect rule direction to transfer",
> >>>>>>> +             .next = NEXT(next_table_attr),
> >>>>>>> +             .call = parse_table,
> >>>>>>> +     },
> >>>>>>>       [TABLE_RULES_NUMBER] = {
> >>>>>>>               .name = "rules_number",
> >>>>>>>               .help = "number of rules in table", @@ -8993,6
> >>>>>>> +9009,16 @@ parse_table(struct context *ctx, const struct token
> >>>>>>> +*token,
> >>>>>>>       case TABLE_TRANSFER:
> >>>>>>>               out->args.table.attr.flow_attr.transfer = 1;
> >>>>>>>               return len;
> >>>>>>> +     case TABLE_TRANSFER_WIRE_ORIG:
> >>>>>>> +             if (!out->args.table.attr.flow_attr.transfer)
> >>>>>>> +                     return -1;
> >>>>>>> +             out->args.table.attr.specialize =
> >>>>>>> RTE_FLOW_TABLE_SPECIALIZE_TRANSFER_WIRE_ORIG;
> >>>>>>> +             return len;
> >>>>>>> +     case TABLE_TRANSFER_VPORT_ORIG:
> >>>>>>> +             if (!out->args.table.attr.flow_attr.transfer)
> >>>>>>> +                     return -1;
> >>>>>>> +             out->args.table.attr.specialize =
> >>>>>>> RTE_FLOW_TABLE_SPECIALIZE_TRANSFER_VPORT_ORIG;
> >>>>>>> +             return len;
> >>>>>>>       default:
> >>>>>>>               return -1;
> >>>>>>>       }
> >>>>>>> diff --git a/doc/guides/prog_guide/rte_flow.rst
> >>>>>>> b/doc/guides/prog_guide/rte_flow.rst
> >>>>>>> index 3e6242803d..d9ca041ae4 100644
> >>>>>>> --- a/doc/guides/prog_guide/rte_flow.rst
> >>>>>>> +++ b/doc/guides/prog_guide/rte_flow.rst
> >>>>>>> @@ -3605,6 +3605,21 @@ and pattern and actions templates are
> >> created.
> >>>>>>>                   &actions_templates, nb_actions_templ,
> >>>>>>>                   &error);
> >>>>>>>
> >>>>>>> +Table Attribute: Specialize
> >>>>>>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>>>>>> +
> >>>>>>> +Application can help optimizing underlayer resources and
> >>>>>>> +insertion rate by specializing template table.
> >>>>>>> +Specialization is done by providing hints in the template table
> >>>>>>> +attribute ``specialize``.
> >>>>>>> +
> >>>>>>> +This attribute is not mandatory for each PMD to implement.
> >>>>>>> +If a hint is not supported, it will be silently ignored, and no
> >>>>>>> +special optimization is done.
> >>>>>>
> >>>>>> Silently ignoring the field does not sit well with the
> >>>>>> application's possible intent to drop represented_port match from
> >>>>>> the
> >> patterns.
> >>>>>> From my point of view, if the application sets this attribute, it
> >>>>>> believes it can rely on it, that is, packets coming from host
> >>>>>> won't match if the attribute asks to match network only, for instance.
> >>>>>> Has this
> >>>> been considered?
> >>>>>>
> >>>>>>> +
> >>>>>>> +If a table is specialized, the application should make sure the
> >>>>>>> +rules comply with the table attribute.
> >>>>>>
> >>>>>> How does the application enforce that? I would appreciate you explain
> it.
> >>>>>>
> >>>>>>> +
> >>>>>>> Asynchronous operations
> >>>>>>> -----------------------
> >>>>>>>
> >>>>>>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>>>>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>>>>> index 96c5ae0fe4..b3238415f4 100644
> >>>>>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>>>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>>>>> @@ -3145,7 +3145,8 @@ It is bound to
> >>>>>> ``rte_flow_template_table_create()``::
> >>>>>>>
> >>>>>>>   flow template_table {port_id} create
> >>>>>>>       [table_id {id}] [group {group_id}]
> >>>>>>> -       [priority {level}] [ingress] [egress] [transfer]
> >>>>>>> +       [priority {level}] [ingress] [egress]
> >>>>>>> +       [transfer [vport_orig] [wire_orig]]
> >>>>>>>       rules_number {number}
> >>>>>>>       pattern_template {pattern_template_id}
> >>>>>>>       actions_template {actions_template_id} diff --git
> >>>>>>> a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> >>>>>>> 8858b56428..c27b48c5c1 100644
> >>>>>>> --- a/lib/ethdev/rte_flow.h
> >>>>>>> +++ b/lib/ethdev/rte_flow.h
> >>>>>>> @@ -5186,6 +5186,29 @@
> >> rte_flow_actions_template_destroy(uint16_t
> >>>>>>> port_id, */ struct rte_flow_template_table;
> >>>>>>>
> >>>>>>> +/**@{@name Special optional flags for template table attribute
> >>>>>>> + * Each bit is a hint for table specialization,
> >>>>>>> + * offering a potential optimization at PMD layer.
> >>>>>>> + * PMD can ignore the unsupported bits silently.
> >>>>>>> + */
> >>>>>>> +/**
> >>>>>>> + * Specialize table for transfer flows which come only from wire.
> >>>>>>> + * It allows PMD not to allocate resources for non-wire
> >>>>>>> +originated
> >> traffic.
> >>>>>>> + * This bit is not a matching criteria, just an optimization hint.
> >>>>>>
> >>>>>> You intended to spell "criterion", I take it. And still, it *is*
> >>>>>> a match
> >> criterion.
> >>>>>> I'm not denying the possible need to have this criterion at the
> >>>>>> earliest processing stage. That might be OK, but I still have a
> >>>>>> hunch that this is too specific.
> >>>>>> Please see my comment above about wasting memory.
> >>>>>> I guess this type of criterion is not the only one that may need
> >>>>>> to be provided as a "hint".
> >>>>>>
> >>>>>>> + * Flow rules which match non-wire originated traffic will be
> >>>>>>> + missed
> >>>>>>> + * if the hint is supported.
> >>>>>>
> >>>>>> And what if it's unsupported? Is it indeed OK to silently ignore it?
> >>>>>>
> >>>>>>> + */
> >>>>>>> +#define RTE_FLOW_TABLE_SPECIALIZE_TRANSFER_WIRE_ORIG
> >>>>>> RTE_BIT32(0)
> >>>>>>
> >>>>>> Why not RTE_FLOW_TABLE_SCOPE_FROM_WIRE ?
> >>>>>>
> >>>>>> To me, TRANSFER looks redundant as this bit is already supposed
> >>>>>> to be ticked in the "struct rte_flow_attr flow_attr" field of the
> >>>>>> "struct rte_flow_template_table_attr".
> >>>>>>
> >>>>>>> +/**
> >>>>>>> + * Specialize table for transfer flows which come only from
> >>>>>>> +vport (e.g. VF,
> >>>>>>> SF).
> >>>>>>
> >>>>>> And PF?
> >>>>>>
> >>>>>>> + * It allows PMD not to allocate resources for non-vport
> >>>>>>> + originated
> >> traffic.
> >>>>>>> + * This bit is not a matching criteria, just an optimization hint.
> >>>>>>> + * Flow rules which match non-vport originated traffic will be
> >>>>>>> +missed
> >>>>>>> + * if the hint is supported.
> >>>>>>> + */
> >>>>>>> +#define RTE_FLOW_TABLE_SPECIALIZE_TRANSFER_VPORT_ORIG
> >>>>>> RTE_BIT32(1)
> >>>>>>
> >>>>>> Why not RTE_FLOW_TABLE_SCOPE_FROM_HOST ?
> >>>>>>
> >>>>>>> +/**@}*/
> >>>>>>> +
> >>>>>>> /**
> >>>>>>> * @warning
> >>>>>>> * @b EXPERIMENTAL: this API may change without prior notice.
> >>>>>>> @@ -5201,6 +5224,11 @@ struct rte_flow_template_table_attr {
> >>>>>>>        * Maximum number of flow rules that this table holds.
> >>>>>>>        */
> >>>>>>>       uint32_t nb_flows;
> >>>>>>> +     /**
> >>>>>>> +      * Optional hint flags for PMD optimization.
> >>>>>>> +      * Value is composed with RTE_FLOW_TABLE_SPECIALIZE_*.
> >>>>>>> +      */
> >>>>>>> +     uint32_t specialize;
> >>>>>>
> >>>>>> Why not "scope" or something?
> >>>>>>
> >>>>>>> };
> >>>>>>>
> >>>>>>> /**
> >>>>>>> --
> >>>>>>> 2.27.0
> >>>>>>>
> >>>>>>
> >>>>>> Thank you.
> >>>>>
> >>>
> >>
> >> Thank you.
> >