From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id B570F370 for ; Wed, 14 Dec 2016 13:16:04 +0100 (CET) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga104.fm.intel.com with ESMTP; 14 Dec 2016 04:16:03 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,346,1477983600"; d="scan'208";a="42244934" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by orsmga005.jf.intel.com with ESMTP; 14 Dec 2016 04:16:02 -0800 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by IRSMSX109.ger.corp.intel.com (163.33.3.23) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 14 Dec 2016 12:16:01 +0000 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.212]) by irsmsx112.ger.corp.intel.com ([169.254.1.86]) with mapi id 14.03.0248.002; Wed, 14 Dec 2016 12:16:01 +0000 From: "Ananyev, Konstantin" To: Michal Miroslaw CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict Thread-Index: AQHSVN2ahB5rgtyRpkyle4teQhaCOKEFry9QgAA3/4CAAANbwIAADPcAgAADYICAABNDgIAAED0QgAAOFACAAD3ioIAASp0AgACnKTA= Date: Wed, 14 Dec 2016 12:16:01 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772583F0EC95B@irsmsx105.ger.corp.intel.com> References: <2601191342CEEE43887BDE71AB9772583F0E6E0B@irsmsx105.ger.corp.intel.com> <20161213135443.ovmlunbh67dr4tew@rere.qmqm.pl> <2601191342CEEE43887BDE71AB9772583F0E7008@irsmsx105.ger.corp.intel.com> <20161213145308.lqqnm6ivryjfxih7@rere.qmqm.pl> <2601191342CEEE43887BDE71AB9772583F0E736D@irsmsx105.ger.corp.intel.com> <20161213161409.ekbagsze5pcy2ppl@rere.qmqm.pl> <2601191342CEEE43887BDE71AB9772583F0E7567@irsmsx105.ger.corp.intel.com> <20161213180240.is54unzdj3yfexq5@rere.qmqm.pl> <2601191342CEEE43887BDE71AB9772583F0EA71F@irsmsx105.ger.corp.intel.com> <20161214021111.2l4h4gsbjxmbffca@rere.qmqm.pl> In-Reply-To: <20161214021111.2l4h4gsbjxmbffca@rere.qmqm.pl> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="iso-8859-2" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Dec 2016 12:16:05 -0000 Hi Michal, > -----Original Message----- > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl] > Sent: Wednesday, December 14, 2016 2:11 AM > To: Ananyev, Konstantin > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict >=20 > On Tue, Dec 13, 2016 at 09:55:12PM +0000, Ananyev, Konstantin wrote: > > > > > > > -----Original Message----- > > > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl] > > > Sent: Tuesday, December 13, 2016 6:03 PM > > > To: Ananyev, Konstantin > > > Cc: dev@dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict > > > > > > On Tue, Dec 13, 2016 at 05:27:31PM +0000, Ananyev, Konstantin wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl] > > > > > Sent: Tuesday, December 13, 2016 4:14 PM > > > > > To: Ananyev, Konstantin > > > > > Cc: dev@dpdk.org > > > > > Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict > > > > > > > > > > On Tue, Dec 13, 2016 at 03:13:42PM +0000, Ananyev, Konstantin wro= te: > > > > > [...] > > > > > > > > > > > Subject: [dpdk-dev] [PATCH 04/13] acl: allow zero ver= dict > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Micha=B3 Miros=B3aw > > > > > > > > > > > --- > > > > > > > > > > > lib/librte_acl/rte_acl.c | 3 +-- > > > > > > > > > > > lib/librte_acl/rte_acl.h | 2 -- > > > > > > > > > > > lib/librte_table/rte_table_acl.c | 2 +- > > > > > > > > > > > 3 files changed, 2 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > > > > > > > > > diff --git a/lib/librte_acl/rte_acl.c b/lib/librte_ac= l/rte_acl.c > > > > > > > > > > > index 8b7e92c..d1f40be 100644 > > > > > > > > > > > --- a/lib/librte_acl/rte_acl.c > > > > > > > > > > > +++ b/lib/librte_acl/rte_acl.c > > > > > > > > > > > @@ -313,8 +313,7 @@ acl_check_rule(const struct rte_a= cl_rule_data *rd) > > > > > > > > > > > if ((RTE_LEN2MASK(RTE_ACL_MAX_CATEGORIES, typeof(rd= ->category_mask)) & > > > > > > > > > > > rd->category_mask) =3D=3D 0 || > > > > > > > > > > > rd->priority > RTE_ACL_MAX_PRIORITY || > > > > > > > > > > > - rd->priority < RTE_ACL_MIN_PRIORITY || > > > > > > > > > > > - rd->userdata =3D=3D RTE_ACL_INVALID_USERDATA) > > > > > > > > > > > + rd->priority < RTE_ACL_MIN_PRIORITY) > > > > > > > > > > > return -EINVAL; > > > > > > > > > > > return 0; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > I am not sure, how it supposed to work properly? > > > > > > > > > > Zero value is reserved and ifnicates that no match were= found for that input. > > > > > > > > > > > > > > > > > > This is actually in use by us. In our use we don't need t= o differentiate > > > > > > > > > matching a rule with zero verdict vs not matching a rule = at all. I also > > > > > > > > > have a patch that changes the value returned in non-match= ing case, but > > > > > > > > > it's in "dirty hack" state, as of yet. > > > > > > > > > > > > > > > > With that chane rte_acl_classify() might produce invalid re= sults. > > > > > > > > Even if you don't need it (I still don't understand how) , = it doesn't mean other people > > > > > > > > don't need it either and it is ok to change it. > > > > > > > > > > > > > > > > > > > > > > > > > > The ACL code does not treat zero userdata specially, so t= his is only > > > > > > > > > a policy choice and as such would be better to be made by= the user. > > > > > > > > > > > > > > > > I believe it does. > > > > > > > > userdata=3D=3D0 is a reserved value. > > > > > > > > When rte_acl_clasify() returns 0 for that particular input,= it means 'no matches were found'. > > > > > > > > > > > > > > Dear Konstantin, > > > > > > > > > > > > > > Can you describe how the ACL code treats zero specially? I co= uld not find > > > > > > > anything, really. The only thing I found is that iff I use ze= ro userdata > > > > > > > in a rule I won't be able to differentiate a case where it ma= tched from > > > > > > > a case where no rule matched. > > > > > > > > > > > > Yes, that's what I am talking about. > > > > > > > > > > > > > If I all my rules have non-zero userdata, > > > > > > > then this patch changes nothing. > > > > > > > > > > > > Ok, then why do you remove a code that does checking for invali= d userdata=3D=3D0? > > > > > > That supposed to prevent user to setup invalid value by mistake= . > > > > > > > > > > > > But if I have a table where 0 means drop > > > > > > > (default-drop policy) then being able to use zero userdata in= DROP rules > > > > > > > makes the ACLs just that more useful. > > > > > > > > > > > > Ok, and what prevents you from do +1 to your policy values befo= re > > > > > > you insert it into the ACL table and -1 after you retrieved it = via rte_acl_classify()? > > > > > > > > > > The check is enforcing an assumption that all users want to disti= nguish > > > > > the cases whether any rule matched and whether no rules matched. = Not all > > > > > users do, hence the assumption is invalid and this patch removes = it. > > > > > > > > The check is based on the assumption that users might need to disti= nguish > > > > the situation when no rules were matched. > > > > To support that we need a reserved userdata value, which would mean > > > > NO_MATCH. > > > > From what I heard, most users do need this ability, those who don't > > > > can easily overcome it. > > > > > > That's actually my point. Some users need the distinction, so they do= n't use > > > zero userdata in their rules and have their work done. Some users don= 't need > > > it and would prefer to just use the convenience of zero being no-matc= h signal > > > to insert "non-matching" rules (now they have to check two values for= the > > > same signal). > > > > > > > > Yes, people can work around it by loosing 1 of 2^32 useful values= and > > > > > convoluting their code. > > > > Yes, one of 2^32 values is reserved. > > > > Any reason why (2^32 - 1) values might not be enough? > > > > > > Sure. We're using userdata as a bitmask of actions to take on the pac= ket, > > > and because of this restriction we're loosing half of the userdata fi= eld. > > > If we would add this "decrement if non-zero" workaround this would ke= ep > > > biting us on every occasion where we touch the ACL verdict code. > > > > > > > > You seem to argue that 0 is somehow an invalid value, but I can't= find > > > > > anything in the ACL that would require it to be so. Could you poi= nt me > > > > > to the code in DPDK where this actually matters? > > > > > > > > It was a while, when I looked into ACL code in details, but as reme= mber > > > > that's the only reason: we need some value to be reserved as NO_MAT= CH. > > > > Let say in build_trie() we set results to zero for rules with unuse= d categories: > > > > for (m =3D context->cfg.num_categories; 0 !=3D m--; ) { > > > > if (rule->f->data.category_mask & (1 << m))= { > > > > end->mrt->results[m] =3D rule->f->d= ata.userdata; > > > > end->mrt->priority[m] =3D rule->f->= data.priority; > > > > } else { > > > > end->mrt->results[m] =3D 0; > > > > end->mrt->priority[m] =3D 0; > > > > } > > > > } > > > > > > So, if I understand correctly, 0 is a default value for category resu= lt. > > > Any matching rule with priority >=3D 0 will override it (leaving last= highest > > > priority rule's userdata). This will just work the same for anyone ne= eding > > > the distinction (when he doesn't use userdata =3D=3D 0) and also for = those who > > > don't -- when the restriction is removed. > > > > > > I think that it comes to documenting the behaviour and let users choo= se > > > their way. At the beginning I haven't found any mention of the restri= ction > > > in the docs, so I had to spend a fair amount of time to find out why = the > > > zero is so special (it wasn't). > > > > Ok, so you suggest the following: > > 1. Zero value for both userdata and results still has a special meaning= : NO_MATCH. > > 2. Allow user to create a rule(s) that would on hit return NO_MATCH for= it, > > as if no rule was matched by that input (i.e. rule's userdata=3D=3D0). > > Is my understanding correct? >=20 > That is exactly it. Ok, the idea still seems a bit unusual to me, but I suppose might be useful= for some cases. Again, can't think up a scenario when it would break something. So don't have any good reason to object :) Before we proceed with it can I ask you for two things:=20 - Verify that autotest_acl works as expected. - Update PG: http://dpdk.org/doc/guides/prog_guide/packet_classif_access_ct= rl.html#rule-definition I think these lines need to be repharsed because of that patch: "userdata: A user-defined field that could be any value except zero." Konstantin