diff mbox series

[v3,2/3] mm: vmalloc: Switch to find_unlink_vmap_area() in vm_unmap_ram()

Message ID 20221222190022.134380-2-urezki@gmail.com
State New
Headers
Series [v3,1/3] mm: vmalloc: Avoid calling __find_vmap_area() twice in __vunmap() |

Commit Message

Uladzislau Rezki Dec. 22, 2022, 7 p.m. UTC 
  Switch from find_vmap_area() to find_unlink_vmap_area() to prevent
a double access to the vmap_area_lock: one for finding area, second
time is for unlinking from a tree.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 mm/vmalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christoph Hellwig Dec. 23, 2022, 8:21 a.m. UTC | #1 
On Thu, Dec 22, 2022 at 08:00:21PM +0100, Uladzislau Rezki (Sony) wrote:
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2252,7 +2252,7 @@ void vm_unmap_ram(const void *mem, unsigned int count)
>  		return;
>  	}
>  
> -	va = find_vmap_area(addr);
> +	va = find_unlink_vmap_area(addr);
>  	BUG_ON(!va);
>  	debug_check_no_locks_freed((void *)va->va_start,
>  				    (va->va_end - va->va_start));

Don't we also need to remove the manual unlink that was done
here previously?   Actually it seems like that manual unlink is missing
after patch 1, creating a bisection hazard.  So either add it there,
or just fold this patch into the previous one.
Uladzislau Rezki Dec. 23, 2022, 4:41 p.m. UTC | #2 
> On Thu, Dec 22, 2022 at 08:00:21PM +0100, Uladzislau Rezki (Sony) wrote:
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -2252,7 +2252,7 @@ void vm_unmap_ram(const void *mem, unsigned int count)
> >  		return;
> >  	}
> >  
> > -	va = find_vmap_area(addr);
> > +	va = find_unlink_vmap_area(addr);
> >  	BUG_ON(!va);
> >  	debug_check_no_locks_freed((void *)va->va_start,
> >  				    (va->va_end - va->va_start));
> 
> Don't we also need to remove the manual unlink that was done
> here previously?   Actually it seems like that manual unlink is missing
> after patch 1, creating a bisection hazard.  So either add it there,
> or just fold this patch into the previous one.
>
Right. In terms of bisection it is not so good. I think folding is the
best.

Andrew, could you please fold this patch into the:

[PATCH v3 1/3] mm: vmalloc: Avoid calling __find_vmap_area() twice in __vunmap() ?

or should i send a v4 instead?

Thank you in advance!

--
Uladzislau Rezki
Andrew Morton Dec. 28, 2022, 11:47 p.m. UTC | #3 
On Fri, 23 Dec 2022 17:41:48 +0100 Uladzislau Rezki <urezki@gmail.com> wrote:

> > Don't we also need to remove the manual unlink that was done
> > here previously?   Actually it seems like that manual unlink is missing
> > after patch 1, creating a bisection hazard.  So either add it there,
> > or just fold this patch into the previous one.
> >
> Right. In terms of bisection it is not so good. I think folding is the
> best.
> 
> Andrew, could you please fold this patch into the:

which patch ;)

> [PATCH v3 1/3] mm: vmalloc: Avoid calling __find_vmap_area() twice in __vunmap() ?
> 
> or should i send a v4 instead?

Yes please.
Uladzislau Rezki Dec. 29, 2022, 12:47 p.m. UTC | #4 
On Wed, Dec 28, 2022 at 03:47:07PM -0800, Andrew Morton wrote:
> On Fri, 23 Dec 2022 17:41:48 +0100 Uladzislau Rezki <urezki@gmail.com> wrote:
> 
> > > Don't we also need to remove the manual unlink that was done
> > > here previously?   Actually it seems like that manual unlink is missing
> > > after patch 1, creating a bisection hazard.  So either add it there,
> > > or just fold this patch into the previous one.
> > >
> > Right. In terms of bisection it is not so good. I think folding is the
> > best.
> > 
> > Andrew, could you please fold this patch into the:
> 
> which patch ;)
> 
Currently the next-20221226 contains three patches:

