Re: [PATCH 1/1] mm: vmalloc: Fix lockdep warning

From: Uladzislau Rezki <urezki@gmail.com>
To: Baoquan He <bhe@redhat.com>
Cc: Uladzislau Rezki <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>,
	Dave Chinner <david@fromorbit.com>,
	Oleksiy Avramchenko <oleksiy.avramchenko@sony.com>,
	Jens Axboe <axboe@kernel.dk>, Omar Sandoval <osandov@fb.com>
Subject: Re: [PATCH 1/1] mm: vmalloc: Fix lockdep warning
Date: Sat, 30 Mar 2024 20:39:15 +0100	[thread overview]
Message-ID: <ZghqY6tpKsFvhETX@pc638.lan> (raw)
In-Reply-To: <ZggR1fW0rAHSKF+j@MiWiFi-R3L-srv>

On Sat, Mar 30, 2024 at 09:21:25PM +0800, Baoquan He wrote:
> On 03/30/24 at 01:55pm, Uladzislau Rezki wrote:
> > On Fri, Mar 29, 2024 at 03:44:40PM +0800, Baoquan He wrote:
> > > On 03/28/24 at 03:03pm, Uladzislau Rezki (Sony) wrote:
> ......snip 
> > > How about below change about va_start_lowest? Personal preference, not
> > > strong opinion.
> > > 
> > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > index 9b1a41e12d70..bd6a66c54ad2 100644
> > > --- a/mm/vmalloc.c
> > > +++ b/mm/vmalloc.c
> > > @@ -1046,19 +1046,19 @@ __find_vmap_area_exceed_addr(unsigned long addr, struct rb_root *root)
> > >  static struct vmap_node *
> > >  find_vmap_area_exceed_addr_lock(unsigned long addr, struct vmap_area **va)
> > >  {
> > > -	unsigned long va_start_lowest;
> > > +	unsigned long va_start_lowest = ULONG_MAX;
> > >  	struct vmap_node *vn;
> > >  	int i;
> > >  
> > >  repeat:
> > > -	for (i = 0, va_start_lowest = 0; i < nr_vmap_nodes; i++) {
> > > +	for (i = 0; i < nr_vmap_nodes; i++) {
> > >  		vn = &vmap_nodes[i];
> > >  
> > >  		spin_lock(&vn->busy.lock);
> > >  		*va = __find_vmap_area_exceed_addr(addr, &vn->busy.root);
> > >  
> > >  		if (*va)
> > > -			if (!va_start_lowest || (*va)->va_start < va_start_lowest)
> > > +			if ((*va)->va_start < va_start_lowest)
> > >  				va_start_lowest = (*va)->va_start;
> > >  		spin_unlock(&vn->busy.lock);
> > >  	}
> > > @@ -1069,7 +1069,7 @@ find_vmap_area_exceed_addr_lock(unsigned long addr, struct vmap_area **va)
> > >  	 * been removed concurrently thus we need to proceed
> > >  	 * with next one what is a rare case.
> > >  	 */
> > > -	if (va_start_lowest) {
> > > +	if (va_start_lowest != ULONG_MAX) {
> > >  		vn = addr_to_node(va_start_lowest);
> > >  
> > >  		spin_lock(&vn->busy.lock);
> > > 
> > > 
> > To me it looks as incomplete. The "va_start_lowest" should be initialized
> > when repeat. Otherwise we can end up with an infinite repeating because
> > va_start_lowest != ULONG_MAX.
> 
> You are right. Anyway, it's just a suggestion from a different code
> style, please feel free to adjust it in or leave the patch as is.
> > 
>
OK!

Thank you.

--
Uladzislau Rezki