On 2/19/21 2:14 AM, Oscar Salvador wrote:
> On Fri, Feb 19, 2021 at 10:56:42AM +0100, Michal Hocko wrote:
>> OK, this should work but I am really wondering whether it wouldn't be
>> just simpler to replace the old page by a new one in the free list
>> directly. Or is there any reason we have to go through the generic
>> helpers path? I mean something like this
>>
>> new_page = alloc_fresh_huge_page();
>> if (!new_page)
>> goto fail;
>> spin_lock(hugetlb_lock);
>> if (!PageHuge(old_page)) {
Yes, something like this should work. I'll let Oscar work out the details.
One thing to note is that you also need to check for old_page not on the
free list here. It could have been allocated and in use. In addition,
make sure to check the new flag HPageFreed to ensure page is on free list
before doing any type of update_and_free_page call.
--
Mike Kravetz
>> /* freed from under us, nothing to do */
>> __update_and_free_page(new_page);
>> goto unlock;
>> }
>> list_del(&old_page->lru);
>> __update_and_free_page(old_page);
>> __enqueue_huge_page(new_page);
>> unlock:
>> spin_unlock(hugetlb_lock);
>>
>> This will require to split update_and_free_page and enqueue_huge_page to
>> counters independent parts but that shouldn't be a big deal. But it will
>> also protect from any races. Not an act of beauty but seems less hackish
>> to me.
>
> Yes, I think this would to the trick, and it is race-free.
> Let me play with it a bit and see what I can come up with.
>
> Thanks for the valuable insight.
>
