[dpdk-dev,04/13] acl: allow zero verdict
Checks
Commit Message
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(-)
Comments
Hi Michal,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michal Miroslaw
> Sent: Tuesday, December 13, 2016 1:08 AM
> To: dev@dpdk.org
> 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.
Konstantin
> diff --git a/lib/librte_acl/rte_acl.h b/lib/librte_acl/rte_acl.h
> index caa91f7..b53179a 100644
> --- a/lib/librte_acl/rte_acl.h
> +++ b/lib/librte_acl/rte_acl.h
> @@ -120,8 +120,6 @@ enum {
> RTE_ACL_MIN_PRIORITY = 0,
> };
>
> -#define RTE_ACL_INVALID_USERDATA 0
> -
> #define RTE_ACL_MASKLEN_TO_BITMASK(v, s) \
> ((v) == 0 ? (v) : (typeof(v))((uint64_t)-1 << ((s) * CHAR_BIT - (v))))
>
> diff --git a/lib/librte_table/rte_table_acl.c b/lib/librte_table/rte_table_acl.c
> index 8f1f8ce..94b69a9 100644
> --- a/lib/librte_table/rte_table_acl.c
> +++ b/lib/librte_table/rte_table_acl.c
> @@ -792,7 +792,7 @@ rte_table_acl_lookup(
>
> pkts_mask &= ~pkt_mask;
>
> - if (action_table_pos != RTE_ACL_INVALID_USERDATA) {
> + if (action_table_pos != 0) {
> pkts_out_mask |= pkt_mask;
> entries[pkt_pos] = (void *)
> &acl->memory[action_table_pos *
> --
> 2.10.2
On Tue, Dec 13, 2016 at 10:36:16AM +0000, Ananyev, Konstantin wrote:
> Hi Michal,
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michal Miroslaw
> > Sent: Tuesday, December 13, 2016 1:08 AM
> > To: dev@dpdk.org
> > 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.
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.
Best Regards,
Michał Mirosław
> -----Original Message-----
> From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> Sent: Tuesday, December 13, 2016 1:55 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 10:36:16AM +0000, Ananyev, Konstantin wrote:
> > Hi Michal,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michal Miroslaw
> > > Sent: Tuesday, December 13, 2016 1:08 AM
> > > To: dev@dpdk.org
> > > 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'.
Konstantin
On Tue, Dec 13, 2016 at 02:14:19PM +0000, Ananyev, Konstantin wrote:
> > -----Original Message-----
> > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> > Sent: Tuesday, December 13, 2016 1:55 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 10:36:16AM +0000, Ananyev, Konstantin wrote:
> > > Hi Michal,
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michal Miroslaw
> > > > Sent: Tuesday, December 13, 2016 1:08 AM
> > > > To: dev@dpdk.org
> > > > 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. If I all my rules have non-zero userdata,
then this patch changes nothing. 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.
Best Regards,
Michał Mirosław
> -----Original Message-----
> From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> Sent: Tuesday, December 13, 2016 2:53 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 02:14:19PM +0000, Ananyev, Konstantin wrote:
> > > -----Original Message-----
> > > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> > > Sent: Tuesday, December 13, 2016 1:55 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 10:36:16AM +0000, Ananyev, Konstantin wrote:
> > > > Hi Michal,
> > > >
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michal Miroslaw
> > > > > Sent: Tuesday, December 13, 2016 1:08 AM
> > > > > To: dev@dpdk.org
> > > > > 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()?
Konstantin
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.
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
On Tue, Dec 13, 2016 at 05:14:09PM +0100, Michal Miroslaw wrote:
> On Tue, Dec 13, 2016 at 03:13:42PM +0000, Ananyev, Konstantin wrote:
[...]
> > > 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?
I just noticed that it's probably you who wrote most of the ACLs code,
so I guest you're the right person to ask the question above.
Nice work, BTW. :-)
Best Regards,
Michał Mirosław
> -----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.
>
> 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?
>
> 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;
}
}
Konstantin
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).
Best Regards,
Michał Mirosław
> -----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?
Konstantin
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
Hi Michal,
> -----Original Message-----
> From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> Sent: Wednesday, December 14, 2016 2:11 AM
> 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 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.
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
@@ -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;
}
@@ -120,8 +120,6 @@ enum {
RTE_ACL_MIN_PRIORITY = 0,
};
-#define RTE_ACL_INVALID_USERDATA 0
-
#define RTE_ACL_MASKLEN_TO_BITMASK(v, s) \
((v) == 0 ? (v) : (typeof(v))((uint64_t)-1 << ((s) * CHAR_BIT - (v))))
@@ -792,7 +792,7 @@ rte_table_acl_lookup(
pkts_mask &= ~pkt_mask;
- if (action_table_pos != RTE_ACL_INVALID_USERDATA) {
+ if (action_table_pos != 0) {
pkts_out_mask |= pkt_mask;
entries[pkt_pos] = (void *)
&acl->memory[action_table_pos *