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

[Uladzislau Rezki]

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