[PATCH] *** Memory Allocation: Adding a new UT for fb_array ***

[PATCH] *** Memory Allocation: Adding a new UT for fb_array ***

[Vipin P R]

*** 

Partial output of fbarray_autotest (app/test/dpdk-test --log-level="*:debug") [WITHOUT THE FIX]
..
..
EAL: ms_idx:1096
EAL: ms_idx:1097
EAL: ms_idx:1098
EAL: ms_idx:1099
EAL: ms_idx:1155
EAL: ms_idx:1156
EAL: ms_idx:1158
EAL: ms_idx:1159
EAL: ms_idx:1154
EAL: ms_idx jumping behind. ms_idx: 1154 prev_ms_idx: 1159
EAL: Test assert test_jump line 445 failed: Incorrect ms_idx jump
 + TestCase [ 2] : test_jump failed
 + TestCase [ 3] : test_find succeeded
 + TestCase [ 4] : test_find succeeded
 + TestCase [ 5] : test_find succeeded
 + TestCase [ 6] : test_find succeeded
 + TestCase [ 7] : test_find succeeded
 + TestCase [ 8] : test_empty succeeded
 + ------------------------------------------------------- +
 + Test Suite Summary
 + Tests Total :        9
 + Tests Skipped :      0
 + Tests Executed :     9
 + Tests Unsupported:   0
 + Tests Passed :       8
 + Tests Failed :       1
 + ------------------------------------------------------- +
Test Failed
RTE>>

Partial output of fbarray_autotest (app/test/dpdk-test --log-level="*:debug") [WITH THE FIX]
..
..
EAL: ms_idx:1109
EAL: ms_idx:1110
EAL: ms_idx:1112
 + TestCase [ 2] : test_jump succeeded
 + TestCase [ 3] : test_find succeeded
 + TestCase [ 4] : test_find succeeded
 + TestCase [ 5] : test_find succeeded
 + TestCase [ 6] : test_find succeeded
 + TestCase [ 7] : test_find succeeded
 + TestCase [ 8] : test_empty succeeded
 + ------------------------------------------------------- +
 + Test Suite Summary
 + Tests Total :        9
 + Tests Skipped :      0
 + Tests Executed :     9
 + Tests Unsupported:   0
 + Tests Passed :       9
 + Tests Failed :       0
 + ------------------------------------------------------- +
Test OK
RTE>>
***

Vipin P R (1):
  Memory Allocation: Adding a new UT for fb_array

 app/test/test_fbarray.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

-- 
2.7.4

[PATCH] Memory Allocation: Adding a new UT for fb_array

[Vipin P R]

add test case coverage to cover the ms_idx jump

Cc: stable@dpdk.org

Signed-off-by: Vipin P R <vipinp@vmware.com>
Acked-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
Depends-on: 0001-Memory-Allocation-Fixes-ms_idx-jump-lookahead-during.patch
Depends-on: 0002-Memory-Allocation-Fixes-ms_idx-jump-lookbehind-durin.patch
---
 app/test/test_fbarray.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/app/test/test_fbarray.c b/app/test/test_fbarray.c
index a691bf4..275449c 100644
--- a/app/test/test_fbarray.c
+++ b/app/test/test_fbarray.c
@@ -11,6 +11,7 @@
 #include <rte_debug.h>
 #include <rte_errno.h>
 #include <rte_fbarray.h>
+#include <rte_memory.h>
 
 #include "test.h"
 
@@ -402,6 +403,53 @@ static int check_used_one(void)
 	return 0;
 }
 
