[dpdk-dev] fix lpm bugs

  hi:
    We test some lpm cases and find some bugs, below is how to fix it. thanks :)
---
 lib/librte_lpm/rte_lpm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


                        [sub_rule_index].next_hop,
@@ -781,7 +782,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
                                        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.8.5.2 (Apple Git-48)
  

On Wed, Oct 21, 2015 at 05:54:13PM +0800, mablexidana wrote:
> hi:
>     We test some lpm cases and find some bugs, below is how to fix it. thanks :)

Hi,

thanks for the patch. Could you perhaps provide a description of how to reproduce
the bug (or bugs you are fixing), so that we can reproduce them and verify the
fix. (A unit test added to the existing lpm unit tests for this would be the 
best solution.)
For the patch itself, the commit message should also describe the bug, and
how the patch fixes it. It's also good to include a one-line "Fixes:" line
in the comment - generated by using the git alias "fixline" added as:
	fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")'

Regards,
/Bruce

> ---
>  lib/librte_lpm/rte_lpm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index 163ba3c..b5199ff 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -735,7 +735,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
>                                         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
> @@ -770,6 +770,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,
> @@ -781,7 +782,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
>                                         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.8.5.2 (Apple Git-48)
  

hi:
    Fixes: 25e4f515fe63 ("fix lpm bugs")




   the random test of lpm , multiple delete and add ip address, it do not recover the last right ip address.
  eg1: 
   add a lot of routes:
 rule id : 1, ip : 16.32.0.0/19, next_hop : 62, 
rule id : 2, ip : 16.32.28.0/22, next_hop : 97,
rule id :  28, ip:16.32.0.0/21, next_hop :36
.....
when you delete rule id 3, then lookup 16.32.0.150, the return is 16.32.28.0/22,but not 16.32.0.0/19. this is because in delete_depth_small function, when lpm->tbl24[i].ext_entry == 0 and lpm->tbl24[i].depth > depth, it will run into the  tbl8 process.then the next_hop will be doing as tbl8_gindex, and the lpm->tbl8[j] data is being wrong processed.
fix: + else if (lpm->tbl24[i].ext_entry == 1) {


eg2:
when add ,delete and add again, it will also has problem.
in delete_depth_small function, the valid_group of new struct rte_lpm_tbl8_entry is INVALID, so when process  lpm->tbl8[j] = new_tbl8_entry, the valid_group is covered. and when just add a route depth > 24,  and  alloc a tbl8 index, then the tbl8_alloc will return it as new index, then the data is being wrong rewrite.
fix:+ .valid_group = VALID,




thanks.    I will provide the testing program later .




regards


yuerxin
   













At 2015-10-21 19:07:49, "Bruce Richardson" <bruce.richardson@intel.com> wrote:
>On Wed, Oct 21, 2015 at 05:54:13PM +0800, mablexidana wrote:
>> hi:
>>     We test some lpm cases and find some bugs, below is how to fix it. thanks :)
>
>Hi,
>
>thanks for the patch. Could you perhaps provide a description of how to reproduce
>the bug (or bugs you are fixing), so that we can reproduce them and verify the
>fix. (A unit test added to the existing lpm unit tests for this would be the 
>best solution.)
>For the patch itself, the commit message should also describe the bug, and
>how the patch fixes it. It's also good to include a one-line "Fixes:" line
>in the comment - generated by using the git alias "fixline" added as:
>	fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")'
>
>Regards,
>/Bruce
>
>> ---
>>  lib/librte_lpm/rte_lpm.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> 
>> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
>> index 163ba3c..b5199ff 100644
>> --- a/lib/librte_lpm/rte_lpm.c
>> +++ b/lib/librte_lpm/rte_lpm.c
>> @@ -735,7 +735,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
>>                                         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
>> @@ -770,6 +770,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,
>> @@ -781,7 +782,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
>>                                         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.8.5.2 (Apple Git-48)
  

hi:
This is test on dpdk version 2.1


Regards 




yuerxin


t 2015-10-22 10:15:16, "mablexidana" <mablexidana@163.com> wrote:
hi:
    Fixes: 25e4f515fe63 ("fix lpm bugs")




   the random test of lpm , multiple delete and add ip address, it do not recover the last right ip address.
  eg1: 
   add a lot of routes:
 rule id : 1, ip : 16.32.0.0/19, next_hop : 62, 
rule id : 2, ip : 16.32.28.0/22, next_hop : 97,
rule id :  28, ip:16.32.0.0/21, next_hop :36
.....
when you delete rule id 3, then lookup 16.32.0.150, the return is 16.32.28.0/22,but not 16.32.0.0/19. this is because in delete_depth_small function, when lpm->tbl24[i].ext_entry == 0 and lpm->tbl24[i].depth > depth, it will run into the  tbl8 process.then the next_hop will be doing as tbl8_gindex, and the lpm->tbl8[j] data is being wrong processed.
fix: + else if (lpm->tbl24[i].ext_entry == 1) {


eg2:
when add ,delete and add again, it will also has problem.
in delete_depth_small function, the valid_group of new struct rte_lpm_tbl8_entry is INVALID, so when process  lpm->tbl8[j] = new_tbl8_entry, the valid_group is covered. and when just add a route depth > 24,  and  alloc a tbl8 index, then the tbl8_alloc will return it as new index, then the data is being wrong rewrite.
fix:+ .valid_group = VALID,




thanks.    I will provide the testing program later .




regards


yuerxin
   













At 2015-10-21 19:07:49, "Bruce Richardson" <bruce.richardson@intel.com> wrote:
>On Wed, Oct 21, 2015 at 05:54:13PM +0800, mablexidana wrote:
>> hi:
>>     We test some lpm cases and find some bugs, below is how to fix it. thanks :)
>
>Hi,
>
>thanks for the patch. Could you perhaps provide a description of how to reproduce
>the bug (or bugs you are fixing), so that we can reproduce them and verify the
>fix. (A unit test added to the existing lpm unit tests for this would be the 
>best solution.)
>For the patch itself, the commit message should also describe the bug, and
>how the patch fixes it. It's also good to include a one-line "Fixes:" line
>in the comment - generated by using the git alias "fixline" added as:
>	fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")'
>
>Regards,
>/Bruce
>
>> ---
>>  lib/librte_lpm/rte_lpm.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> 
>> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
>> index 163ba3c..b5199ff 100644
>> --- a/lib/librte_lpm/rte_lpm.c
>> +++ b/lib/librte_lpm/rte_lpm.c
>> @@ -735,7 +735,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
>>                                         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
>> @@ -770,6 +770,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,
>> @@ -781,7 +782,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
>>                                         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.8.5.2 (Apple Git-48)
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of mablexidana
> Sent: Friday, October 23, 2015 8:48 AM
> To: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] fix lpm bugs
> 
> hi:
> This is test on dpdk version 2.1

Hi,

Thanks for that. The patch needs to be signed and isn't quite in the correct format.

See the following for some quidelines on how to format/submit a patch.

    http://dpdk.org/dev/patchwork/patch/7762/

If you need some extra text to describe the test to describe the test setup, etc., you can use the cover letter or a follow-up reply. The commit message should still contain the issue(s) that is fixed and how to re-produce it.

See the above doc for full details.

John.
--