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 0/4] Some cleanups for the KVA/vmalloc

[Uladzislau Rezki (Sony)]

Patch [1] removes an unused argument "node" from the __alloc_vmap_area()
function and that is it.

Patch [2] is not driven by any particular workload that fails or so,
it is just better approach to handle one specific split case.

Patch [3] some cleanups in merging path. Basically on a first step
the mergeable node is detached and there is no reason to "unlink" it.
The same concerns the second step unless it has been merged on first
one.

Patch [4] moves BUG_ON()/RB_EMPTY_NODE() checks under "unlink" logic.
After [3] merging path "unlink" only linked nodes. Therefore we can say
that removing detached object is a bug in all cases.

v1->v2:
    - update the commit message. [2] patch;
    - fix typos in comments. [2] patch;
    - do the "preload" for NUMA awareness. [2] patch;

Uladzislau Rezki (Sony) (4):
  mm/vmap: remove "node" argument
  mm/vmap: preload a CPU with one object for split purpose
  mm/vmap: get rid of one single unlink_va() when merge
  mm/vmap: move BUG_ON() check to the unlink_va()

 mm/vmalloc.c | 116 +++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 90 insertions(+), 26 deletions(-)

-- 
2.11.0

[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