On Mon, Jun 05 2023 at 21:58, Lorenzo Stoakes wrote:
> On Sat, May 27, 2023 at 08:28:28PM +0100, Lorenzo Stoakes wrote:
>> > static bool purge_fragmented_block(struct vmap_block *vb,
>> > - struct vmap_block_queue *vbq, struct list_head *purge_list)
>> > + struct vmap_block_queue *vbq, struct list_head *purge_list,
>> > + bool force_purge)
>> > {
>> > if (vb->free + vb->dirty != VMAP_BBMAP_BITS ||
>> > vb->dirty == VMAP_BBMAP_BITS)
>> > return false;
>> >
>> > + /* Don't overeagerly purge usable blocks unless requested */
>> > + if (!force_purge && vb->free < VMAP_PURGE_THRESHOLD)
>>
>> Little confused by this - if free capacity is less than the threshold, we
>> _don't_ purge? I thought we should purge in this instance? Should
>> this be an >=?
Yes. Stupid me.
> I'm pretty convinced this is incorrect, so here's a -fix patch to avoid
> this staying in vmalloc by accident as Andrew has already taken it (using
> De Morgan's law for clarity):-
>
> ----8<----
> From aa49ce7d5ca7937cfdf8f910e50296cf932f16cb Mon Sep 17 00:00:00 2001
> From: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
> Date: Mon, 5 Jun 2023 21:44:20 +0100
> Subject: [PATCH] mm/vmalloc: Dont purge usable blocks unnecessarily
>
> We do NOT purge if this conditional evalutates true, so correct
> VMAP_PURGE_THRESHOLD check.
>
> Signed-off-by: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
> ---
> mm/vmalloc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index ab606a80f475..9d64a4098c36 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2102,7 +2102,7 @@ static bool purge_fragmented_block(struct vmap_block *vb,
> return false;
>
> /* Don't overeagerly purge usable blocks unless requested */
> - if (!force_purge && vb->free < VMAP_PURGE_THRESHOLD)
> + if (!(force_purge || vb->free < VMAP_PURGE_THRESHOLD))
Your initial suggestion to make this
if (!force_purge && vb->free >= VMAP_PURGE_THRESHOLD)
return false;
Is way more intuitive IMO.
Thanks,
tglx
