Re: [PATCH v3 07/11] mm: vmalloc: Offload free_vmap_area_lock lock

Uladzislau Rezki
2024-02-08T13:57:14Z

On Thu, Feb 08, 2024 at 08:25:23AM +0800, Baoquan He wrote:
> On 01/02/24 at 07:46pm, Uladzislau Rezki (Sony) wrote:
> ......
> > +static struct vmap_area *
> > +node_alloc(unsigned long size, unsigned long align,
> > + unsigned long vstart, unsigned long vend,
> > + unsigned long *addr, unsigned int *vn_id)
> > +{
> > + struct vmap_area *va;
> > +
> > + *vn_id = 0;
> > + *addr = vend;
> > +
> > + /*
> > + * Fallback to a global heap if not vmalloc or there
> > + * is only one node.
> > + */
> > + if (vstart != VMALLOC_START || vend != VMALLOC_END ||
> > + nr_vmap_nodes == 1)
> > + return NULL;
> > +
> > + *vn_id = raw_smp_processor_id() % nr_vmap_nodes;
> > + va = node_pool_del_va(id_to_node(*vn_id), size, align, vstart, vend);
> > + *vn_id = encode_vn_id(*vn_id);
> > +
> > + if (va)
> > + *addr = va->va_start;
> > +
> > + return va;
> > +}
> > +
> > /*
> > * Allocate a region of KVA of the specified size and alignment, within the
> > * vstart and vend.
> > @@ -1637,6 +1807,7 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> > struct vmap_area *va;
> > unsigned long freed;
> > unsigned long addr;
> > + unsigned int vn_id;
> > int purged = 0;
> > int ret;
> >
> > @@ -1647,11 +1818,23 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> > return ERR_PTR(-EBUSY);
> >
> > might_sleep();
> > - gfp_mask = gfp_mask & GFP_RECLAIM_MASK;
> >
> > - va = kmem_cache_alloc_node(vmap_area_cachep, gfp_mask, node);
> > - if (unlikely(!va))
> > - return ERR_PTR(-ENOMEM);
> > + /*
> > + * If a VA is obtained from a global heap(if it fails here)
> > + * it is anyway marked with this "vn_id" so it is returned
> > + * to this pool's node later. Such way gives a possibility
> > + * to populate pools based on users demand.
> > + *
> > + * On success a ready to go VA is returned.
> > + */
> > + va = node_alloc(size, align, vstart, vend, &addr, &vn_id);
>
> Sorry for late checking.
>
No problem :)

> Here, if no available va got, e.g a empty vp, still we will get an
> effective vn_id with the current cpu_id for VMALLOC region allocation
> request.
>
> > + if (!va) {
> > + gfp_mask = gfp_mask & GFP_RECLAIM_MASK;
> > +
> > + va = kmem_cache_alloc_node(vmap_area_cachep, gfp_mask, node);
> > + if (unlikely(!va))
> > + return ERR_PTR(-ENOMEM);
> > + }
> >
> > /*
> > * Only scan the relevant parts containing pointers to other objects
> > @@ -1660,10 +1843,12 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> > kmemleak_scan_area(&va->rb_node, SIZE_MAX, gfp_mask);
> >
> > retry:
> > - preload_this_cpu_lock(&free_vmap_area_lock, gfp_mask, node);
> > - addr = __alloc_vmap_area(&free_vmap_area_root, &free_vmap_area_list,
> > - size, align, vstart, vend);
> > - spin_unlock(&free_vmap_area_lock);
> > + if (addr == vend) {
> > + preload_this_cpu_lock(&free_vmap_area_lock, gfp_mask, node);
> > + addr = __alloc_vmap_area(&free_vmap_area_root, &free_vmap_area_list,
> > + size, align, vstart, vend);
>
> Then, here, we will get an available va from random location, but its
> vn_id is from the current cpu.
>
> Then in purge_vmap_node(), we will decode the vn_id stored in va->flags,
> and add the relevant va into vn->pool[] according to the vn_id. The
> worst case could be most of va in vn->pool[] are not corresponding to
> the vmap_nodes they belongs to. It doesn't matter?
>
We do not do any "in-front" population, instead it behaves as a cache
miss when you need to access a main memmory to do a load and then keep
the data in a cache.

Same here. As a first step, for a CPU it always a miss, thus a VA is
obtained from the global heap and is marked with a current CPU that
makes an attempt to alloc. Later on that CPU/node is populated by that
marked VA. So second alloc on same CPU goes via fast path.

VAs are populated based on demand and those nodes which do allocations.

> Should we adjust the code of vn_id assigning in node_alloc(), or I missed anything?
Now it is open-coded. Some further refactoring should be done. Agree.

--
Uladzislau Rezki