| Message ID | 20230523140002.747157575@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/vmalloc: Assorted fixes and improvements | expand |
On Tue, May 23, 2023 at 04:02:14PM +0200, Thomas Gleixner wrote: > + if (READ_ONCE(vb->free) < (1UL << order)) > + continue; > + > spin_lock(&vb->lock); > if (vb->free < (1UL << order)) { > spin_unlock(&vb->lock); > @@ -2174,7 +2177,7 @@ static void *vb_alloc(unsigned long size > > pages_off = VMAP_BBMAP_BITS - vb->free; > vaddr = vmap_block_vaddr(vb->va->va_start, pages_off); > - vb->free -= 1UL << order; > + WRITE_ONCE(vb->free, vb->free - (1UL << order)); Maybe just a matter of preference, but wouldn't an atomic_t be better here? We'd have another locked instruction in the alloc path, but I always find the READ_ONCE/WRITE_ONCE usage a bit fragile that I'd rather reserve them to well documented hot path code.
On Tue, May 23 2023 at 17:29, Christoph Hellwig wrote: > On Tue, May 23, 2023 at 04:02:14PM +0200, Thomas Gleixner wrote: >> + if (READ_ONCE(vb->free) < (1UL << order)) >> + continue; >> + >> spin_lock(&vb->lock); >> if (vb->free < (1UL << order)) { >> spin_unlock(&vb->lock); >> @@ -2174,7 +2177,7 @@ static void *vb_alloc(unsigned long size >> >> pages_off = VMAP_BBMAP_BITS - vb->free; >> vaddr = vmap_block_vaddr(vb->va->va_start, pages_off); >> - vb->free -= 1UL << order; >> + WRITE_ONCE(vb->free, vb->free - (1UL << order)); > > Maybe just a matter of preference, but wouldn't an atomic_t be > better here? We'd have another locked instruction in the alloc > path, but I always find the READ_ONCE/WRITE_ONCE usage a bit > fragile that I'd rather reserve them to well documented hot > path code. I don't see a problem with these lockless quickchecks, especially not in this particular case, but no strong opinion either. Thanks tglx
On Tue, May 23, 2023 at 06:17:42PM +0200, Thomas Gleixner wrote: > On Tue, May 23 2023 at 17:29, Christoph Hellwig wrote: > > On Tue, May 23, 2023 at 04:02:14PM +0200, Thomas Gleixner wrote: > >> + if (READ_ONCE(vb->free) < (1UL << order)) > >> + continue; > >> + > >> spin_lock(&vb->lock); > >> if (vb->free < (1UL << order)) { > >> spin_unlock(&vb->lock); > >> @@ -2174,7 +2177,7 @@ static void *vb_alloc(unsigned long size > >> > >> pages_off = VMAP_BBMAP_BITS - vb->free; > >> vaddr = vmap_block_vaddr(vb->va->va_start, pages_off); > >> - vb->free -= 1UL << order; > >> + WRITE_ONCE(vb->free, vb->free - (1UL << order)); > > > > Maybe just a matter of preference, but wouldn't an atomic_t be > > better here? We'd have another locked instruction in the alloc > > path, but I always find the READ_ONCE/WRITE_ONCE usage a bit > > fragile that I'd rather reserve them to well documented hot > > path code. > > I don't see a problem with these lockless quickchecks, especially not > in this particular case, but no strong opinion either. > Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
--- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2166,6 +2166,9 @@ static void *vb_alloc(unsigned long size list_for_each_entry_rcu(vb, &vbq->free, free_list) { unsigned long pages_off; + if (READ_ONCE(vb->free) < (1UL << order)) + continue; + spin_lock(&vb->lock); if (vb->free < (1UL << order)) { spin_unlock(&vb->lock); @@ -2174,7 +2177,7 @@ static void *vb_alloc(unsigned long size pages_off = VMAP_BBMAP_BITS - vb->free; vaddr = vmap_block_vaddr(vb->va->va_start, pages_off); - vb->free -= 1UL << order; + WRITE_ONCE(vb->free, vb->free - (1UL << order)); bitmap_set(vb->used_map, pages_off, (1UL << order)); if (vb->free == 0) { spin_lock(&vbq->lock);
vb_alloc() unconditionally locks a vmap_block on the free list to check the free space. This can be done locklessly because vmap_block::free never increases, it's only decreased on allocations. Check the free space lockless and only if that succeeds, recheck under the lock. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- mm/vmalloc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)