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

[Ananyev, Konstantin]

> -----Original Message-----
> From: Michal Miroslaw [mailto:mirq-linux at rere.qmqm.pl]
> Sent: Tuesday, December 13, 2016 2:53 PM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict
> 
> On Tue, Dec 13, 2016 at 02:14:19PM +0000, Ananyev, Konstantin wrote:
> > > -----Original Message-----
> > > From: Michal Miroslaw [mailto:mirq-linux at rere.qmqm.pl]
> > > Sent: Tuesday, December 13, 2016 1:55 PM
> > > To: Ananyev, Konstantin <konstantin.ananyev at intel.com>
> > > Cc: dev at dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict
> > >
> > > On Tue, Dec 13, 2016 at 10:36:16AM +0000, Ananyev, Konstantin wrote:
> > > > Hi Michal,
> > > >
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Michal Miroslaw
> > > > > Sent: Tuesday, December 13, 2016 1:08 AM
> > > > > To: dev at dpdk.org
> > > > > Subject: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict
> > > > >
> > > > > Signed-off-by: Michał Mirosław <michal.miroslaw at 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()? 
  
Konstantin