RE: [PATCH v1] ethdev: add direction info when creating the transfer table

[Ori Kam <orika@nvidia.com>]

Hi Ivan,

> -----Original Message-----
> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> Sent: Tuesday, 20 September 2022 15:46
> 
> Hi Ori,
> 
> On Tue, 20 Sep 2022, Ori Kam wrote:
> 
> > Hi Ivan, Thomas and Rongwei
> >
> >> -----Original Message-----
> >> From: Thomas Monjalon <thomas@monjalon.net>
> >> Sent: Thursday, 15 September 2022 14:16
> >>
> >> 15/09/2022 12:59, Ivan Malov:
> >>> Hi Rongwei,
> >>>
> >>> In this reply, I do not include the previous mail because the amount
> >>> of inline commentary has gone haywire over the past couple of days.
> >>> Let's re-iterate.
> >>>
> >>> But before I get to that, I'd like to offer a fresh perspective:
> >>>
> >>> Perhaps, if we all agree that term "vport" means an endpoint which
> >>> can stand for any "port" except for physical one, then it should
> >>> be possible to use term ANY_VPORTS rather than ANY_GUEST_PORTS.
> >>
> >> The opposite of "physical" is "virtual" indeed.
> >>
> >>> But that's tricky, of course. I don't have a way with naming,
> >>> so more opinions are welcome and very-very desirable here.
> >>>
> >>> So:
> >>>
> >>> 1) Do you agree that, in your proposal, the new "wire_orig" / "vf_orig"
> >>>     primitives are in fact yet another match criteria?
> >>>
> >>>     ..
> >>>
> >>>     To me, it looks so. If they are match criteria, then they belong
> >>>     in match pattern, that is, they should be expressed as new items.
> >>>
> >>>     For "transfer" rules, the *existing* attributes are: "group"
> >>>     and "priority". As you may note, these are clearly not match
> >>>     criteria. They control the look-up order. So, to this day,
> >>>     there're no match criteria in DPDK expressed as attributes.
> >>>
> >>>     If these "wire_orig" / "vf_orig" are going to be introduced
> >>>     as attributes, that should be backed with strong motivation.
> >>
> >> I prefer we keep matching in a single place, not in attributes.
> >>
> >
> > I think we are talking about two different features.
> > Feature 1:
> > Allow matching on all vports that are not wire
> > Feature 2:
> > Save allocation space and allow fast insertion.
> > In this case, the matching is not on all vports it can be just part of the vports
> > but it will never be the wire port.
> > For example:
> > port 0 - wire
> > ports 1,2,3,4,5  - vports
> > the application want to inset only those rules:
> > represented_port(port_id=2) / eth / ipv4 (src==xx)
> > represented_port(port_id=4) / eth / ipv4 (src==xx)
> > represented_port(port_id=4) / eth / ipv4 (src==yy)
> >
> > For feature 1 I fully agree with you Ivan, this should be added as an item.
> 
> Thank you.
> 
> > For feature 2 I think Rongwei's suggestion is the better option.
> > If I understand correctly the idea is to give hint to the PMD on where to
> allocate memory
> > and how to insert the rules most optimally. Since this is shared for all rules it
> makes more sense
> > to add it as an attribute, just like we don’t have an ingress item (maybe we
> should?)
> 
> But isn't pattern template also supposed to be shared for all rules
> in the table? I.e., the user creates an async flow table and submits
> a flow "shape" (which consists of attrs, pattern template and action
> template). So why should "giving a hint" via an item template be
> considered worse than doig so via an attribute?
> 

The same item template maybe used elsewhere, for example, the following
pattern  eth / ipv4(src, dst) / udp(sport, dport), can be used on number of different
tables.
I think that the main difference between us is that from my point of view this value is just
where to allocate resources / how to better insert the rule. It is not related to matching.
From Nvidia viewpoint we need this information so we can allocate the resource at the correct
place and avoid inserting duplication of rules.
I agree that by using the item we can get the same results, but it is incorrect since we are not matching on it.
Part of the idea of template API is to give as many hints as possible to the PMD so the insertion will be optimized.


