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

[Thomas Monjalon <thomas@monjalon.net>]

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.


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

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