From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
To: Vipin P R <vipinp@vmware.com>
Cc: <dev@dpdk.org>, <stable@dpdk.org>
Subject: Re: [PATCH 1/2] Memory Allocation: Fixes ms_idx jump (lookahead) during find_next_n() in fb_array library
Date: Tue, 16 May 2023 14:49:05 +0100 [thread overview]
Message-ID: <ab34a6d9-cec5-3992-6be7-c9ed2531dd27@intel.com> (raw)
In-Reply-To: <1673615325-20624-2-git-send-email-vipinp@vmware.com>
Hi Vipin,
Fix looks good, took a while to understand what's going on, but it's a
great find!
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
Couple of comments below:
On 1/13/2023 1:08 PM, Vipin P R wrote:
> In the legacy mem mode, when the fb_array is being populated, if there are holes in between, the ms_idx could go backward and there will be an overlap of the region starting from the ms_idx returned later. i.e. it's being mapped to two different physical regions in PA space to a contiguous region in VA space. this would result in the allocator assuming that the memory is contiguous even though there is a hole in between. In legacy mem, allocator assumes that PA contiguous are VA contiguous as well.
Technically, while this bug is indeed triggered in legacy mode, it does
not in any way depend on it, so I would suggest avoid mentioning it
explicitly.
Suggested commit message rewording:
fbarray: fix incorrect lookahead behavior
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 index 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 additional mask index increment.
Fix it to leave current index mask at `lookahead - 1`.
Fixes: c44d09811b40 ("eal: add shared indexed file-backed array")
Cc: anatoly.burakov@intel.com
>
> Cc: stable@dpdk.org
>
> Signed-off-by: Vipin P R <vipinp@vmware.com>
> Acked-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
> .mailmap | 1 +
> lib/eal/common/eal_common_fbarray.c | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/.mailmap b/.mailmap
> index 75884b6..3707bf5 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1391,6 +1391,7 @@ Vincent Guo <guopengfei160@163.com>
> Vincent Jardin <vincent.jardin@6wind.com>
> Vincent Li <vincent.mc.li@gmail.com>
> Vincent S. Cojot <vcojot@redhat.com>
> +Vipin P R <vipinp@vmware.com> <vipinpadmamramesh@gmail.com>
> Vipin Varghese <vipin.varghese@amd.com> <vipin.varghese@intel.com>
> Vipul Ashri <vipul.ashri@oracle.com>
> Vishal Kulkarni <vishal@chelsio.com>
> diff --git a/lib/eal/common/eal_common_fbarray.c b/lib/eal/common/eal_common_fbarray.c
> index f11f879..551bd87 100644
> --- a/lib/eal/common/eal_common_fbarray.c
> +++ b/lib/eal/common/eal_common_fbarray.c
> @@ -236,7 +236,7 @@ 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;
> + msk_idx = lookahead_idx - 1;
> break;
> }
>
--
Thanks,
Anatoly
next parent reply other threads:[~2023-05-16 13:49 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1673615325-20624-1-git-send-email-vipinp@vmware.com>
[not found] ` <1673615325-20624-2-git-send-email-vipinp@vmware.com>
2023-05-16 13:49 ` Burakov, Anatoly [this message]
[not found] ` <1673615325-20624-3-git-send-email-vipinp@vmware.com>
2023-05-16 14:20 ` [PATCH 2/2] Memory Allocation: Fixes ms_idx jump (lookbehind) during find_prev_n() " Burakov, Anatoly
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ab34a6d9-cec5-3992-6be7-c9ed2531dd27@intel.com \
--to=anatoly.burakov@intel.com \
--cc=dev@dpdk.org \
--cc=stable@dpdk.org \
--cc=vipinp@vmware.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).