> As for "ingress" item, - no, one should not add such. We have had
> many discussions concerning this bit in the past. Ingress/egress
> are non-transfer terms. They belong in the scope of vNIC / ethdev
> filtering, not to embedded switch rules.
> 
> In my opinion, in the embedded switch, one should either point to
> some precise switch ports (using REPRESENTOR / REPRESENTED items)
> or use another kind of item to refer to a "super set" of ports
> which have something in common ("all wire ports", "all NON-wire ports").
> 

But this is my point we don't want all wire ports or all NON-wire ports, we just know that in this table
we will have only non-wire / wire ports.

> >
> > Ivan we have the item RTE_FLOW_ITEM_TYPE_PF and
> RTE_FLOW_ITEM_TYPE_VF which are deprecated,
> > So do you want to un-deprecate them?
> 
> No. These items are deprecated because:
> 
> a) their names suggest that application knows whether an ethdev
>     sits on top of a PF or that the application has some
>     knowledge of existence of particular VFs, but in
>     reality applications should not be worried of
>     the underlying function type = to them, all
>     ethdevs are just representors of something,
>     and if the application needs to refer to
>     VFs (or other PFs, - doesn't matter), it
>     should do that via REPRESENTOR items;
> 
> b) such items would duplicate REPRESENTOR / REPRESENTED.
> 
Agree with everything you say. 

> >
> > To summarize, if PMD can use such an hint during rule creation and save
> memory, I vote
> > to allow it.
> > if the idea is to match on all vports then it should be an item.
> 
> But such a hint would effectively be a match criterion, too, right?
> So, in fact it's a combined use case: a match criterion which is
> flexible enough to be a "hint" = i.e. the PMD can see it when
> processing the pattern *template* and treat it as a hint.
>

Yes, but it is an implicit match, just like saying ingress. Egress it has meaning above the
matching. In addition, there is no reason to add extra item for each rule we create, just
to enable something that is fixed during the table creation.
Extra item in pattern template means extra item for each rule.
I know we can avoid this and optimize the code but why add something that no one needs
after table creation?

 
> >
> >>
> >>> 2) From your viewpoint, why items "ANY_PHYS_PORTS" and
> >> "ANY_VPORTS"
> >>>     won't do? Or, which problems do you think they may inflict?
> >>>
> >>>     ..
> >>>
> >>>     Previously, you explained why REPRESENTED_PORT would not
> >>>     fit your needs. And I understand your point: to async API,
> >>>     two pattern templates which both have item REPRESENTED_PORT
> >>>     in them cannot be clearly distinguished and are in fact the
> >>>     same set of criteria (provided that all other items are also
> >>>     the same and have the same masks). Templates are, well,
> >>>     templates (or shapes) of the rules to come later and
> >>>     do not include exact "spec" for the "ethdev_id".
> >>>     Got it.
> >>>
> >>>     But that's not going to be the case with items ANY_PHYS_PORTS and
> >>>     ANY_VPORTS, is it? In one async table template, the user submits
> >>>     item ANY_PHYS_PORTS (instead of table attribute "wire_orig").
> >>>     In another template, the user submits item ANY_VPORTS to
> >>>     state that they want to match only traffic transmitted
> >>>     software endpoints (DPDK ethdevs, guest VFs, etc.)
> >>>     connected to the switch.
> >>>
> >>>     In this example, the PMD will clearly see that the two templates
> >>>     differ. So it will be able to allocate separate resources, each
> >>>     one "cutting one half of traffic" (as per your concept).
> >>>
> >>> 3) In your most recent response, you suggested that one might have
> >>>     had the attributes occupied for some other purposes. To me,
> >>>     they're not. Neither me nor my closest colleagues have
> >>>     any plans on them. When I advocate using item approach
> >>>     over the attribute approach, I do this to ensure
> >>>     a) clarity of the API contract and b) robustness.
> >
> > If something is shared for all rules in the same table, it should be a table
> > property.
> 
> But the whole pattern *template* is also a table property, isn't it?
> 

Like I said above the pattern template can be used in all domains that is why
there is a split between table and patter, in addition to that each table may have
number of pattern templates.