<snip>
[1]
commit c83b70c3cc1ecf99897ca0ea6e44aa2125a61ccb
Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
Date:   Wed Dec 21 18:44:54 2022 +0100

    mm: vmalloc: replace BUG_ON() by WARN_ON_ONCE()

[2]
commit 8a85ea97b35924ee39d51e00ecb3f6d07f748a36
Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
Date:   Wed Dec 21 18:44:53 2022 +0100

    mm: vmalloc: switch to find_unlink_vmap_area() in vm_unmap_ram()

[3]
commit a7c84c673c71cdfad20fe25e5d2051ed229859f7
Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
Date:   Wed Dec 21 18:44:52 2022 +0100

    mm: vmalloc: avoid calling __find_vmap_area() twise in __vunmap()
<snip>

It would be good if you could fold [2] into [3] making it as one
patch. The problem is that, if we leave it as it is, the bisection
mechanism would consider [3] as a buggy patch, because it is not
fully accomplished and depends on [2].

Is that OK for you, i mean to squash on your own? Or i just should
resend one more time?

Thank you in advance!

--
Uladzislau Rezki
Andrew Morton Dec. 29, 2022, 11:17 p.m. UTC | #5 
On Thu, 29 Dec 2022 13:47:43 +0100 Uladzislau Rezki <urezki@gmail.com> wrote:

> [2]
> commit 8a85ea97b35924ee39d51e00ecb3f6d07f748a36
> Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> Date:   Wed Dec 21 18:44:53 2022 +0100
> 
>     mm: vmalloc: switch to find_unlink_vmap_area() in vm_unmap_ram()
> 
> [3]
> commit a7c84c673c71cdfad20fe25e5d2051ed229859f7
> Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> Date:   Wed Dec 21 18:44:52 2022 +0100
> 
>     mm: vmalloc: avoid calling __find_vmap_area() twise in __vunmap()
> <snip>
> 
> It would be good if you could fold [2] into [3] making it as one
> patch. The problem is that, if we leave it as it is, the bisection
> mechanism would consider [3] as a buggy patch, because it is not
> fully accomplished and depends on [2].
> 
> Is that OK for you, i mean to squash on your own? 

I did that.  I updated the "mm: vmalloc: avoid calling
__find_vmap_area() twice in __vunmap()" accordingly, thanks.
Uladzislau Rezki Dec. 31, 2022, 9:17 a.m. UTC | #6 
On Thu, Dec 29, 2022 at 03:17:06PM -0800, Andrew Morton wrote:
> On Thu, 29 Dec 2022 13:47:43 +0100 Uladzislau Rezki <urezki@gmail.com> wrote:
> 
> > [2]
> > commit 8a85ea97b35924ee39d51e00ecb3f6d07f748a36
> > Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > Date:   Wed Dec 21 18:44:53 2022 +0100
> > 
> >     mm: vmalloc: switch to find_unlink_vmap_area() in vm_unmap_ram()
> > 
> > [3]
> > commit a7c84c673c71cdfad20fe25e5d2051ed229859f7
> > Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > Date:   Wed Dec 21 18:44:52 2022 +0100
> > 
> >     mm: vmalloc: avoid calling __find_vmap_area() twise in __vunmap()
> > <snip>
> > 
> > It would be good if you could fold [2] into [3] making it as one
> > patch. The problem is that, if we leave it as it is, the bisection
> > mechanism would consider [3] as a buggy patch, because it is not
> > fully accomplished and depends on [2].
> > 
> > Is that OK for you, i mean to squash on your own? 
> 
> I did that.  I updated the "mm: vmalloc: avoid calling
> __find_vmap_area() twice in __vunmap()" accordingly, thanks.
> 
At least bisection part will not detect anything wrong now.

Happy New Year and thank you!

--
Uladzislau Rezki
diff mbox series

Patch

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index eb91ecaa7277..70e5000b9d68 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2252,7 +2252,7 @@  void vm_unmap_ram(const void *mem, unsigned int count)
 		return;
 	}
 
-	va = find_vmap_area(addr);
+	va = find_unlink_vmap_area(addr);
 	BUG_ON(!va);
 	debug_check_no_locks_freed((void *)va->va_start,
 				    (va->va_end - va->va_start));