Re: [PATCH 5/6] slab: Allocate frozen pages

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

On 6/1/22 14:14, David Hildenbrand wrote:
> On 31.05.22 19:33, Matthew Wilcox wrote:
>> On Tue, May 31, 2022 at 07:15:14PM +0200, David Hildenbrand wrote:
>>> On 31.05.22 17:06, Matthew Wilcox (Oracle) wrote:
>>>> Since slab does not use the page refcount, it can allocate and
>>>> free frozen pages, saving one atomic operation per free.
>>>
>>> I assume that implies that pages that are actually allocated *from* the
>>> buddy now have a refcount == 0.
>> 
>> Yes.
>> 
>>> IIRC, page isolation code (e.g., !page_ref_count check in
>>> has_unmovable_pages()) assumes that any page with a refcount of 0 is
>>> essentially either already free (buddy) or is on its way of getting
>>> freed (!buddy).
>> 
>> That would be a bad assumption.  We already freeze pages for things like
>> splitting, migration, and replacement with a THP.  If the usage is just
>> an optimisation, then that's OK (and maybe the optimisation needs to be
>> tweaked to check PageSlab), but if the code depends on that being true,
>> it was already broken.
> 
> Yes, it's an optimization to not go ahead and mess with the migratetype
> of pageblocks (especially, setting it MIGRATE_ISOLATE to later reset it
> to MIGRATE_MOVABLE) and so forth when it's obvious that there is
> unmovable data.
> 
> However, freezing the refcount -- as used in existing code -- is only a
> temporary thing. And it's frequently used on movable pages.

Agreed.

> If migration froze the refcount, the page is most certainly movable.
> THPs should be movable, so it doesn't matter if we froze the refcount or
> not. Pages that we collapse into a THP should be mostly LRU pages and,
> therefore, movable.
> 
> So the frozen refcount doesn't result in additional "false negatives" in
> e.g., has_unmovable_pages(), at least for these users.
> 
> 
>> 
>> For this particular case, I think has_unmovable_pages() is only called
>> for ZONE_MOVEABLE and Slab never allocates from ZONE_MOVEABLE, so it's
>> not an issue?
> 
> 
> has_unmovable_pages() is primarily useful for ZONE_NORMAL. For
> ZONE_MOVABLE, we really only check if there are any boot time
> allocations (PageReserved()).
> 
> For example, alloc_contig_range() implicitly calls has_unmovable_pages()
> when isolating pageblocks and ends up getting called on ZONE_NORMAL for
> example for runtime allocation of gigantic pages or via virtio-mem
> (nowadays even in pageblock granularity).
> 
> 
> Long story short, we should document accordingly what it actually means
> when we allocate without increasing the refcount, and that people should
> be careful with that. Regarding has_unmovable_pages() and frozen
> allocations, we might just be able to keep the existing detection
> unmodified by special-casing PageSlab() pages and detecting them as
> unmovable immediately.

I wonder if it makes sense to encourage long-term freezing anyway, if it
complicates other checks that could for now rely on the relatively simple
rules. For slab it might save some atomics, but allocating/freeing a new
slab page is already a very slow path, so that will be negligible.
So I wouldn't mind if only up to 4/6 of this series was merged.
OTOH once the API is available somebody will eventually use it for a
long-term frozen allocation (but they achieve do that now too, so maybe
not), so agree about documenting the implications.