[PATCH v1 0/4] fbarray lookahead/lookbehind fixes

[PATCH v1 0/4] fbarray lookahead/lookbehind fixes

[Anatoly Burakov]

Once upon a time, a few patches were submitted by Vipin P R:

https://patches.dpdk.org/project/dpdk/patch/1673615669-21011-2-git-send-email-vipinp@vmware.com/
https://patches.dpdk.org/project/dpdk/patch/1673615669-21011-3-git-send-email-vipinp@vmware.com/
https://patches.dpdk.org/project/dpdk/patch/1673615669-21011-2-git-send-email-vipinp@vmware.com/
https://patches.dpdk.org/project/dpdk/patch/1673615669-21011-3-git-send-email-vipinp@vmware.com/

They were reviewed and changes were requested, but the author never followed up
and these patches kind of fell through the cracks. The patches fixed real bugs
in fbarray lookahead/lookbehind behavior, so now these bugs have resurfaced in
some customer reports.

This is a resubmit with improvements and added unit tests.

Anatoly Burakov (4):
  fbarray: fix incorrect lookahead behavior
  fbarray: fix incorrect lookbehind behavior
  fbarray: fix lookahead ignore mask handling
  fbarray: fix lookbehind ignore mask handling

 app/test/test_fbarray.c             | 102 ++++++++++++++++++++++++++++
 lib/eal/common/eal_common_fbarray.c |  28 ++++++--
 2 files changed, 123 insertions(+), 7 deletions(-)

-- 
2.43.0

[PATCH v1 1/4] fbarray: fix incorrect lookahead behavior

[Anatoly Burakov]

Currently, whenever last bit of current index mask is set (meaning, there is
potentially a run starting at the end of the mask), lookahead loop is entered.
In that loop, if the first bit of lookahead mask is not set, the lookahead is
stopped, and the current lookahead mask index is assigned to current index mask.
However, because at that point we are inside a for-loop that increments current
index mask after each iteration, this results in erroneous mask index
increment.

Fixlookahead to avoid erroneous increment, and add corresponding unit test.

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

Signed-off-by: Vipin P R <vipinp@vmware.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 app/test/test_fbarray.c             | 23 +++++++++++++++++++++++
 lib/eal/common/eal_common_fbarray.c |  3 ++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/app/test/test_fbarray.c b/app/test/test_fbarray.c
index 26a51e2a3e..bf89b99e5b 100644
--- a/app/test/test_fbarray.c
+++ b/app/test/test_fbarray.c
@@ -103,6 +103,14 @@ static int empty_msk_test_setup(void)
 	return 0;
 }
 
+static int lookahead_test_setup(void)
+{
+	/* set index 64 as used */
+	param.start = 64;
+	param.end = 64;
+	return init_array();
+}
+
 static int test_invalid(void)
 {
 	struct rte_fbarray dummy;
@@ -709,6 +717,20 @@ static int test_empty(void)
 	return TEST_SUCCESS;
 }
 
+static int test_lookahead(void)
+{
+	int ret;
+
+	/* run regular test first */
+	ret = test_find();
+	if (ret != TEST_SUCCESS)
+		return ret;
+
+	/* test if we can find free chunk while not starting with 0 */
+	TEST_ASSERT_EQUAL(rte_fbarray_find_next_n_free(&param.arr, 1, param.start),
+			param.start + 1, "Free chunk index is wrong\n");
+	return TEST_SUCCESS;
+}
 
 static struct unit_test_suite fbarray_test_suite = {
 	.suite_name = "fbarray autotest",
@@ -723,6 +745,7 @@ static struct unit_test_suite fbarray_test_suite = {
 		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_CASES_END()
 	}
 };
diff --git a/lib/eal/common/eal_common_fbarray.c b/lib/eal/common/eal_common_fbarray.c
index 0fe5bcfe06..2680b34823 100644
--- a/lib/eal/common/eal_common_fbarray.c
+++ b/lib/eal/common/eal_common_fbarray.c
@@ -236,7 +236,8 @@ find_next_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
 				 * as well, so skip that on next iteration.
 				 */
 				ignore_msk = ~((1ULL << need) - 1);
-				msk_idx = lookahead_idx;
+				/* outer loop will increment msk_idx so add 1 */
+				msk_idx = lookahead_idx - 1;
 				break;
 			}
 
-- 
2.43.0

[PATCH v1 2/4] fbarray: fix incorrect lookbehind behavior

[Anatoly Burakov]

