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

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

[Uladzislau Rezki (Sony)]

A lockdep reports a possible deadlock in the find_vmap_area_exceed_addr_lock()
function:

============================================
WARNING: possible recursive locking detected
6.9.0-rc1-00060-ged3ccc57b108-dirty #6140 Not tainted
--------------------------------------------
drgn/455 is trying to acquire lock:
ffff0000c00131d0 (&vn->busy.lock/1){+.+.}-{2:2}, at: find_vmap_area_exceed_addr_lock+0x64/0x124

but task is already holding lock:
ffff0000c0011878 (&vn->busy.lock/1){+.+.}-{2:2}, at: find_vmap_area_exceed_addr_lock+0x64/0x124

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&vn->busy.lock/1);
  lock(&vn->busy.lock/1);

 *** DEADLOCK ***

indeed it can happen if the find_vmap_area_exceed_addr_lock()
gets called concurrently because it tries to acquire two nodes
locks. It was done to prevent removing a lowest VA found on a
previous step.

To address this a lowest VA is found first without holding a
node lock where it resides. As a last step we check if a VA
still there because it can go away, if removed, proceed with
next lowest.

Fixes: 53becf32aec1 ("mm: vmalloc: support multiple nodes in vread_iter")
Tested-by: Jens Axboe <axboe@kernel.dk>
Tested-by: Omar Sandoval <osandov@fb.com>
Reported-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 mm/vmalloc.c | 74 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 30 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index e94ce4562805..a5a5dfc3843e 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -989,6 +989,27 @@ unsigned long vmalloc_nr_pages(void)
 	return atomic_long_read(&nr_vmalloc_pages);
 }
 
+static struct vmap_area *__find_vmap_area(unsigned long addr, struct rb_root *root)
+{
+	struct rb_node *n = root->rb_node;
+
+	addr = (unsigned long)kasan_reset_tag((void *)addr);
+
+	while (n) {
+		struct vmap_area *va;
+
+		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;
+	}
+
+	return NULL;
+}
+
 /* Look up the first VA which satisfies addr < va_end, NULL if none. */
 static struct vmap_area *
 __find_vmap_area_exceed_addr(unsigned long addr, struct rb_root *root)
