Hi Ori,
On Tue, 20 Sep 2022, Ori Kam wrote:
Hi Ivan,
-----Original Message-----
From: Ivan Malov <ivan.ma...@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 <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.
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.
In my understanding, the user may want to create flow table A
and use pattern template A' for it, which is as follows:
any_vports / eth / ipv4 / udp
The PMD can see this item and treat it exactly the same
way as it could treat such attribute ("where to allocate
resources, etc.").
Then the user may want to create flow table B and
use pattern template B' for it:
any_phy_ports / eth / ipv4 / udp
Once again, the PMD can clearly see the difference between
the A' and B' templates and, this time, allocate resources
the other way (as per efficiency requirements).
By saying "can be used on number of different tables", do you mean
that it is important to make the *network* part of the pattern
shareable between flow tables? I.e. are you saying that
templates A' and B' cause resource duplication just
because of the same *network* part in your case?
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.
To me, it *is* the match criterion which, at the same time, serves
as a value indicating the way how resources should be allocated.
But before all, it is a match criterion.
If it refers to a group of ports = in order to ditch "the other half"
of traffic from consideration (like Rongwei explained), then it
looks like a match criterion.
From Nvidia viewpoint we need this information so we can allocate the resource
at the correct
place and avoid inserting duplication of rules.
I see.
I agree that by using the item we can get the same results, but it is incorrect
since we are not matching on it.
If one provides item UDP in the pattern and does not match on any UDP
fields, doing so nevertheless *is* matching on particular packet type.
The same seemingly goes for the new attribute / item. If it is
provided, then the user doesn't want the rule to affect
packets coming from certain ports (i.e. from wire).
So still sounds like matching.
Part of the idea of template API is to give as many hints as possible to the
PMD so the insertion will be optimized.
I see.
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.
But how do these two viewpoints contradict each other?
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.
Great we're on the same page regarding this bit.
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?
Good question. But, in case some way exists to make such optimisation
laconic enough to avoid confusion etc., then it should be no problem
in preferring the pattern approach over attribute approach.
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.
This is a valuable clarification. However, even if the attribute way
may seem OK after this explanation, then I still don't understand
why it is required to add this attribute to the generic "struct
rte_flow_attr" and not just to the *table* attr.
Generic "struct rte_flow_attr" is used both for async and
sync (regular) approach. So why add something to generic
struct which is never going to make sense to sync flows?
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
Thanks,
Ivan
