linux-kernel - Re: [PATCH] mm: migrate: handle freed page at the first place

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: <20191112113716.GC2763@dhcp22.suse.cz>
Date:   Tue, 12 Nov 2019 12:37:16 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Yang Shi <yang.shi@...ux.alibaba.com>
Cc:     mgorman@...hsingularity.net, vbabka@...e.cz,
        akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: migrate: handle freed page at the first place

On Tue 12-11-19 09:04:01, Michal Hocko wrote:
> On Tue 12-11-19 06:09:25, Yang Shi wrote:
> > When doing migration if the freed page is met, we just return without
> > migrating it since it is pointless to migrate a freed page.  But, the
> > current code did two things before handling freed page:
> > 
> > 1. Return -ENOMEM if the page is THP and THP migration is not supported.
> > 2. Allocate target page unconditionally.
> > 
> > Both makes not too much sense.  If we handle freed page at the first place
> > we don't have to worry about allocating/freeing target page and split
> > THP at all.
> > 
> > For example (worst case) if we are trying to migrate a freed THP without
> > THP migration supported, the migrate_pages() would just split the THP then
> > retry to migrate base pages one by one by pointless allocating and freeing
> > pages, this is just waste of time.
> > 
> > I didn't run into any actual problem with the current code (or I may
> > just not notice it yet), it was found by visual inspection.
> 
> It would be preferable to accompany a change like this with some actual
> numbers. A race with page freeing should be a very rare situation. Maybe
> it is not under some workloads but that would better be checked and
> documented. I also do not like to do page state changes for THP
> migration without a support. I cannot really say this is 100% correct
> from top of my head and I do not see a sufficient justification to go
> and chase all those tiny details because that is time consuming.

And I forgot to mention one thing. I wouldn't be really opposed to
moving the allocation after the race check because that makes sense even
when the race is rare but moving the thp support check down is far from
clear without a much better justification. 
-- 
Michal Hocko
SUSE Labs