Received-SPF: pass (google.com: domain of linux-kernel+bounces-58201-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Uladzislau Rezki <urezki@gmail.com>
Date: Thu, 8 Feb 2024 14:57:06 +0100
To: Baoquan He <bhe@redhat.com>
Cc: "Uladzislau Rezki (Sony)" <urezki@gmail.com>, linux-mm@kvack.org,
	Andrew Morton <akpm@linux-foundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Lorenzo Stoakes <lstoakes@gmail.com>,
	Christoph Hellwig <hch@infradead.org>,
	Matthew Wilcox <willy@infradead.org>,
	"Liam R . Howlett" <Liam.Howlett@oracle.com>,
	Dave Chinner <david@fromorbit.com>,
	"Paul E . McKenney" <paulmck@kernel.org>,
	Joel Fernandes <joel@joelfernandes.org>,
	Oleksiy Avramchenko <oleksiy.avramchenko@sony.com>
Subject: Re: [PATCH v3 07/11] mm: vmalloc: Offload free_vmap_area_lock lock
Message-ID: <ZcTdssE-4xpkMaPH@pc636>
References: <20240102184633.748113-1-urezki@gmail.com>
 <20240102184633.748113-8-urezki@gmail.com>
 <ZcQfc6myl5KCFk3V@MiWiFi-R3L-srv>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZcQfc6myl5KCFk3V@MiWiFi-R3L-srv>

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