Re: [PATCH] vmalloc: Convert to XArray

[Matthew Wilcox <willy@infradead.org>]

On Wed, Jun 03, 2020 at 11:35:24AM -0600, William Kucharski wrote:
> > -	err = radix_tree_preload(gfp_mask);
> > -	if (unlikely(err)) {
> > +	err = xa_insert(&vmap_blocks, vb_idx, vb, gfp_mask);
> > +	if (err) {
> 
> Should the "(err)" here be "unlikely(err)" as the radix tree version was?

That's a good question.  In previous years, GCC used to be stupider and
we had to help it out by annotating which paths were more or less likely
to be taken.  Now it generally makes the right decision (eg understanding
that an "early exit" which unwinds state from a function is likely to
be an error case and thus the slow path), so we no longer need to mark
nearly as much code with unlikely() as we used to.  Similar things are
true for prefetch() calls.

I took a look at the disassembly of this code with and without the
unlikely() and I also compared if (err) with if (err < 0).  In the end,
it makes no difference to the control flow (all variants jump to the end
of the function) although it changes the register allocation decisions
a little (!)

What did make a difference was moving all the error handling to the
end of the function; reduced the size of the function by 48 bytes.
This is with gcc-9.3.  I can submit this patch as a follow-up since it's
basically unrelated to the other change.

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 375bbb410a94..3d5b5c32c840 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1569,10 +1569,8 @@ static void *new_vmap_block(unsigned int order, gfp_t gfp_mask)
 	va = alloc_vmap_area(VMAP_BLOCK_SIZE, VMAP_BLOCK_SIZE,
 					VMALLOC_START, VMALLOC_END,
 					node, gfp_mask);
-	if (IS_ERR(va)) {
-		kfree(vb);
-		return ERR_CAST(va);
-	}
+	if (IS_ERR(va))
+		goto free_vb;
 
 	vaddr = vmap_block_vaddr(va->va_start, 0);
 	spin_lock_init(&vb->lock);
@@ -1587,11 +1585,8 @@ static void *new_vmap_block(unsigned int order, gfp_t gfp_mask)
 
 	vb_idx = addr_to_vb_idx(va->va_start);
 	err = xa_insert(&vmap_blocks, vb_idx, vb, gfp_mask);
-	if (err) {
-		kfree(vb);
-		free_vmap_area(va);
-		return ERR_PTR(err);
-	}
+	if (err < 0)
+		goto free_va;
 
 	vbq = &get_cpu_var(vmap_block_queue);
 	spin_lock(&vbq->lock);
@@ -1600,6 +1595,13 @@ static void *new_vmap_block(unsigned int order, gfp_t gfp_mask)
 	put_cpu_var(vmap_block_queue);
 
 	return vaddr;
+
+free_va:
+	free_vmap_area(va);
+	va = ERR_PTR(err);
+free_vb:
+	kfree(vb);
+	return ERR_CAST(va);
 }
 
 static void free_vmap_block(struct vmap_block *vb)

> Nice change and simplifies the code quite a bit.
> 
> Reviewed-by: William Kucharski <william.kucharski@oracle.com>