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?
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").
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.
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.
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?
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 understandnamely rte_eth_dev_info.switch_info.domain_idport 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 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.
Thank you.
Ivan