+/* the following test case verifies that the jump in ms_idx for an fb-array is correct. */
+static int test_jump(void)
+{
+    struct rte_fbarray test_array;
+    int input[] = {1, 1070, 1, 2, 1, 2, 4, 12, 2, 2, 1, 2, 1};
+    int ms_idx, prev_ms_idx, delta;
+    int len;
+    ms_idx = prev_ms_idx = 0;
+
+    int ret = rte_fbarray_init(&test_array, "test", 32768, sizeof(struct rte_memseg));
+    if (ret == 0) {
+        RTE_LOG(DEBUG, EAL, "FB array init success\n");
+        int k = 0;
+        for(int i=0; i < sizeof(input)/sizeof(int); i++) {
+            if (i == 0) {
+                len = input[i];
+            } else {
+                len = input[i] + 1;
+            }
+            prev_ms_idx = ms_idx;
+            ms_idx = rte_fbarray_find_next_n_free(&test_array, k, len);
+
+            if (i != 0) {
+                ms_idx++;
+            }
+
+            for (int j=0; j < input[i]; j++) {
+                RTE_LOG(DEBUG, EAL, "ms_idx:%d\n", ms_idx);
+                rte_fbarray_set_used(&test_array, ms_idx);
+                ms_idx++;
+            }
+
+            if (prev_ms_idx) {
+                /* The value of ms_idx should be monotonically increasing
+                 * given the above input sequence in test_array.
+                 * */
+                delta = ms_idx - prev_ms_idx;
+                if (!(delta > 0)) {
+                    RTE_LOG(ERR, EAL, "ms_idx jumping behind. ms_idx: %d prev_ms_idx: %d\n", ms_idx - 1, prev_ms_idx - 1);
+                    TEST_ASSERT(0, "Incorrect ms_idx jump");
+                }
+            }
+        }
+    }
+    return 0;
+}
+
 static int test_basic(void)
 {
 	const int idx = 0;
@@ -717,6 +765,7 @@ static struct unit_test_suite fbarray_test_suite = {
 	.unit_test_cases = {
 		TEST_CASE(test_invalid),
 		TEST_CASE(test_basic),
+		TEST_CASE(test_jump),
 		TEST_CASE_ST(first_msk_test_setup, reset_array, test_find),
 		TEST_CASE_ST(cross_msk_test_setup, reset_array, test_find),
 		TEST_CASE_ST(multi_msk_test_setup, reset_array, test_find),
-- 
2.7.4

Re: [PATCH] Memory Allocation: Adding a new UT for fb_array

[Stephen Hemminger]

On Fri, 13 Jan 2023 13:12:47 +0000
Vipin P R <vipinp@vmware.com> wrote:

> add test case coverage to cover the ms_idx jump
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Vipin P R <vipinp@vmware.com>
> Acked-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
> Depends-on: 0001-Memory-Allocation-Fixes-ms_idx-jump-lookahead-during.patch
> Depends-on: 0002-Memory-Allocation-Fixes-ms_idx-jump-lookbehind-durin.patch

This looks like a good idea but lots of style errors on this patch.
Please run checkpatch, fix and resubmit.

Re: [PATCH] Memory Allocation: Adding a new UT for fb_array

[Burakov, Anatoly]

Hi Vipin!

Thanks for all of the work on this bug, it is highly appreciated. Below 
are suggestions for improvements for this patch.

On 1/13/2023 1:12 PM, Vipin P R wrote:
> add test case coverage to cover the ms_idx jump

This message could be expanded to be more informative. Suggested rewording:

test/fbarray: add test case for incorrect lookahead behavior

> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Vipin P R <vipinp@vmware.com>
> Acked-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
> Depends-on: 0001-Memory-Allocation-Fixes-ms_idx-jump-lookahead-during.patch
> Depends-on: 0002-Memory-Allocation-Fixes-ms_idx-jump-lookbehind-durin.patch

This makes no difference for commit, but for future reference: 
depends-on should reference link to actual patches, not a patch file name.

> ---
>   app/test/test_fbarray.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
> 
> diff --git a/app/test/test_fbarray.c b/app/test/test_fbarray.c
> index a691bf4..275449c 100644
> --- a/app/test/test_fbarray.c
> +++ b/app/test/test_fbarray.c
> @@ -11,6 +11,7 @@
>   #include <rte_debug.h>
>   #include <rte_errno.h>
>   #include <rte_fbarray.h>
> +#include <rte_memory.h>

This is presumably added to get access to `struct rte_memseg`, but this 
is not needed, because the bug is in the mask behavior, which does not 
depend on specific data size.

>   
>   #include "test.h"
>   
> @@ -402,6 +403,53 @@ static int check_used_one(void)
>   	return 0;
>   }
>   
> +/* the following test case verifies that the jump in ms_idx for an fb-array is correct. */
> +static int test_jump(void)
> +{
> +    struct rte_fbarray test_array;
> +    int input[] = {1, 1070, 1, 2, 1, 2, 4, 12, 2, 2, 1, 2, 1};

I've managed to reduce this bug down to a more minimal example:

{ 63, 1, 2 }

> +    int ms_idx, prev_ms_idx, delta;
> +    int len;
> +    ms_idx = prev_ms_idx = 0;
> +
> +    int ret = rte_fbarray_init(&test_array, "test", 32768, sizeof(struct rte_memseg));
> +    if (ret == 0) {
> +        RTE_LOG(DEBUG, EAL, "FB array init success\n");

If the code did an early exit, an additional indentation level could've 
been avoided, like so:

	TEST_ASSERT(rte_fbarray_init(&test_array, "test", 256, 8) == 0,
			"Failed to initialize fbarray\n");

Also, missing corresponding `rte_fbarray_destroy` call.

> +        int k = 0;

Seems like the only place where this is used is in find_next_n_free, and 
it never changes, so I don't think this variable is needed at all.

> +        for(int i=0; i < sizeof(input)/sizeof(int); i++) {

RTE_DIM? Also, array indices are `unsigned int` rather than `int`, 
compiler gives a warning.

> +            if (i == 0) {
> +                len = input[i];
> +            } else {
> +                len = input[i] + 1;
> +            }

All of this could be rewritten as follows:

	int len, hole;

	/* if this is not the first iteration, create a hole */
	hole = i != 0;
	len = input[i] + hole;

> +            prev_ms_idx = ms_idx;
> +            ms_idx = rte_fbarray_find_next_n_free(&test_array, k, len);

Like I said above, `k` is unneeded, we can just replace it with 0.

> +
> +            if (i != 0) {
> +                ms_idx++;
> +            }

Given suggestion above, could use `if (hole)` instead, would be more 
readable.

> +
> +            for (int j=0; j < input[i]; j++) {

Array indices are unsigned, and also could replace with

	for (unsigned int j = hole; j < len; j++)

IMO would be more readable.

> +                RTE_LOG(DEBUG, EAL, "ms_idx:%d\n", ms_idx);

I don't think this log is needed.

> +                rte_fbarray_set_used(&test_array, ms_idx);
> +                ms_idx++;
> +            }
> +
> +            if (prev_ms_idx) {
> +                /* The value of ms_idx should be monotonically increasing
> +                 * given the above input sequence in test_array.
> +                 * */
> +                delta = ms_idx - prev_ms_idx;
> +                if (!(delta > 0)) {

Given above suggestions, this can be replaced with `if (delta != len)`. 
Also, given the `TEST_ASSERT(0)` below, I think this could just be 
replaced with an assert and a message, e.g.

	TEST_ASSERT(delta == len, "Incorrect fbarray index\n");

-- 
Thanks,
Anatoly

Re: [PATCH] Memory Allocation: Adding a new UT for fb_array

[Burakov, Anatoly]

On 5/16/2023 2:39 PM, Burakov, Anatoly wrote:
> Hi Vipin!
> 
> Thanks for all of the work on this bug, it is highly appreciated. Below 
> are suggestions for improvements for this patch.
> 
> On 1/13/2023 1:12 PM, Vipin P R wrote:
>> add test case coverage to cover the ms_idx jump
> 
> This message could be expanded to be more informative. Suggested rewording:
> 
> test/fbarray: add test case for incorrect lookahead behavior
> 
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Vipin P R <vipinp@vmware.com>
>> Acked-by: Kumara Parameshwaran <kparameshwar@vmware.com>
>> ---
>> Depends-on: 
>> 0001-Memory-Allocation-Fixes-ms_idx-jump-lookahead-during.patch
>> Depends-on: 
>> 0002-Memory-Allocation-Fixes-ms_idx-jump-lookbehind-durin.patch
> 
> This makes no difference for commit, but for future reference: 
> depends-on should reference link to actual patches, not a patch file name.
> 
>> ---
>>   app/test/test_fbarray.c | 49 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>
>> diff --git a/app/test/test_fbarray.c b/app/test/test_fbarray.c
>> index a691bf4..275449c 100644
>> --- a/app/test/test_fbarray.c
>> +++ b/app/test/test_fbarray.c
>> @@ -11,6 +11,7 @@
>>   #include <rte_debug.h>
>>   #include <rte_errno.h>
>>   #include <rte_fbarray.h>
>> +#include <rte_memory.h>
> 
> This is presumably added to get access to `struct rte_memseg`, but this 
> is not needed, because the bug is in the mask behavior, which does not 
> depend on specific data size.
> 
>>   #include "test.h"
>> @@ -402,6 +403,53 @@ static int check_used_one(void)
>>       return 0;
>>   }
>> +/* the following test case verifies that the jump in ms_idx for an 
>> fb-array is correct. */
>> +static int test_jump(void)

I think the test functions would be better named "test_lookahead" and 
"test_lookbehind" respectively.

>> +{
>> +    struct rte_fbarray test_array;
>> +    int input[] = {1, 1070, 1, 2, 1, 2, 4, 12, 2, 2, 1, 2, 1};
> 
> I've managed to reduce this bug down to a more minimal example:
> 
> { 63, 1, 2 }
> 


I've managed to reduce the test down to an even more minimal example, so 
all of the other code, loops etc. is actually not needed:

1. Allocate fbarray with 256 entries
2. Set idx 64 as used
3. Call rte_fbarray_find_next_n_free() starting with index 1 and length 
of 64

Returned value should be 65, but without the fix it returns 129.

-- 
Thanks,
Anatoly