* Re: [PATCH] Memory Allocation: Adding a new UT for fb_array
[not found] ` <1673615567-20873-2-git-send-email-vipinp@vmware.com>
2023-01-16 17:54 ` [PATCH] Memory Allocation: Adding a new UT for fb_array Stephen Hemminger
@ 2023-05-16 13:39 ` Burakov, Anatoly
2023-05-16 14:25 ` Burakov, Anatoly
1 sibling, 1 reply; 3+ messages in thread
From: Burakov, Anatoly @ 2023-05-16 13:39 UTC (permalink / raw)
To: Vipin P R; +Cc: dev, stable
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
^ permalink raw reply [flat|nested] 3+ messages in thread