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 <[email protected]>
Tested-by: Omar Sandoval <[email protected]>
Reported-by: Jens Axboe <[email protected]>
Signed-off-by: Uladzislau Rezki (Sony) <[email protected]>
---
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
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 <[email protected]>
> Tested-by: Omar Sandoval <[email protected]>
> Reported-by: Jens Axboe <[email protected]>
> Signed-off-by: Uladzislau Rezki (Sony) <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> > Tested-by: Omar Sandoval <[email protected]>
> > Reported-by: Jens Axboe <[email protected]>
> > Signed-off-by: Uladzislau Rezki (Sony) <[email protected]>
> > ---
> > 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 <[email protected]>
>
Thank you!
--
Uladzislau Rezki
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 <[email protected]>
> >
> Thank you!
>
> --
> 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