[dpdk-dev] [PATCH v2 0/2] Fix two issues in lpm

[dpdk-dev] [PATCH v2 0/2] Fix two issues in lpm

[Jijiang Liu]

Fixes two issues in the delete_depth_small() function.

v2 changes:
  Split a patch into two patches for two issues.
  Add more clear issue description. 


*** BLURB HERE ***

Jijiang Liu (2):
  fix an issue of condition check in delete_depth_small().
  fix an initialization issue of valid_group in the delete_depth_small()

 lib/librte_lpm/rte_lpm.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

-- 
1.7.7.6

[dpdk-dev] [PATCH v2 1/2] lib/lpm:fix an issue of condition check in delete_depth_small()

[Jijiang Liu]

Fixes an issue of check logic in delete_depth_small function.

For a tbl24 entry, the 'ext_entry' field indicates whether we need to use tbl8_gindex to read the next_hop from a tbl8 entry, or whether it can be read directly from this entry.

If a route is deleted, the prefix of previous route is used to override the deleted route.

When checking the depth of the previous route the conditional checks both the ext_entry and the depth, but the "else" leg fails to take account that the condition could fail for one of two possible reasons, leading to an incorrect flow when 'ext_entry == 0' is true , but 'lpm->tbl24[i].depth > depth' is false. The fix here is to add a condition check to the else leg so that it only executes when ext_entry is set.

Signed-off-by: NaNa <nana.nn@alibaba-inc.com>

