Hi Michal, > -----Original Message----- > From: Michal Miroslaw [mailto:mirq-li...@rere.qmqm.pl] > Sent: Wednesday, December 14, 2016 2:11 AM > To: Ananyev, Konstantin <konstantin.anan...@intel.com> > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict > > On Tue, Dec 13, 2016 at 09:55:12PM +0000, Ananyev, Konstantin wrote: > > > > > > > -----Original Message----- > > > From: Michal Miroslaw [mailto:mirq-li...@rere.qmqm.pl] > > > Sent: Tuesday, December 13, 2016 6:03 PM > > > To: Ananyev, Konstantin <konstantin.anan...@intel.com> > > > 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-li...@rere.qmqm.pl] > > > > > Sent: Tuesday, December 13, 2016 4:14 PM > > > > > To: Ananyev, Konstantin <konstantin.anan...@intel.com> > > > > > 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 > > > > > > > > > > > <michal.miros...@atendesoftware.pl> > > > > > > > > > > > --- > > > > > > > > > > > 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.
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: - Verify that autotest_acl works as expected. - Update PG: http://dpdk.org/doc/guides/prog_guide/packet_classif_access_ctrl.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
