From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rere.qmqm.pl (rere.qmqm.pl [84.10.57.10]) by dpdk.org (Postfix) with ESMTP id F266036E for ; Wed, 14 Dec 2016 03:11:12 +0100 (CET) Received: by rere.qmqm.pl (Postfix, from userid 1000) id B1D246082; Wed, 14 Dec 2016 03:11:12 +0100 (CET) Date: Wed, 14 Dec 2016 03:11:12 +0100 From: Michal Miroslaw To: "Ananyev, Konstantin" Cc: "dev@dpdk.org" Message-ID: <20161214021111.2l4h4gsbjxmbffca@rere.qmqm.pl> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-2 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0EA71F@irsmsx105.ger.corp.intel.com> User-Agent: Mutt/1.6.2-neo (2016-07-23) 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 02:11:13 -0000 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 wrote: > > > > [...] > > > > > > > > > > Subject: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict > > > > > > > > > > > > > > > > > > > > Signed-off-by: Michał Mirosław > > > > > > > > > > --- > > > > > > > > > > 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_acl/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_acl_rule_data *rd) > > > > > > > > > > if ((RTE_LEN2MASK(RTE_ACL_MAX_CATEGORIES, typeof(rd->category_mask)) & > > > > > > > > > > rd->category_mask) == 0 || > > > > > > > > > > rd->priority > RTE_ACL_MAX_PRIORITY || > > > > > > > > > > - rd->priority < RTE_ACL_MIN_PRIORITY || > > > > > > > > > > - rd->userdata == 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 to 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-matching case, but > > > > > > > > it's in "dirty hack" state, as of yet. > > > > > > > > > > > > > > With that chane rte_acl_classify() might produce invalid results. > > > > > > > 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 this is only > > > > > > > > a policy choice and as such would be better to be made by the user. > > > > > > > > > > > > > > I believe it does. > > > > > > > userdata==0 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 could not find > > > > > > anything, really. The only thing I found is that iff I use zero userdata > > > > > > in a rule I won't be able to differentiate a case where it matched 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 invalid userdata==0? > > > > > 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 before > > > > > 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 distinguish > > > > 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 distinguish > > > 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 don'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-match 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 packet, > > and because of this restriction we're loosing half of the userdata field. > > If we would add this "decrement if non-zero" workaround this would keep > > 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 point me > > > > to the code in DPDK where this actually matters? > > > > > > It was a while, when I looked into ACL code in details, but as remember > > > that's the only reason: we need some value to be reserved as NO_MATCH. > > > Let say in build_trie() we set results to zero for rules with unused categories: > > > for (m = context->cfg.num_categories; 0 != m--; ) { > > > if (rule->f->data.category_mask & (1 << m)) { > > > end->mrt->results[m] = rule->f->data.userdata; > > > end->mrt->priority[m] = rule->f->data.priority; > > > } else { > > > end->mrt->results[m] = 0; > > > end->mrt->priority[m] = 0; > > > } > > > } > > > > So, if I understand correctly, 0 is a default value for category result. > > Any matching rule with priority >= 0 will override it (leaving last highest > > priority rule's userdata). This will just work the same for anyone needing > > the distinction (when he doesn't use userdata == 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 choose > > their way. At the beginning I haven't found any mention of the restriction > > 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==0). > Is my understanding correct? That is exactly it. Best Regards, Michał Mirosław