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

Michal Miroslaw mirq-linux at rere.qmqm.pl
Tue Dec 13 17:14:09 CET 2016


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 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()? 

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.

Yes, people can work around it by loosing 1 of 2^32 useful values and
convoluting their 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?

Best Regards,
Michał Mirosław


More information about the dev mailing list