When using huge_unlink we unlink the segment right after allocation. Although we unlink the file we keep the fd in fd_list so file still exist just the path deleted. When freeing the hugepage we need to close the fd and assign it with (-1) in fd_list for the page to be released. The current flow fails rte_malloc in the following flow when working with --huge-unlink option: 1. alloc_seg() for segment A - We allocate segment, unlink the path to the segment and keep the file descriptor in fd_list. 2. free_seg() for segment A - We clear the segment metadata and return - without closing fd or assigning (-1) in fd list. 3. alloc_seg() for segment A again - We find segment A as available, try to allocate it, find the old fd in fd_list try to unlink it as part of alloc_seg() but failed because path doesn't exist. The impact of such error is falsly failing rte_malloc() although we have hugepages available. Fixes: d435aad37da7 ("mem: support --huge-unlink mode") Signed-off-by: Roy Shterman <roy.shterman@vastdata.com> --- lib/librte_eal/linux/eal_memalloc.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/lib/librte_eal/linux/eal_memalloc.c b/lib/librte_eal/linux/eal_memalloc.c index 6dc1b2bae..c590d6043 100644 --- a/lib/librte_eal/linux/eal_memalloc.c +++ b/lib/librte_eal/linux/eal_memalloc.c @@ -709,7 +709,6 @@ free_seg(struct rte_memseg *ms, struct hugepage_info *hi, uint64_t map_offset; char path[PATH_MAX]; int fd, ret = 0; - bool exit_early; const struct internal_config *internal_conf = eal_get_internal_configuration(); @@ -725,17 +724,8 @@ free_seg(struct rte_memseg *ms, struct hugepage_info *hi, eal_mem_set_dump(ms->addr, ms->len, false); - exit_early = false; - /* if we're using anonymous hugepages, nothing to be done */ - if (internal_conf->in_memory && !memfd_create_supported) - exit_early = true; - - /* if we've already unlinked the page, nothing needs to be done */ - if (!internal_conf->in_memory && internal_conf->hugepage_unlink) - exit_early = true; - - if (exit_early) { + if (internal_conf->in_memory && !memfd_create_supported) { memset(ms, 0, sizeof(*ms)); return 0; } @@ -761,7 +751,7 @@ free_seg(struct rte_memseg *ms, struct hugepage_info *hi, /* if we're able to take out a write lock, we're the last one * holding onto this page. */ - if (!internal_conf->in_memory) { + if (!internal_conf->in_memory && !internal_conf->hugepage_unlink) { ret = lock(fd, LOCK_EX); if (ret >= 0) { /* no one else is using this page */ -- 2.24.1
On 22-Feb-21 10:41 AM, Roy Shterman wrote:
> When using huge_unlink we unlink the segment right
> after allocation. Although we unlink the file we keep
> the fd in fd_list so file still exist just the path deleted.
> When freeing the hugepage we need to close the fd and assign
> it with (-1) in fd_list for the page to be released.
>
> The current flow fails rte_malloc in the following flow when working
> with --huge-unlink option:
> 1. alloc_seg() for segment A -
> We allocate segment, unlink the path to the segment
> and keep the file descriptor in fd_list.
> 2. free_seg() for segment A -
> We clear the segment metadata and return - without closing fd
> or assigning (-1) in fd list.
> 3. alloc_seg() for segment A again -
> We find segment A as available, try to allocate it,
> find the old fd in fd_list try to unlink it
> as part of alloc_seg() but failed because path doesn't exist.
>
> The impact of such error is falsly failing rte_malloc()
> although we have hugepages available.
>
> Fixes: d435aad37da7 ("mem: support --huge-unlink mode")
>
> Signed-off-by: Roy Shterman <roy.shterman@vastdata.com>
Cc: stable@dpdk.org
Provisionally, patch looks fine, but i'll have to have a closer look.
--
Thanks,
Anatoly
On Mon, Feb 22, 2021 at 5:53 PM Burakov, Anatoly <anatoly.burakov@intel.com> wrote: > On 22-Feb-21 10:41 AM, Roy Shterman wrote: > > When using huge_unlink we unlink the segment right > > after allocation. Although we unlink the file we keep > > the fd in fd_list so file still exist just the path deleted. > > When freeing the hugepage we need to close the fd and assign > > it with (-1) in fd_list for the page to be released. > > > > The current flow fails rte_malloc in the following flow when working > > with --huge-unlink option: > > 1. alloc_seg() for segment A - > > We allocate segment, unlink the path to the segment > > and keep the file descriptor in fd_list. > > 2. free_seg() for segment A - > > We clear the segment metadata and return - without closing fd > > or assigning (-1) in fd list. > > 3. alloc_seg() for segment A again - > > We find segment A as available, try to allocate it, > > find the old fd in fd_list try to unlink it > > as part of alloc_seg() but failed because path doesn't exist. > > > > The impact of such error is falsly failing rte_malloc() > > although we have hugepages available. > > > > Fixes: d435aad37da7 ("mem: support --huge-unlink mode") > > > > Signed-off-by: Roy Shterman <roy.shterman@vastdata.com> > > Cc: stable@dpdk.org > > Provisionally, patch looks fine, but i'll have to have a closer look. > Hi Anatoly, Do I need to send this patch also to stable or it will happen automatically if the patch will reach the next release candidate? Also I wonder if you had more time to review this one? > > -- > Thanks, > Anatoly > Thanks, Roy
On 28-Feb-21 1:21 PM, Roy Shterman wrote: > > > On Mon, Feb 22, 2021 at 5:53 PM Burakov, Anatoly > <anatoly.burakov@intel.com <mailto:anatoly.burakov@intel.com>> wrote: > > On 22-Feb-21 10:41 AM, Roy Shterman wrote: > > When using huge_unlink we unlink the segment right > > after allocation. Although we unlink the file we keep > > the fd in fd_list so file still exist just the path deleted. > > When freeing the hugepage we need to close the fd and assign > > it with (-1) in fd_list for the page to be released. > > > > The current flow fails rte_malloc in the following flow when working > > with --huge-unlink option: > > 1. alloc_seg() for segment A - > > We allocate segment, unlink the path to the segment > > and keep the file descriptor in fd_list. > > 2. free_seg() for segment A - > > We clear the segment metadata and return - without closing fd > > or assigning (-1) in fd list. > > 3. alloc_seg() for segment A again - > > We find segment A as available, try to allocate it, > > find the old fd in fd_list try to unlink it > > as part of alloc_seg() but failed because path doesn't exist. > > > > The impact of such error is falsly failing rte_malloc() > > although we have hugepages available. > > > > Fixes: d435aad37da7 ("mem: support --huge-unlink mode") > > > > Signed-off-by: Roy Shterman <roy.shterman@vastdata.com > <mailto:roy.shterman@vastdata.com>> > > Cc: stable@dpdk.org <mailto:stable@dpdk.org> > > Provisionally, patch looks fine, but i'll have to have a closer look. > > > Hi Anatoly, > > Do I need to send this patch also to stable or it will happen > automatically if the patch will reach the next release candidate? Not automatically, generally you should add a CC to stable for the patch to be considered as part of stable release. That said, usually Fixed: tag is also used to determine whether a patch is backport-able, so having a correctly identified Fixed: tag is a mandatory minimum :) > Also I wonder if you had more time to review this one? Not yet, but i'll try to make some time this week. > > > -- > Thanks, > Anatoly > > > Thanks, > Roy -- Thanks, Anatoly
Hello,
On Mon, Mar 1, 2021 at 11:44 AM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 28-Feb-21 1:21 PM, Roy Shterman wrote:
> >
> >
> > On Mon, Feb 22, 2021 at 5:53 PM Burakov, Anatoly
> > <anatoly.burakov@intel.com <mailto:anatoly.burakov@intel.com>> wrote:
> >
> > On 22-Feb-21 10:41 AM, Roy Shterman wrote:
> > > When using huge_unlink we unlink the segment right
> > > after allocation. Although we unlink the file we keep
> > > the fd in fd_list so file still exist just the path deleted.
> > > When freeing the hugepage we need to close the fd and assign
> > > it with (-1) in fd_list for the page to be released.
> > >
> > > The current flow fails rte_malloc in the following flow when working
> > > with --huge-unlink option:
> > > 1. alloc_seg() for segment A -
> > > We allocate segment, unlink the path to the segment
> > > and keep the file descriptor in fd_list.
> > > 2. free_seg() for segment A -
> > > We clear the segment metadata and return - without closing fd
> > > or assigning (-1) in fd list.
> > > 3. alloc_seg() for segment A again -
> > > We find segment A as available, try to allocate it,
> > > find the old fd in fd_list try to unlink it
> > > as part of alloc_seg() but failed because path doesn't exist.
> > >
> > > The impact of such error is falsly failing rte_malloc()
> > > although we have hugepages available.
> > >
> > > Fixes: d435aad37da7 ("mem: support --huge-unlink mode")
> > >
> > > Signed-off-by: Roy Shterman <roy.shterman@vastdata.com
> > <mailto:roy.shterman@vastdata.com>>
> >
> > Cc: stable@dpdk.org <mailto:stable@dpdk.org>
> >
> > Provisionally, patch looks fine, but i'll have to have a closer look.
> >
> >
> > Hi Anatoly,
> >
> > Do I need to send this patch also to stable or it will happen
> > automatically if the patch will reach the next release candidate?
>
> Not automatically, generally you should add a CC to stable for the patch
> to be considered as part of stable release. That said, usually Fixed:
> tag is also used to determine whether a patch is backport-able, so
> having a correctly identified Fixed: tag is a mandatory minimum :)
>
> > Also I wonder if you had more time to review this one?
>
> Not yet, but i'll try to make some time this week.
Any update?
Thanks.
--
David Marchand
On 22-Feb-21 10:41 AM, Roy Shterman wrote:
> When using huge_unlink we unlink the segment right
> after allocation. Although we unlink the file we keep
> the fd in fd_list so file still exist just the path deleted.
> When freeing the hugepage we need to close the fd and assign
> it with (-1) in fd_list for the page to be released.
>
> The current flow fails rte_malloc in the following flow when working
> with --huge-unlink option:
> 1. alloc_seg() for segment A -
> We allocate segment, unlink the path to the segment
> and keep the file descriptor in fd_list.
> 2. free_seg() for segment A -
> We clear the segment metadata and return - without closing fd
> or assigning (-1) in fd list.
> 3. alloc_seg() for segment A again -
> We find segment A as available, try to allocate it,
> find the old fd in fd_list try to unlink it
> as part of alloc_seg() but failed because path doesn't exist.
>
> The impact of such error is falsly failing rte_malloc()
> although we have hugepages available.
>
> Fixes: d435aad37da7 ("mem: support --huge-unlink mode")
>
> Signed-off-by: Roy Shterman <roy.shterman@vastdata.com>
> ---
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
--
Thanks,
Anatoly
On Thu, Apr 1, 2021 at 1:07 PM Burakov, Anatoly <anatoly.burakov@intel.com> wrote: > > On 22-Feb-21 10:41 AM, Roy Shterman wrote: > > When using huge_unlink we unlink the segment right > > after allocation. Although we unlink the file we keep > > the fd in fd_list so file still exist just the path deleted. > > When freeing the hugepage we need to close the fd and assign > > it with (-1) in fd_list for the page to be released. > > > > The current flow fails rte_malloc in the following flow when working > > with --huge-unlink option: > > 1. alloc_seg() for segment A - > > We allocate segment, unlink the path to the segment > > and keep the file descriptor in fd_list. > > 2. free_seg() for segment A - > > We clear the segment metadata and return - without closing fd > > or assigning (-1) in fd list. > > 3. alloc_seg() for segment A again - > > We find segment A as available, try to allocate it, > > find the old fd in fd_list try to unlink it > > as part of alloc_seg() but failed because path doesn't exist. > > > > The impact of such error is falsly failing rte_malloc() falsely* > > although we have hugepages available. > > > > Fixes: d435aad37da7 ("mem: support --huge-unlink mode") Cc: stable@dpdk.org > > > > Signed-off-by: Roy Shterman <roy.shterman@vastdata.com> > Acked-by: Anatoly Burakov <anatoly.burakov@intel.com> Applied, thanks. -- David Marchand