Re: [PATCH] vmalloc: Use atomic_long_add_return_relaxed()

Uladzislau Rezki
2025-04-16T07:58:28Z

On Tue, Apr 15, 2025 at 07:37:31PM +0200, Mateusz Guzik wrote:
> On Tue, Apr 15, 2025 at 01:26:46PM +0200, Uladzislau Rezki (Sony) wrote:
> > Switch from the atomic_long_add_return() to its relaxed version.
> >
> > We do not need a full memory barrier or any memory ordering during
> > increasing the "vmap_lazy_nr" variable. What we only need is to do it
> > atomically. This is what atomic_long_add_return_relaxed() guarantees.
> >
> > AARCH64:
> >
> > <snip>
> > Default:
> > 40ec: d34cfe94 lsr x20, x20, #12
> > 40f0: 14000044 b 4200 <free_vmap_area_noflush+0x19c>
> > 40f4: 94000000 bl 0 <__sanitizer_cov_trace_pc>
> > 40f8: 90000000 adrp x0, 0 <__traceiter_alloc_vmap_area>
> > 40fc: 91000000 add x0, x0, #0x0
> > 4100: f8f40016 ldaddal x20, x22, [x0]
> > 4104: 8b160296 add x22, x20, x22
> >
> > Relaxed:
> > 40ec: d34cfe94 lsr x20, x20, #12
> > 40f0: 14000044 b 4200 <free_vmap_area_noflush+0x19c>
> > 40f4: 94000000 bl 0 <__sanitizer_cov_trace_pc>
> > 40f8: 90000000 adrp x0, 0 <__traceiter_alloc_vmap_area>
> > 40fc: 91000000 add x0, x0, #0x0
> > 4100: f8340016 ldadd x20, x22, [x0]
> > 4104: 8b160296 add x22, x20, x22
> > <snip>
> >
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > ---
> > mm/vmalloc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 7bb32f498d39..9d4027041a3f 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -2370,7 +2370,7 @@ static void free_vmap_area_noflush(struct vmap_area *va)
> > if (WARN_ON_ONCE(!list_empty(&va->list)))
> > return;
> >
> > - nr_lazy = atomic_long_add_return(va_size(va) >> PAGE_SHIFT,
> > + nr_lazy = atomic_long_add_return_relaxed(va_size(va) >> PAGE_SHIFT,
> > &vmap_lazy_nr);
> >
> > /*
>
> If touching this, maybe a step further -- I see false sharing with:
> nm vmlinux | sort -k 1
>
> [snip]
> ffffffff845cb240 b vmap_lazy_nr
> ffffffff845cb248 b nr_vmalloc_pages
> ffffffff845cb260 b single
> [/snip]
>
> If this is used enough to warrant messing with eliding a fence, then it
> probably also wants to elide some false sharing.
>
> So at least the first two should be annotated with __cacheline_aligned_in_smp?
>
> I am not going to push though, feel free to ignore.
>
OK, i see your point. I will check closer. We can place both into two
different cache-lines to prevent interference between each other.

But that will be a different patch.

Thank you for the nit!

--
Uladzislau Rezki