diff mbox series

[v1,1/1] fbarray: fix find_next_n for unaligned length

Message ID 229663bd30b1f64accf51bdefa20f6abf9ee8ad0.1720612158.git.anatoly.burakov@intel.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series [v1,1/1] fbarray: fix find_next_n for unaligned length |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS

Commit Message

Burakov, Anatoly July 10, 2024, 11:49 a.m. UTC 
When array length is not aligned on a power of 2, we need to mask out the
unaligned bits from the mask whenever we reach the last mask. However, when both
ignore mask (e.g. due to starting at unaligned bit) and last index ignore mask
are specified, we combine them with an OR, which is incorrect. Fix it to combine
them with AND instead, and add a unit test covering this case.

The reverse function does not suffer from this issue because it does not have to
deal with array end, and array beginning is always aligned.

Fixes: c44d09811b40 ("eal: add shared indexed file-backed array")
Cc: stable@dpdk.org

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 app/test/test_fbarray.c             | 123 +++++++++++++++++++++-------
 lib/eal/common/eal_common_fbarray.c |   2 +-
 2 files changed, 94 insertions(+), 31 deletions(-)

Comments

David Marchand July 12, 2024, 8:53 a.m. UTC | #1 
Hello,

On Wed, Jul 10, 2024 at 1:49 PM Anatoly Burakov
<anatoly.burakov@intel.com> wrote:
>
> When array length is not aligned on a power of 2, we need to mask out the
> unaligned bits from the mask whenever we reach the last mask. However, when both
> ignore mask (e.g. due to starting at unaligned bit) and last index ignore mask
> are specified, we combine them with an OR, which is incorrect. Fix it to combine
> them with AND instead, and add a unit test covering this case.
>
> The reverse function does not suffer from this issue because it does not have to
> deal with array end, and array beginning is always aligned.
>
> Fixes: c44d09811b40 ("eal: add shared indexed file-backed array")
> Cc: stable@dpdk.org
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Please don't forget to run checkpatch before submitting.
I reformatted the commitlog and applied, thanks Anatoly.
diff mbox series

Patch

diff --git a/app/test/test_fbarray.c b/app/test/test_fbarray.c
index 13c6691e50..09f6907fb1 100644
--- a/app/test/test_fbarray.c
+++ b/app/test/test_fbarray.c
@@ -21,23 +21,41 @@  struct fbarray_testsuite_params {
 };
 
 static struct fbarray_testsuite_params param;
+static struct fbarray_testsuite_params unaligned;
 
 #define FBARRAY_TEST_ARR_NAME "fbarray_autotest"
 #define FBARRAY_TEST_LEN 256
+#define FBARRAY_UNALIGNED_TEST_ARR_NAME "fbarray_unaligned_autotest"
+#define FBARRAY_UNALIGNED_TEST_LEN 60
 #define FBARRAY_TEST_ELT_SZ (sizeof(int))
 
 static int autotest_setup(void)
 {
-	return rte_fbarray_init(&param.arr, FBARRAY_TEST_ARR_NAME,
+	int ret;
+
+	ret = rte_fbarray_init(&param.arr, FBARRAY_TEST_ARR_NAME,
 			FBARRAY_TEST_LEN, FBARRAY_TEST_ELT_SZ);
+	if (ret) {
+		printf("Failed to initialize test array\n");
+		return -1;
+	}
+	ret = rte_fbarray_init(&unaligned.arr, FBARRAY_UNALIGNED_TEST_ARR_NAME,
+			FBARRAY_UNALIGNED_TEST_LEN, FBARRAY_TEST_ELT_SZ);
+	if (ret) {
+		printf("Failed to initialize unaligned test array\n");
+		rte_fbarray_destroy(&param.arr);
+		return -1;
+	}
+	return 0;
 }
 
 static void autotest_teardown(void)
 {
 	rte_fbarray_destroy(&param.arr);
+	rte_fbarray_destroy(&unaligned.arr);
 }
 
-static int init_array(void)
+static int init_aligned(void)
 {
 	int i;
 	for (i = param.start; i <= param.end; i++) {
@@ -47,11 +65,35 @@  static int init_array(void)
 	return 0;
 }
 
-static void reset_array(void)
+static int init_unaligned(void)
+{
+	int i;
+	for (i = unaligned.start; i <= unaligned.end; i++) {
+		if (rte_fbarray_set_used(&unaligned.arr, i))
+			return -1;
+	}
+	return 0;
+}
+
+static void reset_aligned(void)
 {
 	int i;
 	for (i = 0; i < FBARRAY_TEST_LEN; i++)
 		rte_fbarray_set_free(&param.arr, i);
+	/* reset param as well */
+	param.start = -1;
+	param.end = -1;
+}
+
+static void reset_unaligned(void)
+{
+	int i;
+	for (i = 0; i < FBARRAY_UNALIGNED_TEST_LEN; i++)
+		rte_fbarray_set_free(&unaligned.arr, i);
+	/* reset param as well */
+	unaligned.start = -1;
+	unaligned.end = -1;
+
 }
 
 static int first_msk_test_setup(void)
@@ -59,7 +101,7 @@  static int first_msk_test_setup(void)
 	/* put all within first mask */
 	param.start = 3;
 	param.end = 10;
-	return init_array();
+	return init_aligned();
 }
 
 static int cross_msk_test_setup(void)
@@ -67,7 +109,7 @@  static int cross_msk_test_setup(void)
 	/* put all within second and third mask */
 	param.start = 70;
 	param.end = 160;
-	return init_array();
+	return init_aligned();
 }
 
 static int multi_msk_test_setup(void)
@@ -75,7 +117,7 @@  static int multi_msk_test_setup(void)
 	/* put all within first and last mask */
 	param.start = 3;
 	param.end = FBARRAY_TEST_LEN - 20;