---
 lib/librte_lpm/rte_lpm.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 163ba3c..57ec2f0 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -734,8 +734,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 			if (lpm->tbl24[i].ext_entry == 0 &&
 					lpm->tbl24[i].depth <= depth ) {
 				lpm->tbl24[i].valid = INVALID;
-			}
-			else {
+			} else if (lpm->tbl24[i].ext_entry == 1) {
 				/*
 				 * If TBL24 entry is extended, then there has
 				 * to be a rule with depth >= 25 in the
@@ -780,8 +779,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 			if (lpm->tbl24[i].ext_entry == 0 &&
 					lpm->tbl24[i].depth <= depth ) {
 				lpm->tbl24[i] = new_tbl24_entry;
-			}
-			else {
+			} else  if (lpm->tbl24[i].ext_entry == 1) {
 				/*
 				 * If TBL24 entry is extended, then there has
 				 * to be a rule with depth >= 25 in the
-- 
1.7.7.6

Re: [dpdk-dev] [PATCH v2 1/2] lib/lpm:fix an issue of condition check in delete_depth_small()

[Bruce Richardson]

On Fri, Oct 30, 2015 at 09:14:38PM +0800, Jijiang Liu wrote:
> Fixes an issue of check logic in delete_depth_small function.
> 
> For a tbl24 entry, the 'ext_entry' field indicates whether we need to use tbl8_gindex to read the next_hop from a tbl8 entry, or whether it can be read directly from this entry.
> 
> If a route is deleted, the prefix of previous route is used to override the deleted route.
> 
> When checking the depth of the previous route the conditional checks both the ext_entry and the depth, but the "else" leg fails to take account that the condition could fail for one of two possible reasons, leading to an incorrect flow when 'ext_entry == 0' is true , but 'lpm->tbl24[i].depth > depth' is false. The fix here is to add a condition check to the else leg so that it only executes when ext_entry is set.
> 
> Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[dpdk-dev] [PATCH v2 2/2] lib/lpm:fix an initialization issue of valid_group in the delete_depth_small()

[Jijiang Liu]

Fixes an initialization issue of 'valid_group' in the delete_depth_small function.

In this function, use new rte_lpm_tbl8_entry we call A to replace the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it 
will be INVALID.

Then when adding a new route which depth is > 24,the tbl8_alloc() function will search the rte_lpm_tbl8_entrys to find INVALID 
valid_group, and it will return the A to the add_depth_big function, so A's data is overridden.

Signed-off-by: NaNa <nana.nn@alibaba-inc.com>

---
 lib/librte_lpm/rte_lpm.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 57ec2f0..3981452 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -769,6 +769,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 
 		struct rte_lpm_tbl8_entry new_tbl8_entry = {
 			.valid = VALID,
+			.valid_group = VALID,
 			.depth = sub_rule_depth,
 			.next_hop = lpm->rules_tbl
 			[sub_rule_index].next_hop,
-- 
1.7.7.6

Re: [dpdk-dev] [PATCH v2 2/2] lib/lpm:fix an initialization issue of valid_group in the delete_depth_small()

[Bruce Richardson]

On Fri, Oct 30, 2015 at 09:14:39PM +0800, Jijiang Liu wrote:

Title can be shortened to: "lpm: fix initialization of valid_group field"

> Fixes an initialization issue of 'valid_group' in the delete_depth_small function.
> 
> In this function, use new rte_lpm_tbl8_entry we call A to replace the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it 
> will be INVALID.
> 
> Then when adding a new route which depth is > 24,the tbl8_alloc() function will search the rte_lpm_tbl8_entrys to find INVALID 
> valid_group, and it will return the A to the add_depth_big function, so A's data is overridden.
> 

Not sure this message is entirely clear.
How about:
  When adding an entry to a tbl8, the .valid_group field should always be set,
  so that future adds do not accidently find and use this table, thinking it is
  currently invalid, i.e. unused, and thereby overwrite existing entries.

> Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
> 
> ---
>  lib/librte_lpm/rte_lpm.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index 57ec2f0..3981452 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -769,6 +769,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
>  
>  		struct rte_lpm_tbl8_entry new_tbl8_entry = {
>  			.valid = VALID,
> +			.valid_group = VALID,
>  			.depth = sub_rule_depth,
>  			.next_hop = lpm->rules_tbl
>  			[sub_rule_index].next_hop,
> -- 
> 1.7.7.6
> 

Re: [dpdk-dev] [PATCH v2 2/2] lib/lpm:fix an initialization issue of valid_group in the delete_depth_small()

[Bruce Richardson]

On Fri, Oct 30, 2015 at 02:22:27PM +0000, Bruce Richardson wrote:
> On Fri, Oct 30, 2015 at 09:14:39PM +0800, Jijiang Liu wrote:
> 
> Title can be shortened to: "lpm: fix initialization of valid_group field"
> 
> > Fixes an initialization issue of 'valid_group' in the delete_depth_small function.
> > 
> > In this function, use new rte_lpm_tbl8_entry we call A to replace the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it 
> > will be INVALID.
> > 
> > Then when adding a new route which depth is > 24,the tbl8_alloc() function will search the rte_lpm_tbl8_entrys to find INVALID 
> > valid_group, and it will return the A to the add_depth_big function, so A's data is overridden.
> > 
> 
> Not sure this message is entirely clear.
> How about:
>   When adding an entry to a tbl8, the .valid_group field should always be set,
>   so that future adds do not accidently find and use this table, thinking it is
>   currently invalid, i.e. unused, and thereby overwrite existing entries.
> 
> > Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
> > 
Assuming we get a little cleanup on commit title and log message (Thomas, perhaps
just a rewrite on commit?):

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v2 2/2] lib/lpm:fix an initialization issue of valid_group in the delete_depth_small()

[Thomas Monjalon]

2015-10-30 14:24, Bruce Richardson:
> On Fri, Oct 30, 2015 at 02:22:27PM +0000, Bruce Richardson wrote:
> > On Fri, Oct 30, 2015 at 09:14:39PM +0800, Jijiang Liu wrote:
> > 
> > Title can be shortened to: "lpm: fix initialization of valid_group field"
> > 
> > > Fixes an initialization issue of 'valid_group' in the delete_depth_small function.
> > > 
> > > In this function, use new rte_lpm_tbl8_entry we call A to replace the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it 
> > > will be INVALID.
> > > 
> > > Then when adding a new route which depth is > 24,the tbl8_alloc() function will search the rte_lpm_tbl8_entrys to find INVALID 
> > > valid_group, and it will return the A to the add_depth_big function, so A's data is overridden.
> > > 
> > 
> > Not sure this message is entirely clear.
> > How about:
> >   When adding an entry to a tbl8, the .valid_group field should always be set,
> >   so that future adds do not accidently find and use this table, thinking it is
> >   currently invalid, i.e. unused, and thereby overwrite existing entries.
> > 
> > > Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
> > > 
> Assuming we get a little cleanup on commit title and log message (Thomas, perhaps
> just a rewrite on commit?):

Giving the name of a field in the title is not really useful for the overview.
It's better to talk about the use case which is fixed.

Re: [dpdk-dev] [PATCH v2 2/2] lib/lpm:fix an initialization issue of valid_group in the delete_depth_small()

[Richardson, Bruce]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> 
> 2015-10-30 14:24, Bruce Richardson:
> > On Fri, Oct 30, 2015 at 02:22:27PM +0000, Bruce Richardson wrote:
> > > On Fri, Oct 30, 2015 at 09:14:39PM +0800, Jijiang Liu wrote:
> > >
> > > Title can be shortened to: "lpm: fix initialization of valid_group
> field"
> > >
> > > > Fixes an initialization issue of 'valid_group' in the
> delete_depth_small function.
> > > >
> > > > In this function, use new rte_lpm_tbl8_entry we call A to replace
> > > > the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so
> it will be INVALID.
> > > >
> > > > Then when adding a new route which depth is > 24,the tbl8_alloc()
> > > > function will search the rte_lpm_tbl8_entrys to find INVALID
> valid_group, and it will return the A to the add_depth_big function, so
> A's data is overridden.
> > > >
> > >
> > > Not sure this message is entirely clear.
> > > How about:
> > >   When adding an entry to a tbl8, the .valid_group field should always
> be set,
> > >   so that future adds do not accidently find and use this table,
> thinking it is
> > >   currently invalid, i.e. unused, and thereby overwrite existing
> entries.
> > >
> > > > Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
> > > >
> > Assuming we get a little cleanup on commit title and log message
> > (Thomas, perhaps just a rewrite on commit?):
> 
> Giving the name of a field in the title is not really useful for the
> overview.
> It's better to talk about the use case which is fixed.

"lpm: fix incorrect reuse of already allocated tbl8" ??

Re: [dpdk-dev] [PATCH v2 2/2] lib/lpm:fix an initialization issue of valid_group in the delete_depth_small()

[Liu, Jijiang]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Friday, October 30, 2015 10:22 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 2/2] lib/lpm:fix an initialization issue of
> valid_group in the delete_depth_small()
> 
> On Fri, Oct 30, 2015 at 09:14:39PM +0800, Jijiang Liu wrote:
> 
> Title can be shortened to: "lpm: fix initialization of valid_group field"
Ok
> > Fixes an initialization issue of 'valid_group' in the delete_depth_small
> function.
> >
> > In this function, use new rte_lpm_tbl8_entry we call A to replace the
> > old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it will be
> INVALID.
> >
> > Then when adding a new route which depth is > 24,the tbl8_alloc()
> > function will search the rte_lpm_tbl8_entrys to find INVALID valid_group,
> and it will return the A to the add_depth_big function, so A's data is
> overridden.
> >
> 
> Not sure this message is entirely clear.
> How about:
>   When adding an entry to a tbl8, the .valid_group field should always be set,
>   so that future adds do not accidently find and use this table, thinking it is
>   currently invalid, i.e. unused, and thereby overwrite existing entries.
It is ok for me. 
Nana, what do you think?
> > Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
> >
> > ---
> >  lib/librte_lpm/rte_lpm.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c index
> > 57ec2f0..3981452 100644
> > --- a/lib/librte_lpm/rte_lpm.c
> > +++ b/lib/librte_lpm/rte_lpm.c
> > @@ -769,6 +769,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t
> > ip_masked,
> >
> >  		struct rte_lpm_tbl8_entry new_tbl8_entry = {
> >  			.valid = VALID,
> > +			.valid_group = VALID,
> >  			.depth = sub_rule_depth,
> >  			.next_hop = lpm->rules_tbl
> >  			[sub_rule_index].next_hop,
> > --
> > 1.7.7.6
> >

Re: [dpdk-dev] [PATCH v2 0/2] Fix two issues in lpm

[Thomas Monjalon]

2015-10-30 21:14, Jijiang Liu:
> Jijiang Liu (2):
>   fix an issue of condition check in delete_depth_small().
>   fix an initialization issue of valid_group in the delete_depth_small()

There is an authorship issue.
It seems Jijiang is not the author of these patches but the From: line is missing.
Moreover, a real name is expected for author's name and SignedOff lines.
Thanks

Re: [dpdk-dev] [PATCH v2 0/2] Fix two issues in lpm

[Liu, Jijiang]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, November 02, 2015 2:44 AM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 0/2] Fix two issues in lpm
> 
> 2015-10-30 21:14, Jijiang Liu:
> > Jijiang Liu (2):
> >   fix an issue of condition check in delete_depth_small().
> >   fix an initialization issue of valid_group in the delete_depth_small()
> 
> There is an authorship issue.
> It seems Jijiang is not the author of these patches but the From: line is
> missing.
> Moreover, a real name is expected for author's name and SignedOff lines.
> Thanks
Nana from Alibaba is the author, I sent this path set because Nana have not build a usable environment to send path.

Re: [dpdk-dev] [PATCH v2 0/2] Fix two issues in lpm

[Thomas Monjalon]

2015-11-02 08:09, Liu, Jijiang:
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Monday, November 02, 2015 2:44 AM
> > To: Liu, Jijiang
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 0/2] Fix two issues in lpm
> > 
> > 2015-10-30 21:14, Jijiang Liu:
> > > Jijiang Liu (2):
> > >   fix an issue of condition check in delete_depth_small().
> > >   fix an initialization issue of valid_group in the delete_depth_small()
> > 
> > There is an authorship issue.
> > It seems Jijiang is not the author of these patches but the From: line is
> > missing.
> > Moreover, a real name is expected for author's name and SignedOff lines.
> > Thanks
> Nana from Alibaba is the author, I sent this path set because Nana have not build a usable environment to send path.

Jijiang,
Have you understood you must use his full name and make him the author?
	git commit --amend --author=

Re: [dpdk-dev] [PATCH v2 0/2] Fix two issues in lpm

[Liu, Jijiang]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, November 02, 2015 4:26 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 0/2] Fix two issues in lpm
> 
> 2015-11-02 08:09, Liu, Jijiang:
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Sent: Monday, November 02, 2015 2:44 AM
> > > To: Liu, Jijiang
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v2 0/2] Fix two issues in lpm
> > >
> > > 2015-10-30 21:14, Jijiang Liu:
> > > > Jijiang Liu (2):
> > > >   fix an issue of condition check in delete_depth_small().
> > > >   fix an initialization issue of valid_group in the
> > > > delete_depth_small()
> > >
> > > There is an authorship issue.
> > > It seems Jijiang is not the author of these patches but the From:
> > > line is missing.
> > > Moreover, a real name is expected for author's name and SignedOff lines.
> > > Thanks
> > Nana from Alibaba is the author, I sent this path set because Nana have not
> build a usable environment to send path.
> 
> Jijiang,
> Have you understood you must use his full name and make him the author?
> 	git commit --amend --author=
Got it, thanks.