Currently, whenever first bit of current index mask is set (meaning, there is
potentially a run starting at the start of the mask), lookbehind loop is
entered. In that loop, if the last bit of lookbehind mask is not set, the
lookbehind is stopped, and the current lookbehind mask index is assigned to
current index mask. However, because at that point we are inside a while-loop
that decrements current index mask after each iteration, this results in
erroneous mask index decrement.

Fix lookbehind to avoid erroneous decrement, and add corresponding unit test.

Fixes: e1ca5dc86226 ("fbarray: add reverse finding of chunk")
Cc: stable@dpdk.org

Signed-off-by: Vipin P R <vipinp@vmware.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 app/test/test_fbarray.c             | 24 ++++++++++++++++++++++++
 lib/eal/common/eal_common_fbarray.c |  3 ++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/app/test/test_fbarray.c b/app/test/test_fbarray.c
index bf89b99e5b..147d6e2a07 100644
--- a/app/test/test_fbarray.c
+++ b/app/test/test_fbarray.c
@@ -111,6 +111,14 @@ static int lookahead_test_setup(void)
 	return init_array();
 }
 
+static int lookbehind_test_setup(void)
+{
+	/* set index 63 as used */
+	param.start = 63;
+	param.end = 63;
+	return init_array();
+}
+
 static int test_invalid(void)
 {
 	struct rte_fbarray dummy;
@@ -732,6 +740,21 @@ static int test_lookahead(void)
 	return TEST_SUCCESS;
 }
 
+static int test_lookbehind(void)
+{
+	int ret, free_len = 2;
+
+	/* run regular test first */
+	ret = test_find();
+	if (ret != TEST_SUCCESS)
+		return ret;
+
+	/* test if we can find free chunk while crossing mask boundary */
+	TEST_ASSERT_EQUAL(rte_fbarray_find_prev_n_free(&param.arr, param.start + 1, free_len),
+			param.start - free_len, "Free chunk index is wrong\n");
+	return TEST_SUCCESS;
+}
+
 static struct unit_test_suite fbarray_test_suite = {
 	.suite_name = "fbarray autotest",
 	.setup = autotest_setup,
@@ -746,6 +769,7 @@ static struct unit_test_suite fbarray_test_suite = {
 		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_CASES_END()
 	}
 };
diff --git a/lib/eal/common/eal_common_fbarray.c b/lib/eal/common/eal_common_fbarray.c
index 2680b34823..b4f0b0b0c3 100644
--- a/lib/eal/common/eal_common_fbarray.c
+++ b/lib/eal/common/eal_common_fbarray.c
@@ -512,7 +512,8 @@ find_prev_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
 				 * as well, so skip that on next iteration.
 				 */
 				ignore_msk = UINT64_MAX << need;
-				msk_idx = lookbehind_idx;
+				/* outer loop will decrement msk_idx so add 1 */
+				msk_idx = lookbehind_idx + 1;
 				break;
 			}
 
-- 
2.43.0

[PATCH v1 3/4] fbarray: fix lookahead ignore mask handling

[Anatoly Burakov]

When lookahead mask does not have its first bit set, we can infer that we've
lost our run. However, currently, we set ignore mask to ignore `need` number of
bits, which is incorrect because while there is no *current* run within those
bits, we might still be able to start a new run within those ignored bits later.

This issue is fixed by counting how many shifts it took to lose the run, and
this is the number of bits we should ignore (+1 to skip one we stopped on).
Also, add unit tests to reproduce the problem.

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

Signed-off-by: Vipin P R <vipinp@vmware.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 app/test/test_fbarray.c             | 28 ++++++++++++++++++++++++++++
 lib/eal/common/eal_common_fbarray.c | 13 ++++++++++---
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/app/test/test_fbarray.c b/app/test/test_fbarray.c
index 147d6e2a07..4b17ef6be3 100644
--- a/app/test/test_fbarray.c
+++ b/app/test/test_fbarray.c
@@ -755,6 +755,32 @@ static int test_lookbehind(void)
 	return TEST_SUCCESS;
 }
 
