Re: [PATCH] vmalloc: Convert to XArray

[William Kucharski <william.kucharski@oracle.com>]

Again, nice - for the followup:

Reviewed-by: William Kucharski <william.kucharski@oracle.com>

> On Jun 3, 2020, at 9:33 PM, Matthew Wilcox <willy@infradead.org> wrote:
> 
> 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>
>