Hi Ivan, Thomas and Rongwei
> -----Original Message-----
> From: Thomas Monjalon <tho...@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.
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?)
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?
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.
>
> > 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.
> >
> > 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?
> > Thank you.
>
>
