[dpdk-dev] [PATCH v2 2/2] lib/lpm:fix an initialization issue of valid_group in the delete_depth_small()
> -Original Message- > From: Richardson, Bruce > Sent: Friday, October 30, 2015 10:22 PM > To: Liu, Jijiang > Cc: dev at 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 > > > > --- > > 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 > >
[dpdk-dev] [PATCH v2 2/2] lib/lpm:fix an initialization issue of valid_group in the delete_depth_small()
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 --- 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
[dpdk-dev] [PATCH v2 2/2] lib/lpm:fix an initialization issue of valid_group in the delete_depth_small()
2015-10-30 14:24, Bruce Richardson: > On Fri, Oct 30, 2015 at 02:22:27PM +, 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 > > > > 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.
[dpdk-dev] [PATCH v2 2/2] lib/lpm:fix an initialization issue of valid_group in the delete_depth_small()
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > > 2015-10-30 14:24, Bruce Richardson: > > On Fri, Oct 30, 2015 at 02:22:27PM +, 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 > > > > > > 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" ??
[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 02:22:27PM +, 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 > > Assuming we get a little cleanup on commit title and log message (Thomas, perhaps just a rewrite on commit?): Acked-by: Bruce Richardson
[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" > 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 > > --- > 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 >