> >
> >>>
> >>> 4) Also, in your response, you suggested that I might have
> >>>     confused item mask and spec. That is not the case.
> >>>     If we agree, that switch domain ID is unneeded in
> >>>     the new items, then these items will have no
> >>>     fields in them (like item PF had not had any
> >>>     before it was deprecated).
> >>>
> >>>     No fields in new items => no field masks.
> >>>     So what's the problem then?
> >>>
> >>> 5) With regard to our talk about identifying the relationship
> >>>     between ethdevs and switch domains, you said that the user
> >>>     could know the difference from the very beginning:
> >>>     /sysfs/ .... /PF_BDF/sriov_num
> >>>
> >>>     That is true for the user who starts the application, but
> >>>     this knowledge is hard to obtain from the application
> >>>     perspective = it's hard to automate.
> >>>
> >>>     This is why ethdevs are able to advertise their domain IDs.
> >>>     And, as I explained, looking at domain ID to understand
> >>
> >> namely rte_eth_dev_info.switch_info.domain_id
> >>
> >>>     port relationship is valid, whilst looking at proxy IDs
> >>>     to achieve the same goal is not. Proxy port IDs only
> >>>     serve the purpose of finding an entry point for
> >>>     managing flows. That has slightly different
> >>>     meaning, but this subtle difference is important.
> >>
> >> There is also a concept of sibling ports
> >> to get all ports belonging to the same hardware.
> >>
> >>
> >>> 6) As for the confusion over the difference between fixing
> >>>     bugs and making the code robust by extra checks:
> >>>
> >>>     Yes, I agree that the programmer who writes the
> >>>     application must be intelligent enough to use
> >>>     flow primitives the proper way. Yes, the user
> >>>     who starts the application also should thread
> >>>     carefully. But that does not prevent some
> >>>     mistakes in other parts of code from
> >>>     corrupting various chunks of memory,
> >>>     including, for example, flow attrs.
> >>>
> >>>     You say that such mistakes have to be "just fixed"
> >>>     as any other bugs. Right. But how much time will
> >>>     the programmer spend to identify the bugs?
> >>>
> >>>     If the PMDs do all the checks (as with attributes),
> >>>     the hypothetical bug will manifest itself much
> >>>     earlier. That will simplify debugging by a lot...
> >>>
> >>>     So, my point is that it's still better to ensure
> >>>     that new flow primitives have all necessary
> >>>     checks in place. For attributes, it is
> >>>     required to add them separately.
> >>
> >> If flow insertion is done in a fast path,
> >> such checks may be skipped.
> >
> > The idea is that all rules in this table will share the same configuration,
> > there is no reason to say everything again for each rule. This is why
> > the rule attributes were moved to the table struct and not per rule.
> >
> >>
> >>>     For items, as I explained, it might not be necessary
> >>>     in the majority of cases simply because of the
> >>>     switch (item->type) { case } structure.
> >>>
> >>> So, these are some of my points to explain why the
> >>> attribute approach is untenable. To me, attributes
> >>> are something global, which demands checks in all
> >>> flow-capable PMDs. Items seem better because they
> >>> are don't cares to all PMDs which are unaware of
> >>> the async concept. So, even if someone does not
> >>> implement the async concept or does not like
> >>> the new item names, they can turn a blind
> >>> eye to this - with attributes, thay can't.
> >>>
> >
> > Good point,
> > Maybe we should add hints in the attribute,
> > for example, hint_only_wire in this case it will be clear that
> > PMD may ignore this, and it should be fully documented that this is not a
> mandatory field.
> > What do you think?
> 
> Theoretically, making terminology softer (like with the word "hint")
> could make things easier for vendors who may find the new feature
> confusing or something like that. But if, in reality, this hint
> is indeed another match criterion (see my comments above), then
> in no event shall the prefix "hint" be an excuse for this
> criterion not being expressed as a pattern item.
> 

Please see my response above. This is the point it is much more than matching.

> Please hear me out: I don't mean to sound arrogant, - just trying
> to understand why expressing the new bit as an item can't be
> efficient enough for the async flow approach.
>

I don't think you are arrogant, and I hope that you see that I do understand your comments.
saying that I hope I explained why I think it is better to have it as a table attribute and not as an
item. (We are not matching on it, this helps the PMD allocate the table at the best location and avoid
duplication of rules)  

If you wish, we can have a short phone call and discuss this.

Best,
Ori

 
> >
> >>> Thank you.
> >>
> >>
> >
> >
> 
> Ivan