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

Christoph Hellwig
2023-05-23T15:21:16Z

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.