Re: [patch 2/6] mm/vmalloc: Avoid iterating over per CPU vmap blocks twice

From: Christoph Hellwig <hch@lst.de>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
	Christoph Hellwig <hch@lst.de>,
	Uladzislau Rezki <urezki@gmail.com>,
	Lorenzo Stoakes <lstoakes@gmail.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Baoquan He <bhe@redhat.com>
Subject: Re: [patch 2/6] mm/vmalloc: Avoid iterating over per CPU vmap blocks twice
Date: Tue, 23 May 2023 17:21:10 +0200	[thread overview]
Message-ID: <20230523152110.GB12341@lst.de> (raw)
In-Reply-To: <20230523140002.634591885@linutronix.de>

On Tue, May 23, 2023 at 04:02:12PM +0200, Thomas Gleixner wrote:
> _vunmap_aliases() walks the per CPU xarrays to find partially unmapped
> blocks and then walks the per cpu free lists to purge fragmented blocks.
> 
> Arguably that's waste of CPU cycles and cache lines as the full xarray walk
> already touches every block.
> 
> Avoid this double iteration:
> 
>   - Split out the code to purge one block and the code to free the local
>     purge list into helper functions.
> 
>   - Try to purge the fragmented blocks in the xarray walk before looking at
>     their dirty space.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  mm/vmalloc.c |   66 ++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 23 deletions(-)
> 
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2086,39 +2086,52 @@ static void free_vmap_block(struct vmap_
>  	kfree_rcu(vb, rcu_head);
>  }
>  
> +static bool purge_fragmented_block(struct vmap_block *vb, struct vmap_block_queue *vbq,
> +				   struct list_head *purge_list)

Please stick to 80 character lines for the vmalloc code.  And while
it's personal preference, two tab indents for the continuation make
prototypes like this a lot more readable.
a lot easier if you 

> +	if (!(vb->free + vb->dirty == VMAP_BBMAP_BITS && vb->dirty != VMAP_BBMAP_BITS))
> +		return false;

This comes from the old code, but it does looks almost intentionally
obsfucated..

	if (vb->free + vb->dirty != VMAP_BBMAP_BITS ||
	    vb->dirty == VMAP_BBMAP_BITS)

actually gets the logic across much better.

> +	 /* prevent further allocs after releasing lock */
> +	vb->free = 0;
> +	 /* prevent purging it again */

extra spaces before the comments.

Otherwise the refactoring looks nice, thanks.