@@ -1025,47 +1046,40 @@ __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)
 {
-	struct vmap_node *vn, *va_node = NULL;
-	struct vmap_area *va_lowest;
+	unsigned long va_start_lowest;
+	struct vmap_node *vn;
 	int i;
 
-	for (i = 0; i < nr_vmap_nodes; i++) {
+repeat:
+	for (i = 0, va_start_lowest = 0; i < nr_vmap_nodes; i++) {
 		vn = &vmap_nodes[i];
 
 		spin_lock(&vn->busy.lock);
-		va_lowest = __find_vmap_area_exceed_addr(addr, &vn->busy.root);
-		if (va_lowest) {
-			if (!va_node || va_lowest->va_start < (*va)->va_start) {
-				if (va_node)
-					spin_unlock(&va_node->busy.lock);
-
-				*va = va_lowest;
-				va_node = vn;
-				continue;
-			}
-		}
+		*va = __find_vmap_area_exceed_addr(addr, &vn->busy.root);
+
+		if (*va)
+			if (!va_start_lowest || (*va)->va_start < va_start_lowest)
+				va_start_lowest = (*va)->va_start;
 		spin_unlock(&vn->busy.lock);
 	}
 
-	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.
+	 * 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.
+	 */
+	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;
 	}
 
 	return NULL;
-- 
2.39.2

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

[Baoquan He]

On 03/28/24 at 03:03pm, Uladzislau Rezki (Sony) wrote:
> A lockdep reports a possible deadlock in the find_vmap_area_exceed_addr_lock()
> function:
> 
> ============================================
> WARNING: possible recursive locking detected
> 6.9.0-rc1-00060-ged3ccc57b108-dirty #6140 Not tainted
> --------------------------------------------
> drgn/455 is trying to acquire lock:
> ffff0000c00131d0 (&vn->busy.lock/1){+.+.}-{2:2}, at: find_vmap_area_exceed_addr_lock+0x64/0x124
> 
> but task is already holding lock:
> ffff0000c0011878 (&vn->busy.lock/1){+.+.}-{2:2}, at: find_vmap_area_exceed_addr_lock+0x64/0x124
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(&vn->busy.lock/1);
>   lock(&vn->busy.lock/1);
> 
>  *** DEADLOCK ***
> 
> indeed it can happen if the find_vmap_area_exceed_addr_lock()
> gets called concurrently because it tries to acquire two nodes
> locks. It was done to prevent removing a lowest VA found on a
> previous step.
> 
> To address this a lowest VA is found first without holding a
> node lock where it resides. As a last step we check if a VA
> still there because it can go away, if removed, proceed with
> next lowest.
> 
> Fixes: 53becf32aec1 ("mm: vmalloc: support multiple nodes in vread_iter")
> Tested-by: Jens Axboe <axboe@kernel.dk>
> Tested-by: Omar Sandoval <osandov@fb.com>
> Reported-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  mm/vmalloc.c | 74 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 44 insertions(+), 30 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index e94ce4562805..a5a5dfc3843e 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -989,6 +989,27 @@ unsigned long vmalloc_nr_pages(void)
>  	return atomic_long_read(&nr_vmalloc_pages);
>  }
>  
> +static struct vmap_area *__find_vmap_area(unsigned long addr, struct rb_root *root)
> +{
> +	struct rb_node *n = root->rb_node;
> +
> +	addr = (unsigned long)kasan_reset_tag((void *)addr);
> +
> +	while (n) {
> +		struct vmap_area *va;
> +
> +		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;
> +	}
> +
> +	return NULL;
> +}
> +
>  /* Look up the first VA which satisfies addr < va_end, NULL if none. */
>  static struct vmap_area *
>  __find_vmap_area_exceed_addr(unsigned long addr, struct rb_root *root)
> @@ -1025,47 +1046,40 @@ __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)
>  {
> -	struct vmap_node *vn, *va_node = NULL;
> -	struct vmap_area *va_lowest;
> +	unsigned long va_start_lowest;
> +	struct vmap_node *vn;
>  	int i;
>  
> -	for (i = 0; i < nr_vmap_nodes; i++) {
> +repeat:
> +	for (i = 0, va_start_lowest = 0; i < nr_vmap_nodes; i++) {
>  		vn = &vmap_nodes[i];
>  
>  		spin_lock(&vn->busy.lock);
> -		va_lowest = __find_vmap_area_exceed_addr(addr, &vn->busy.root);
> -		if (va_lowest) {
> -			if (!va_node || va_lowest->va_start < (*va)->va_start) {
> -				if (va_node)
> -					spin_unlock(&va_node->busy.lock);
> -
> -				*va = va_lowest;
> -				va_node = vn;
> -				continue;
> -			}
> -		}
> +		*va = __find_vmap_area_exceed_addr(addr, &vn->busy.root);
> +
> +		if (*va)
> +			if (!va_start_lowest || (*va)->va_start < va_start_lowest)
> +				va_start_lowest = (*va)->va_start;

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);


>  		spin_unlock(&vn->busy.lock);
>  	}
>  
> -	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>

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

[Uladzislau Rezki]

On Fri, Mar 29, 2024 at 03:44:40PM +0800, Baoquan He wrote:
> On 03/28/24 at 03:03pm, Uladzislau Rezki (Sony) wrote:
> > A lockdep reports a possible deadlock in the find_vmap_area_exceed_addr_lock()
> > function:
> > 
> > ============================================
> > WARNING: possible recursive locking detected
> > 6.9.0-rc1-00060-ged3ccc57b108-dirty #6140 Not tainted
> > --------------------------------------------
> > drgn/455 is trying to acquire lock:
> > ffff0000c00131d0 (&vn->busy.lock/1){+.+.}-{2:2}, at: find_vmap_area_exceed_addr_lock+0x64/0x124
> > 
> > but task is already holding lock:
> > ffff0000c0011878 (&vn->busy.lock/1){+.+.}-{2:2}, at: find_vmap_area_exceed_addr_lock+0x64/0x124
> > 
> > other info that might help us debug this:
> >  Possible unsafe locking scenario:
> > 
> >        CPU0
> >        ----
> >   lock(&vn->busy.lock/1);
> >   lock(&vn->busy.lock/1);
> > 
> >  *** DEADLOCK ***
> > 
> > indeed it can happen if the find_vmap_area_exceed_addr_lock()
> > gets called concurrently because it tries to acquire two nodes
> > locks. It was done to prevent removing a lowest VA found on a
> > previous step.
> > 
> > To address this a lowest VA is found first without holding a
> > node lock where it resides. As a last step we check if a VA
> > still there because it can go away, if removed, proceed with
> > next lowest.
> > 
> > Fixes: 53becf32aec1 ("mm: vmalloc: support multiple nodes in vread_iter")
> > Tested-by: Jens Axboe <axboe@kernel.dk>
> > Tested-by: Omar Sandoval <osandov@fb.com>
> > Reported-by: Jens Axboe <axboe@kernel.dk>
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > ---
> >  mm/vmalloc.c | 74 +++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 44 insertions(+), 30 deletions(-)
> > 
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index e94ce4562805..a5a5dfc3843e 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -989,6 +989,27 @@ unsigned long vmalloc_nr_pages(void)
> >  	return atomic_long_read(&nr_vmalloc_pages);
> >  }
> >  
> > +static struct vmap_area *__find_vmap_area(unsigned long addr, struct rb_root *root)
> > +{
> > +	struct rb_node *n = root->rb_node;
> > +
> > +	addr = (unsigned long)kasan_reset_tag((void *)addr);
> > +
> > +	while (n) {
> > +		struct vmap_area *va;
> > +
> > +		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;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> >  /* Look up the first VA which satisfies addr < va_end, NULL if none. */
> >  static struct vmap_area *
> >  __find_vmap_area_exceed_addr(unsigned long addr, struct rb_root *root)
> > @@ -1025,47 +1046,40 @@ __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)
> >  {
> > -	struct vmap_node *vn, *va_node = NULL;
> > -	struct vmap_area *va_lowest;
> > +	unsigned long va_start_lowest;
> > +	struct vmap_node *vn;
> >  	int i;
> >  
> > -	for (i = 0; i < nr_vmap_nodes; i++) {
> > +repeat:
> > +	for (i = 0, va_start_lowest = 0; i < nr_vmap_nodes; i++) {
> >  		vn = &vmap_nodes[i];
> >  
> >  		spin_lock(&vn->busy.lock);
> > -		va_lowest = __find_vmap_area_exceed_addr(addr, &vn->busy.root);
> > -		if (va_lowest) {
> > -			if (!va_node || va_lowest->va_start < (*va)->va_start) {
> > -				if (va_node)
> > -					spin_unlock(&va_node->busy.lock);
> > -
> > -				*va = va_lowest;
> > -				va_node = vn;
> > -				continue;
> > -			}
> > -		}
> > +		*va = __find_vmap_area_exceed_addr(addr, &vn->busy.root);
> > +
> > +		if (*va)
> > +			if (!va_start_lowest || (*va)->va_start < va_start_lowest)
> > +				va_start_lowest = (*va)->va_start;
> 
> 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.

> >  	}
> >  
> > -	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

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

[Baoquan He]

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
> 

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

[Uladzislau Rezki]

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