Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict

[Michal Miroslaw <mirq-linux@rere.qmqm.pl>]

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 <konstantin.ananyev@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-linux@rere.qmqm.pl]
> > > > Sent: Tuesday, December 13, 2016 4:14 PM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@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.miroslaw@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.

Best Regards,
Michał Mirosław