linux-kernel - Re: [PATCH v3 2/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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210226102424.GA3557@linux>
Date:   Fri, 26 Feb 2021 11:24:29 +0100
From:   Oscar Salvador <osalvador@...e.de>
To:     Michal Hocko <mhocko@...e.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        David Hildenbrand <david@...hat.com>,
        Muchun Song <songmuchun@...edance.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] mm: Make alloc_contig_range handle in-use hugetlb
 pages

On Fri, Feb 26, 2021 at 09:46:57AM +0100, Michal Hocko wrote:
> On Mon 22-02-21 14:51:37, Oscar Salvador wrote:
> [...]
> > @@ -2394,9 +2397,19 @@ bool isolate_or_dissolve_huge_page(struct page *page)
> >  	 */
> >  	if (hstate_is_gigantic(h))
> >  		return ret;
> > -
> > -	if (!page_count(head) && alloc_and_dissolve_huge_page(h, head))
> > +retry:
> > +	if (page_count(head) && isolate_huge_page(head, list)) {
> >  		ret = true;
> > +	} else if (!page_count(head)) {
> 
> This is rather head spinning. Do we need to test page_count in the else
> branch? Do you want to optimize for a case where the page cannot be
> isolated because of page_huge_active?

Well, I wanted to explictly call out both cases.
We either 1) have an in-use page and we try to issolate it or 2) we have a free
page (count == 0).

If the page could not be dissolved due to page_huge_active, this would either
mean that page is about to be freed, or that someone has already issolated the
page.
Being the former case, one could say that falling-through alloc_and_dissolve is
ok.

But no, I did not really want to optimize anything here, just wanted to be explicit
about what we are checking and why.

> > +
> > +		if (!err) {
> > +			ret = true;
> > +		} else if (err == -EBUSY && try_again) {
> > +			try_again = false;
> > +			goto retry;
> > +		}
> 
> Is this retry once logic really needed? Does it really give us any real
> benefit? alloc_and_dissolve_huge_page already retries when the page is
> being freed.

alloc_and_dissolve_huge_page retries when the page is about to be freed.
Here we retry in case alloc_and_dissolve_huge_page() noticed that someone grabbed
the page (refcount 0 -> 1), which would possibly mean userspace started using this
page. If that is the case, we give isolate_huge_page another chance to see if we
can succeed and we can migrate the page.

Chances this to happen? Honestly, as any race, quite low.
So, the answer to your question would be, no, it is not really needed, I just felt
we could play "clever" here.

-- 
Oscar Salvador
SUSE L3