+static int test_lookahead_mask(void)
+{
+	/*
+	 * There is a certain type of lookahead behavior we want to test here,
+	 * namely masking of bits that were scanned with lookahead but that we
+	 * know do not match our criteria. This is achieved in following steps:
+	 *
+	 *   0. Look for a big enough chunk of free space (say, 62 elements)
+	 *   1. Trigger lookahead by breaking a run somewhere inside mask 0
+	 *      (indices 0-63)
+	 *   2. Fail lookahead by breaking the run somewhere inside mask 1
+	 *      (indices 64-127)
+	 *   3. Ensure that we can still find free space in mask 1 afterwards
+	 */
+
+	/* break run on first mask */
+	rte_fbarray_set_used(&param.arr, 61);
+	/* break run on second mask */
+	rte_fbarray_set_used(&param.arr, 70);
+
+	/* we expect to find free space at 71 */
+	TEST_ASSERT_EQUAL(rte_fbarray_find_next_n_free(&param.arr, 0, 62),
+			71, "Free chunk index is wrong\n");
+	return TEST_SUCCESS;
+}
+
 static struct unit_test_suite fbarray_test_suite = {
 	.suite_name = "fbarray autotest",
 	.setup = autotest_setup,
@@ -770,6 +796,8 @@ static struct unit_test_suite fbarray_test_suite = {
 		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),
+		/* setup for these tests is more complex so do it in test func */
+		TEST_CASE_ST(NULL, reset_array, test_lookahead_mask),
 		TEST_CASES_END()
 	}
 };
