[dpdk-dev,v3,05/18] librte_acl: fix a bug at build phase that can cause matches beeing overwirtten.

Message ID 1421779267-18492-6-git-send-email-konstantin.ananyev@intel.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Ananyev, Konstantin Jan. 20, 2015, 6:40 p.m. UTC
  Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 lib/librte_acl/acl_bld.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Neil Horman Jan. 25, 2015, 5:34 p.m. UTC | #1
On Tue, Jan 20, 2015 at 06:40:54PM +0000, Konstantin Ananyev wrote:
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  lib/librte_acl/acl_bld.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_acl/acl_bld.c b/lib/librte_acl/acl_bld.c
> index 8bf4a54..22f7934 100644
> --- a/lib/librte_acl/acl_bld.c
> +++ b/lib/librte_acl/acl_bld.c
> @@ -1907,7 +1907,7 @@ rte_acl_build(struct rte_acl_ctx *ctx, const struct rte_acl_config *cfg)
>  				bcx.num_tries, bcx.cfg.num_categories,
>  				RTE_ACL_MAX_FIELDS * RTE_DIM(bcx.tries) *
>  				sizeof(ctx->data_indexes[0]),
> -				bcx.num_build_rules);
> +				bcx.num_build_rules + 1);
>  		if (rc == 0) {
>  
>  			/* set data indexes. */
> -- 
> 1.8.5.3
> 
> 
Shouldn't you add to num_build_rules inside rte_acl_gen?  That way other future
users of the function don't have to remember to do so.
Neil
  
Ananyev, Konstantin Jan. 25, 2015, 10:40 p.m. UTC | #2
Hi Neil,

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Sunday, January 25, 2015 5:35 PM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 05/18] librte_acl: fix a bug at build phase that can cause matches beeing overwirtten.
> 
> On Tue, Jan 20, 2015 at 06:40:54PM +0000, Konstantin Ananyev wrote:
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> >  lib/librte_acl/acl_bld.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_acl/acl_bld.c b/lib/librte_acl/acl_bld.c
> > index 8bf4a54..22f7934 100644
> > --- a/lib/librte_acl/acl_bld.c
> > +++ b/lib/librte_acl/acl_bld.c
> > @@ -1907,7 +1907,7 @@ rte_acl_build(struct rte_acl_ctx *ctx, const struct rte_acl_config *cfg)
> >  				bcx.num_tries, bcx.cfg.num_categories,
> >  				RTE_ACL_MAX_FIELDS * RTE_DIM(bcx.tries) *
> >  				sizeof(ctx->data_indexes[0]),
> > -				bcx.num_build_rules);
> > +				bcx.num_build_rules + 1);
> >  		if (rc == 0) {
> >
> >  			/* set data indexes. */
> > --
> > 1.8.5.3
> >
> >
> Shouldn't you add to num_build_rules inside rte_acl_gen?  That way other future
> users of the function don't have to remember to do so.

In that patch, I just fix the bug to stop generate invalid tries for some corener cases.
In the later patch in that set, I did something similar to what you are suggesting here -
make rte_acl_gen() to allocate indexes for all match nodes too (as it already doing for all other nodes).
See  [PATCH v3 07/18] librte_acl: build/gen phase - simplify the way match nodes are allocated.

Konstantin

> Neil
  
Neil Horman Jan. 26, 2015, 12:08 p.m. UTC | #3
On Sun, Jan 25, 2015 at 10:40:23PM +0000, Ananyev, Konstantin wrote:
> Hi Neil,
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Sunday, January 25, 2015 5:35 PM
> > To: Ananyev, Konstantin
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 05/18] librte_acl: fix a bug at build phase that can cause matches beeing overwirtten.
> > 
> > On Tue, Jan 20, 2015 at 06:40:54PM +0000, Konstantin Ananyev wrote:
> > > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > ---
> > >  lib/librte_acl/acl_bld.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/librte_acl/acl_bld.c b/lib/librte_acl/acl_bld.c
> > > index 8bf4a54..22f7934 100644
> > > --- a/lib/librte_acl/acl_bld.c
> > > +++ b/lib/librte_acl/acl_bld.c
> > > @@ -1907,7 +1907,7 @@ rte_acl_build(struct rte_acl_ctx *ctx, const struct rte_acl_config *cfg)
> > >  				bcx.num_tries, bcx.cfg.num_categories,
> > >  				RTE_ACL_MAX_FIELDS * RTE_DIM(bcx.tries) *
> > >  				sizeof(ctx->data_indexes[0]),
> > > -				bcx.num_build_rules);
> > > +				bcx.num_build_rules + 1);
> > >  		if (rc == 0) {
> > >
> > >  			/* set data indexes. */
> > > --
> > > 1.8.5.3
> > >
> > >
> > Shouldn't you add to num_build_rules inside rte_acl_gen?  That way other future
> > users of the function don't have to remember to do so.
> 
> In that patch, I just fix the bug to stop generate invalid tries for some corener cases.
> In the later patch in that set, I did something similar to what you are suggesting here -
> make rte_acl_gen() to allocate indexes for all match nodes too (as it already doing for all other nodes).
> See  [PATCH v3 07/18] librte_acl: build/gen phase - simplify the way match nodes are allocated.
> 
Ok, thank you
Neil

> Konstantin
> 
> > Neil
> 
>
  

Patch

diff --git a/lib/librte_acl/acl_bld.c b/lib/librte_acl/acl_bld.c
index 8bf4a54..22f7934 100644
--- a/lib/librte_acl/acl_bld.c
+++ b/lib/librte_acl/acl_bld.c
@@ -1907,7 +1907,7 @@  rte_acl_build(struct rte_acl_ctx *ctx, const struct rte_acl_config *cfg)
 				bcx.num_tries, bcx.cfg.num_categories,
 				RTE_ACL_MAX_FIELDS * RTE_DIM(bcx.tries) *
 				sizeof(ctx->data_indexes[0]),
-				bcx.num_build_rules);
+				bcx.num_build_rules + 1);
 		if (rc == 0) {
 
 			/* set data indexes. */