-	return init_array();
+	return init_aligned();
 }
 
 static int last_msk_test_setup(void)
@@ -83,7 +125,7 @@  static int last_msk_test_setup(void)
 	/* put all within last mask */
 	param.start = FBARRAY_TEST_LEN - 20;
 	param.end = FBARRAY_TEST_LEN - 1;
-	return init_array();
+	return init_aligned();
 }
 
 static int full_msk_test_setup(void)
@@ -91,16 +133,7 @@  static int full_msk_test_setup(void)
 	/* fill entire mask */
 	param.start = 0;
 	param.end = FBARRAY_TEST_LEN - 1;
-	return init_array();
-}
-
-static int empty_msk_test_setup(void)
-{
-	/* do not fill anything in */
-	reset_array();
-	param.start = -1;
-	param.end = -1;
-	return 0;
+	return init_aligned();
 }
 
 static int lookahead_test_setup(void)
@@ -108,7 +141,7 @@  static int lookahead_test_setup(void)
 	/* set index 64 as used */
 	param.start = 64;
 	param.end = 64;
-	return init_array();
+	return init_aligned();
 }
 
 static int lookbehind_test_setup(void)
@@ -116,7 +149,15 @@  static int lookbehind_test_setup(void)
 	/* set index 63 as used */
 	param.start = 63;
 	param.end = 63;
-	return init_array();
+	return init_aligned();
+}
+
+static int unaligned_test_setup(void)
+{
+	unaligned.start = 0;
+	/* leave one free bit at the end */
+	unaligned.end = FBARRAY_UNALIGNED_TEST_LEN - 2;
+	return init_unaligned();
 }
 
 static int test_invalid(void)
@@ -470,7 +511,7 @@  static int test_basic(void)
 	if (check_free())
 		return TEST_FAILED;
 
-	reset_array();
+	reset_aligned();
 
 	return TEST_SUCCESS;
 }
@@ -713,6 +754,26 @@  static int test_find(void)
 	return TEST_SUCCESS;
 }
 
+static int test_find_unaligned(void)
+{
+	TEST_ASSERT_EQUAL((int)unaligned.arr.count, unaligned.end - unaligned.start + 1,
+			"Wrong element count\n");
+	/* ensure space is free before start */
+	if (ensure_correct(&unaligned.arr, 0, unaligned.start - 1, false))
+		return TEST_FAILED;
+	/* ensure space is occupied where it's supposed to be */
+	if (ensure_correct(&unaligned.arr, unaligned.start, unaligned.end, true))
+		return TEST_FAILED;
+	/* ensure space after end is free as well */
+	if (ensure_correct(&unaligned.arr, unaligned.end + 1, FBARRAY_UNALIGNED_TEST_LEN - 1,
+			false))
+		return TEST_FAILED;
+	/* test if find_biggest API's work correctly */
+	if (test_biggest(&unaligned.arr, unaligned.start, unaligned.end))
+		return TEST_FAILED;
+	return TEST_SUCCESS;
+}
+
 static int test_empty(void)
 {
 	TEST_ASSERT_EQUAL((int)param.arr.count, 0, "Wrong element count\n");
@@ -814,17 +875,19 @@  static struct unit_test_suite fbarray_test_suite = {
 	.unit_test_cases = {
 		TEST_CASE(test_invalid),
 		TEST_CASE(test_basic),
-		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),
-		TEST_CASE_ST(last_msk_test_setup, reset_array, test_find),
-		TEST_CASE_ST(full_msk_test_setup, reset_array, test_find),
-		TEST_CASE_ST(empty_msk_test_setup, reset_array, test_empty),
-		TEST_CASE_ST(lookahead_test_setup, reset_array, test_lookahead),
-		TEST_CASE_ST(lookbehind_test_setup, reset_array, test_lookbehind),
+		TEST_CASE_ST(first_msk_test_setup, reset_aligned, test_find),
+		TEST_CASE_ST(cross_msk_test_setup, reset_aligned, test_find),
+		TEST_CASE_ST(multi_msk_test_setup, reset_aligned, test_find),
+		TEST_CASE_ST(last_msk_test_setup, reset_aligned, test_find),
+		TEST_CASE_ST(full_msk_test_setup, reset_aligned, test_find),
+		/* empty test does not need setup */
+		TEST_CASE_ST(NULL, reset_aligned, test_empty),
+		TEST_CASE_ST(lookahead_test_setup, reset_aligned, test_lookahead),
+		TEST_CASE_ST(lookbehind_test_setup, reset_aligned, test_lookbehind),
 		/* setup for these tests is more complex so do it in test func */
-		TEST_CASE_ST(NULL, reset_array, test_lookahead_mask),
-		TEST_CASE_ST(NULL, reset_array, test_lookbehind_mask),
+		TEST_CASE_ST(NULL, reset_aligned, test_lookahead_mask),
+		TEST_CASE_ST(NULL, reset_aligned, test_lookbehind_mask),
+		TEST_CASE_ST(unaligned_test_setup, reset_unaligned, test_find_unaligned),
 		TEST_CASES_END()
 	}
 };
diff --git a/lib/eal/common/eal_common_fbarray.c b/lib/eal/common/eal_common_fbarray.c
index 63d8b731f5..22b43073c6 100644
--- a/lib/eal/common/eal_common_fbarray.c
+++ b/lib/eal/common/eal_common_fbarray.c
@@ -173,7 +173,7 @@  find_next_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
 
 		/* combine current ignore mask with last index ignore mask */
 		if (msk_idx == last)
-			ignore_msk |= last_msk;
+			ignore_msk &= last_msk;
 
 		/* if we have an ignore mask, ignore once */
 		if (ignore_msk) {