Re: [PATCH v2 3/4] mm/vmap: get rid of one single unlink_va() when merge

Re: [PATCH v2 3/4] mm/vmap: get rid of one single unlink_va() when merge

[Uladzislau Rezki]

On Mon, May 27, 2019 at 11:07:12AM +0800, Hillf Danton wrote:
> 
> On Mon, 27 May 2019 05:22:28 +0800 Uladzislau Rezki (Sony) wrote:
> > It does not make sense to try to "unlink" the node that is
> > definitely not linked with a list nor tree. On the first
> > merge step VA just points to the previously disconnected
> > busy area.
> > 
> > On the second step, check if the node has been merged and do
> > "unlink" if so, because now it points to an object that must
> > be linked.
> > 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > ---
> 
> Acked-by: Hillf Danton <hdanton@sina.com>
> 
Thanks!

> >  mm/vmalloc.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index b553047aa05b..6f91136f2cc8 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -718,9 +718,6 @@ merge_or_add_vmap_area(struct vmap_area *va,
> >  			/* Check and update the tree if needed. */
> >  			augment_tree_propagate_from(sibling);
> > 
> > -			/* Remove this VA, it has been merged. */
> > -			unlink_va(va, root);
> > -
> >  			/* Free vmap_area object. */
> >  			kmem_cache_free(vmap_area_cachep, va);
> > 
> > @@ -745,12 +742,12 @@ merge_or_add_vmap_area(struct vmap_area *va,
> >  			/* Check and update the tree if needed. */
> >  			augment_tree_propagate_from(sibling);
> >
> > -			/* Remove this VA, it has been merged. */
> > -			unlink_va(va, root);
> > +			/* Remove this VA, if it has been merged. */
> > +			if (merged)
> > +				unlink_va(va, root);
> >
> The change makes the code much easier to read, thanks.
> What is more, checking merged makes the polished comment unnecessary, imo.
> And it can be applied, I think, to the above hunk.
> 
That is odd. Will remove it.

--
Vlad Rezki

[PATCH v2 3/4] mm/vmap: get rid of one single unlink_va() when merge

[Uladzislau Rezki (Sony)]

It does not make sense to try to "unlink" the node that is
definitely not linked with a list nor tree. On the first
merge step VA just points to the previously disconnected
busy area.

On the second step, check if the node has been merged and do
"unlink" if so, because now it points to an object that must
be linked.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 mm/vmalloc.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index b553047aa05b..6f91136f2cc8 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -718,9 +718,6 @@ merge_or_add_vmap_area(struct vmap_area *va,
 			/* Check and update the tree if needed. */
 			augment_tree_propagate_from(sibling);
 
-			/* Remove this VA, it has been merged. */
-			unlink_va(va, root);
-
 			/* Free vmap_area object. */
 			kmem_cache_free(vmap_area_cachep, va);
 
@@ -745,12 +742,12 @@ merge_or_add_vmap_area(struct vmap_area *va,
 			/* Check and update the tree if needed. */
 			augment_tree_propagate_from(sibling);
 
-			/* Remove this VA, it has been merged. */
-			unlink_va(va, root);
+			/* Remove this VA, if it has been merged. */
+			if (merged)
+				unlink_va(va, root);
 
 			/* Free vmap_area object. */
 			kmem_cache_free(vmap_area_cachep, va);
-
 			return;
 		}
 	}
-- 
2.11.0