linux-kernel - Re: [PATCH 1/2] mm: Make alloc_contig

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 19 Feb 2021 12:00:38 -0800
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Oscar Salvador <osalvador@...e.de>, Michal Hocko <mhocko@...e.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>,
        Muchun Song <songmuchun@...edance.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

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.
>