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: sta...@dpdk.org

Signed-off-by: Vipin P R <vip...@vmware.com>
Acked-by: Kumara Parameshwaran <kparamesh...@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

Reply via email to