From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3BC83A00C3; Tue, 20 Sep 2022 14:45:57 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D3DD540DFB; Tue, 20 Sep 2022 14:45:56 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 555494069B for ; Tue, 20 Sep 2022 14:45:55 +0200 (CEST) Received: by shelob.oktetlabs.ru (Postfix, from userid 115) id A40427B; Tue, 20 Sep 2022 15:45:54 +0300 (MSK) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on mail1.oktetlabs.ru X-Spam-Level: X-Spam-Status: No, score=0.8 required=5.0 tests=ALL_TRUSTED, DKIM_ADSP_DISCARD autolearn=no autolearn_force=no version=3.4.6 Received: from bree.oktetlabs.ru (bree.oktetlabs.ru [192.168.34.5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPS id 30C9A66; Tue, 20 Sep 2022 15:45:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 30C9A66 Authentication-Results: shelob.oktetlabs.ru/30C9A66; dkim=none; dkim-atps=neutral Date: Tue, 20 Sep 2022 15:45:53 +0300 (MSK) From: Ivan Malov To: Ori Kam cc: "NBU-Contact-Thomas Monjalon (EXTERNAL)" , Rongwei Liu , Matan Azrad , Slava Ovsiienko , Aman Singh , Yuying Zhang , Andrew Rybchenko , "dev@dpdk.org" , Raslan Darawsheh Subject: RE: [PATCH v1] ethdev: add direction info when creating the transfer table In-Reply-To: Message-ID: References: <20220907024020.2474860-1-rongweil@nvidia.com> <297a487e-b5d-4928-e96-c80b2603dd8@oktetlabs.ru> <2834724.SvYEEZNnvj@thomas> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="-1463810782-1161068163-1663677953=:105420" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. ---1463810782-1161068163-1663677953=:105420 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8BIT Hi Ori, On Tue, 20 Sep 2022, Ori Kam wrote: > Hi Ivan, Thomas and Rongwei > >> -----Original Message----- >> From: Thomas Monjalon >> 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 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 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 ---1463810782-1161068163-1663677953=:105420--