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 01ADCA0032; Mon, 12 Sep 2022 18:57:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 996D5400D4; Mon, 12 Sep 2022 18:57:30 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id A5DAF4003C for ; Mon, 12 Sep 2022 18:57:28 +0200 (CEST) Received: by shelob.oktetlabs.ru (Postfix, from userid 115) id 301EF7D; Mon, 12 Sep 2022 19:57:28 +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, URIBL_BLOCKED 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 55EA97B; Mon, 12 Sep 2022 19:57:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 55EA97B Authentication-Results: shelob.oktetlabs.ru/55EA97B; dkim=none; dkim-atps=neutral Date: Mon, 12 Sep 2022 19:57:27 +0300 (MSK) From: Ivan Malov To: Rongwei Liu cc: matan@nvidia.com, viacheslavo@nvidia.com, orika@nvidia.com, thomas@monjalon.net, Aman Singh , Yuying Zhang , Andrew Rybchenko , dev@dpdk.org, rasland@nvidia.com Subject: Re: [PATCH v1] ethdev: add direction info when creating the transfer table In-Reply-To: <20220907024020.2474860-1-rongweil@nvidia.com> Message-ID: <1be72d6-be5b-88b2-f15-16fd2c6d0c0@oktetlabs.ru> References: <20220907024020.2474860-1-rongweil@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed 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 Hi, On Wed, 7 Sep 2022, Rongwei Liu wrote: > The transfer domain rule is able to match traffic wire/vf > origin and it means two directions' underlayer resource. The point of fact is that matching traffic coming from some entity like wire / VF has been long generalised in the form of representors. So, a flow rule with attribute "transfer" is able to match traffic coming from either a REPRESENTED_PORT or from a PORT_REPRESENTOR (please find these items). > > In customer deployments, they usually match only one direction > traffic in single flow table: either from wire or from vf. Which customer deployments? Could you please provide detailed examples? > > Introduce one new member transfer_mode into rte_flow_attr to > indicate the flow table direction property: from wire, from vf > or bi-direction(default). AFAIK, 'rte_flow_attr' serves both traditional flow rule insertion and asynchronous (table) approach. The patch adds the attributes to generic 'rte_flow_attr' but, for some reason, ignores non-table rules. For example, the diff below adds the attributes to "table" commands in testpmd but does not add them to regular (non-table) commands like "flow create". Why? > > It helps to save underlayer memory also on insertion rate. Which memory? Host memory? NIC memory? Term "underlayer" is vague. I suggest that the commit message be revised to first explain how such memory is spent currently, then explain why this is not optimal and, finally, which way the patch is supposed to improve that. I.e. be more specific. > > By default, the transfer domain is bi-direction, and no behavior changes. > > 1. Match wire origin only > flow template_table 0 create group 0 priority 0 transfer wire_orig... > 2. Match vf origin only > flow template_table 0 create group 0 priority 0 transfer vf_orig... > > Signed-off-by: Rongwei Liu > --- > app/test-pmd/cmdline_flow.c | 26 +++++++++++++++++++++ > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++- > lib/ethdev/rte_flow.h | 9 ++++++- > 3 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > index 7f50028eb7..b25b595e82 100644 > --- a/app/test-pmd/cmdline_flow.c > +++ b/app/test-pmd/cmdline_flow.c > @@ -177,6 +177,8 @@ enum index { > TABLE_INGRESS, > TABLE_EGRESS, > TABLE_TRANSFER, > + TABLE_TRANSFER_WIRE_ORIG, > + TABLE_TRANSFER_VF_ORIG, > TABLE_RULES_NUMBER, > TABLE_PATTERN_TEMPLATE, > TABLE_ACTIONS_TEMPLATE, > @@ -1141,6 +1143,8 @@ static const enum index next_table_attr[] = { > TABLE_INGRESS, > TABLE_EGRESS, > TABLE_TRANSFER, > + TABLE_TRANSFER_WIRE_ORIG, > + TABLE_TRANSFER_VF_ORIG, > TABLE_RULES_NUMBER, > TABLE_PATTERN_TEMPLATE, > TABLE_ACTIONS_TEMPLATE, > @@ -2881,6 +2885,18 @@ static const struct token token_list[] = { > .next = NEXT(next_table_attr), > .call = parse_table, > }, > + [TABLE_TRANSFER_WIRE_ORIG] = { > + .name = "wire_orig", > + .help = "affect rule direction to transfer", This does not explain the "wire" aspect. It's too broad. > + .next = NEXT(next_table_attr), > + .call = parse_table, > + }, > + [TABLE_TRANSFER_VF_ORIG] = { > + .name = "vf_orig", > + .help = "affect rule direction to transfer", This explanation simply duplicates such of the "wire_orig". It does not explain the "vf" part. Should be more specific. > + .next = NEXT(next_table_attr), > + .call = parse_table, > + }, > [TABLE_RULES_NUMBER] = { > .name = "rules_number", > .help = "number of rules in table", > @@ -8894,6 +8910,16 @@ parse_table(struct context *ctx, const struct token > *token, > case TABLE_TRANSFER: > out->args.table.attr.flow_attr.transfer = 1; > return len; > + case TABLE_TRANSFER_WIRE_ORIG: > + if (!out->args.table.attr.flow_attr.transfer) > + return -1; > + out->args.table.attr.flow_attr.transfer_mode = 1; > + return len; > + case TABLE_TRANSFER_VF_ORIG: > + if (!out->args.table.attr.flow_attr.transfer) > + return -1; > + out->args.table.attr.flow_attr.transfer_mode = 2; > + return len; > default: > return -1; > } > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > index 330e34427d..603b7988dd 100644 > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > @@ -3332,7 +3332,8 @@ It is bound to ``rte_flow_template_table_create()``:: > > flow template_table {port_id} create > [table_id {id}] [group {group_id}] > - [priority {level}] [ingress] [egress] [transfer] > + [priority {level}] [ingress] [egress] > + [transfer [vf_orig] [wire_orig]] Is it correct? Shouldn't it rather be [transfer] [vf_orig] [wire_orig] ? > rules_number {number} > pattern_template {pattern_template_id} > actions_template {actions_template_id} > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h > index a79f1e7ef0..512b08d817 100644 > --- a/lib/ethdev/rte_flow.h > +++ b/lib/ethdev/rte_flow.h > @@ -130,7 +130,14 @@ struct rte_flow_attr { > * through a suitable port. @see rte_flow_pick_transfer_proxy(). > */ > uint32_t transfer:1; > - uint32_t reserved:29; /**< Reserved, must be zero. */ > + /** > + * 0 means bidirection, > + * 0x1 origin uplink, What does "uplink" mean? It's too vague. Hardly a good term. > + * 0x2 origin vport, What does "origin vport" mean? Hardly a good term as well. > + * N/A both set. What's this? > + */ > + uint32_t transfer_mode:2; > + uint32_t reserved:27; /**< Reserved, must be zero. */ > }; > > /** > -- > 2.27.0 > Since the attributes are added to generic 'struct rte_flow_attr', non-table (synchronous) flow rules are supposed to support them, too. If that is indeed the case, then I'm afraid such proposal does not agree with the existing items PORT_REPRESENTOR and REPRESENTED_PORT. They do exactly the same thing, but they are designed to be way more generic. Why not use them? Ivan