Received-SPF: pass (google.com: domain of linux-kernel+bounces-125732-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sat, 30 Mar 2024 21:21:25 +0800
From: Baoquan He <bhe@redhat.com>
To: Uladzislau Rezki <urezki@gmail.com>
Cc: 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
Message-ID: <ZggR1fW0rAHSKF+j@MiWiFi-R3L-srv>
References: <20240328140330.4747-1-urezki@gmail.com>
 <ZgZxaOg6/qZAfUUA@MiWiFi-R3L-srv>
 <ZggLwbnaeK0AsRwQ@pc636>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZggLwbnaeK0AsRwQ@pc636>

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.
> 
> > >  	}
> > >  
> > > -	return va_node;
> > > -}
> > > -
> > > -static struct vmap_area *__find_vmap_area(unsigned long addr, struct rb_root *root)
> > > -{
> > > -	struct rb_node *n = root->rb_node;
> > > +	/*
> > > +	 * Check if found VA exists, it might it is gone away.
> >                                         ~~~~ grammer mistake?
> > > +	 * In this case we repeat the search because a VA has
> > > +	 * been removed concurrently thus we need to proceed
> > > +	 * with next one what is a rare case.
> >                          ~~~~ typo, which?
> > > +	 */
> > > +	if (va_start_lowest) {
> > > +		vn = addr_to_node(va_start_lowest);
> > >  
> > > -	addr = (unsigned long)kasan_reset_tag((void *)addr);
> > > +		spin_lock(&vn->busy.lock);
> > > +		*va = __find_vmap_area(va_start_lowest, &vn->busy.root);
> > >  
> > > -	while (n) {
> > > -		struct vmap_area *va;
> > > +		if (*va)
> > > +			return vn;
> > >  
> > > -		va = rb_entry(n, struct vmap_area, rb_node);
> > > -		if (addr < va->va_start)
> > > -			n = n->rb_left;
> > > -		else if (addr >= va->va_end)
> > > -			n = n->rb_right;
> > > -		else
> > > -			return va;
> > > +		spin_unlock(&vn->busy.lock);
> > > +		goto repeat;
> > >  	}
> > 
> > Other than above nickpick concerns, this looks good to me.
> > 
> > Reviewed-by: Baoquan He <bhe@redhat.com>
> > 
> Thank you!
> 
> --
> Uladzislau Rezki
>