diff --git a/lib/eal/common/eal_common_fbarray.c b/lib/eal/common/eal_common_fbarray.c
index b4f0b0b0c3..195f8394be 100644
--- a/lib/eal/common/eal_common_fbarray.c
+++ b/lib/eal/common/eal_common_fbarray.c
@@ -216,6 +216,8 @@ find_next_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
 		for (lookahead_idx = msk_idx + 1; lookahead_idx < msk->n_masks;
 				lookahead_idx++) {
 			unsigned int s_idx, need;
+			uint64_t first_bit = 1;
+
 			lookahead_msk = msk->data[lookahead_idx];
 
 			/* if we're looking for free space, invert the mask */
@@ -225,17 +227,22 @@ find_next_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
 			/* figure out how many consecutive bits we need here */
 			need = RTE_MIN(left, MASK_ALIGN);
 
-			for (s_idx = 0; s_idx < need - 1; s_idx++)
+			/* count number of shifts we performed */
+			for (s_idx = 0; s_idx < need - 1; s_idx++) {
 				lookahead_msk &= lookahead_msk >> 1ULL;
+				/* did we lose the run yet? */
+				if ((lookahead_msk & first_bit) == 0)
+					break;
+			}
 
 			/* if first bit is not set, we've lost the run */
-			if ((lookahead_msk & 1) == 0) {
+			if ((lookahead_msk & first_bit) == 0) {
 				/*
 				 * we've scanned this far, so we know there are
 				 * no runs in the space we've lookahead-scanned
 				 * as well, so skip that on next iteration.
 				 */
-				ignore_msk = ~((1ULL << need) - 1);
+				ignore_msk = ~((1ULL << (s_idx + 1)) - 1);
 				/* outer loop will increment msk_idx so add 1 */
 				msk_idx = lookahead_idx - 1;
 				break;
-- 
2.43.0

[PATCH v1 4/4] fbarray: fix lookbehind ignore mask handling

[Anatoly Burakov]

When lookahead mask does not have its last bit set, we can infer that we've lost
our run. However, currently, we set ignore mask to ignore first `need` bits,
which is incorrect for two reasons: first, using `need` bits as ignore bit count
means we might miss opportunities to start a new run within those bits, and more
improtantly when doing lookbehind, we start looking from the top, so we should
be ignoring *last* N bits, not *first* N bits of the mask.

This issue is fixed by counting how many shifts it took to lose the run, and
this is the number of bits we should ignore from the top (+1 to skip one we
stopped on). Also, add unit tests to reproduce the problem.

Fixes: e1ca5dc86226 ("fbarray: add reverse finding of chunk")
Cc: stable@dpdk.org

Signed-off-by: Vipin P R <vipinp@vmware.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 app/test/test_fbarray.c             | 27 +++++++++++++++++++++++++++
 lib/eal/common/eal_common_fbarray.c |  9 +++++++--
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/app/test/test_fbarray.c b/app/test/test_fbarray.c
index 4b17ef6be3..13c6691e50 100644
--- a/app/test/test_fbarray.c
+++ b/app/test/test_fbarray.c
@@ -781,6 +781,32 @@ static int test_lookahead_mask(void)
 	return TEST_SUCCESS;
 }
 
+static int test_lookbehind_mask(void)
+{
+	/*
+	 * There is a certain type of lookbehind behavior we want to test here,
+	 * namely masking of bits that were scanned with lookbehind but that we
+	 * know do not match our criteria. This is achieved in two steps:
+	 *
+	 *   0. Look for a big enough chunk of free space (say, 62 elements)
+	 *   1. Trigger lookbehind by breaking a run somewhere inside mask 2
+	 *      (indices 128-191)
+	 *   2. Fail lookbehind by breaking the run somewhere inside mask 1
+	 *      (indices 64-127)
+	 *   3. Ensure that we can still find free space in mask 1 afterwards
+	 */
+
+	/* break run on mask 2 */
+	rte_fbarray_set_used(&param.arr, 130);
+	/* break run on mask 1 */
+	rte_fbarray_set_used(&param.arr, 70);
+
+	/* start from 190, we expect to find free space at 8 */
+	TEST_ASSERT_EQUAL(rte_fbarray_find_prev_n_free(&param.arr, 190, 62),
+			8, "Free chunk index is wrong\n");
+	return TEST_SUCCESS;
+}
+
 static struct unit_test_suite fbarray_test_suite = {
 	.suite_name = "fbarray autotest",
 	.setup = autotest_setup,
@@ -798,6 +824,7 @@ static struct unit_test_suite fbarray_test_suite = {
 		TEST_CASE_ST(lookbehind_test_setup, reset_array, 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_CASES_END()
 	}
 };
diff --git a/lib/eal/common/eal_common_fbarray.c b/lib/eal/common/eal_common_fbarray.c
index 195f8394be..63d8b731f5 100644
--- a/lib/eal/common/eal_common_fbarray.c
+++ b/lib/eal/common/eal_common_fbarray.c
@@ -508,8 +508,13 @@ find_prev_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
 			/* figure out how many consecutive bits we need here */
 			need = RTE_MIN(left, MASK_ALIGN);
 
-			for (s_idx = 0; s_idx < need - 1; s_idx++)
+			/* count number of shifts we performed */
+			for (s_idx = 0; s_idx < need - 1; s_idx++) {
 				lookbehind_msk &= lookbehind_msk << 1ULL;
+				/* did we lose the run yet? */
+				if ((lookbehind_msk & last_bit) == 0)
+					break;
+			}
 
 			/* if last bit is not set, we've lost the run */
 			if ((lookbehind_msk & last_bit) == 0) {
@@ -518,7 +523,7 @@ find_prev_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
 				 * no runs in the space we've lookbehind-scanned
 				 * as well, so skip that on next iteration.
 				 */
-				ignore_msk = UINT64_MAX << need;
+				ignore_msk = ~(UINT64_MAX << (MASK_ALIGN - s_idx - 1));
 				/* outer loop will decrement msk_idx so add 1 */
 				msk_idx = lookbehind_idx + 1;
 				break;
-- 
2.43.0

Re: [PATCH v1 0/4] fbarray lookahead/lookbehind fixes

[David Marchand]

On Mon, Jul 8, 2024 at 6:07 PM Anatoly Burakov
<anatoly.burakov@intel.com> wrote:
>
> Once upon a time, a few patches were submitted by Vipin P R:
>
> https://patches.dpdk.org/project/dpdk/patch/1673615669-21011-2-git-send-email-vipinp@vmware.com/
> https://patches.dpdk.org/project/dpdk/patch/1673615669-21011-3-git-send-email-vipinp@vmware.com/
> https://patches.dpdk.org/project/dpdk/patch/1673615669-21011-2-git-send-email-vipinp@vmware.com/
> https://patches.dpdk.org/project/dpdk/patch/1673615669-21011-3-git-send-email-vipinp@vmware.com/
>
> They were reviewed and changes were requested, but the author never followed up
> and these patches kind of fell through the cracks. The patches fixed real bugs
> in fbarray lookahead/lookbehind behavior, so now these bugs have resurfaced in
> some customer reports.
>
> This is a resubmit with improvements and added unit tests.
>
> Anatoly Burakov (4):
>   fbarray: fix incorrect lookahead behavior
>   fbarray: fix incorrect lookbehind behavior
>   fbarray: fix lookahead ignore mask handling
>   fbarray: fix lookbehind ignore mask handling
>
>  app/test/test_fbarray.c             | 102 ++++++++++++++++++++++++++++
>  lib/eal/common/eal_common_fbarray.c |  28 ++++++--
>  2 files changed, 123 insertions(+), 7 deletions(-)

Thank you, series applied.


-- 
David Marchand