linux-kernel - Re: [RFC PATCH] treewide: remove GFP

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]

Message-ID: <20170831092909.kavmkrdisq7xx2eu@suse.de>
Date:   Thu, 31 Aug 2017 10:29:09 +0100
From:   Mel Gorman <mgorman@...e.de>
To:     Pavel Machek <pavel@....cz>
Cc:     Michal Hocko <mhocko@...nel.org>, NeilBrown <neilb@...e.com>,
        linux-mm@...ck.org, Matthew Wilcox <willy@...radead.org>,
        Vlastimil Babka <vbabka@...e.cz>, Neil Brown <neilb@...e.de>,
        Theodore Ts'o <tytso@....edu>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] treewide: remove GFP_TEMPORARY allocation flag

On Thu, Aug 31, 2017 at 11:07:22AM +0200, Pavel Machek wrote:
> > > > "This allocation is temporary. It lasts milliseconds, not hours."
> > > 
> > > It isn't sufficient to give a rule for when GFP_TEMPORARY will be used,
> > > you also need to explain (at least in general terms) how the information
> > > will be used.  Also you need to give guidelines on whether the flag
> > > should be set for allocation that will last seconds or minutes.
> > > 
> > > If we have a flag that doesn't have a well defined meaning that actually
> > > affects behavior, it will not be used consistently, and if we ever
> > > change exactly how it behaves we can expect things to break.  So it is
> > > better not to have a flag, than to have a poorly defined flag.
> > 
> > Absolutely agreed!
> > 
> > > My current thoughts is that the important criteria is not how long the
> > > allocation will be used for, but whether it is reclaimable.  Allocations
> > > that will only last 5 msecs are reclaimable by calling "usleep(5000)".
> > > Other allocations might be reclaimable in other ways.  Allocations that
> > > are not reclaimable may well be directed to a more restricted pool of
> > > memory, and might be more likely to fail.  If we grew a strong
> > > "reclaimable" concept, this 'temporary' concept that you want to hold on
> > > to would become a burden.
> > 
> > ... and here again. The whole motivation for the flag was to gather
> > these objects together and reduce chances of internal fragmentation
> > due to long lived objects mixed with short term ones. Without an
> > explicit way to reclaim those objects or having a clear checkpoint to
> > wait for it is not really helping us to reach desired outcome (less
> > fragmented memory).
> 
> Really?
> 
> If you group allocations that last << 1 second, and ones that last >>
> 1 second, I'm pretty sure it reduces fragmentation... "reclaimable" or
> not.
> 

If this was always done reliably then sure, it makes sense. At the time it
was introduced by me, proc was used to relay large amounts of information
to userspace. The patch had a noticable impact but on limited memory,
32-bit and this proc relay was in use. In retrospect, it's possible that
it was the monitoring itself that showed a "benefit" for the patch.

If the flag is used incorrectly even once then the value is diminished
and it can even cause harm (not as severe as misusing __GFP_MOVABLE but
harmful nonetheless). It only has a benefit if there is a large source of
temporary allocations that are long-lived enough to cause fragmentation
during small intervals and to be honest, directly measuring that is
extremely difficult. The benefit is too marginal, the potential for harm
is high and Michal is right to remove it.

-- 
Mel Gorman
SUSE Labs