Memory Allocation: Adding a new UT for fb_array
Checks
Commit Message
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(+)
Comments
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.
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");
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.
@@ -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),