linux-kernel - Re: [RFC 2/3] mm/vmalloc: add support for __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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Oct 2021 09:49:08 +1100
From:   "NeilBrown" <neilb@...e.de>
To:     "Uladzislau Rezki" <urezki@...il.com>
Cc:     "Michal Hocko" <mhocko@...e.com>,
        "Uladzislau Rezki" <urezki@...il.com>,
        "Linux Memory Management List" <linux-mm@...ck.org>,
        "Dave Chinner" <david@...morbit.com>,
        "Andrew Morton" <akpm@...ux-foundation.org>,
        "Christoph Hellwig" <hch@...radead.org>,
        linux-fsdevel@...r.kernel.org,
        "LKML" <linux-kernel@...r.kernel.org>,
        "Ilya Dryomov" <idryomov@...il.com>,
        "Jeff Layton" <jlayton@...nel.org>
Subject: Re: [RFC 2/3] mm/vmalloc: add support for __GFP_NOFAIL

On Thu, 21 Oct 2021, Uladzislau Rezki wrote:
> > On Thu 21-10-21 21:13:35, Neil Brown wrote:
> > > On Thu, 21 Oct 2021, Uladzislau Rezki wrote:
> > > > On Wed, Oct 20, 2021 at 05:00:28PM +0200, Uladzislau Rezki wrote:
> > > > > >
> > > > > > On Wed 20-10-21 16:29:14, Uladzislau Rezki wrote:
> > > > > > > On Wed, Oct 20, 2021 at 4:06 PM Michal Hocko <mhocko@...e.com> wrote:
> > > > > > [...]
> > > > > > > > As I've said I am OK with either of the two. Do you or anybody have any
> > > > > > > > preference? Without any explicit event to wake up for neither of the two
> > > > > > > > is more than just an optimistic retry.
> > > > > > > >
> > > > > > > From power perspective it is better to have a delay, so i tend to say
> > > > > > > that delay is better.
> > > > > >
> > > > > > I am a terrible random number generator. Can you give me a number
> > > > > > please?
> > > > > >
> > > > > Well, we can start from one jiffy so it is one timer tick: schedule_timeout(1)
> > > > > 
> > > > A small nit, it is better to replace it by the simple msleep() call: msleep(jiffies_to_msecs(1));
> > > 
> > > I disagree.  I think schedule_timeout_uninterruptible(1) is the best
> > > wait to sleep for 1 ticl
> > > 
> > > msleep() contains
> > >   timeout = msecs_to_jiffies(msecs) + 1;
> > > and both jiffies_to_msecs and msecs_to_jiffies might round up too.
> > > So you will sleep for at least twice as long as you asked for, possible
> > > more.
> > 
> > That was my thinking as well. Not to mention jiffies_to_msecs just to do
> > msecs_to_jiffies right after which seems like a pointless wasting of
> > cpu cycle. But maybe I was missing some other reasons why msleep would
> > be superior.
> >
> 
> To me the msleep is just more simpler from semantic point of view, i.e.
> it is as straight forward as it can be. In case of interruptable/uninteraptable
> sleep it can be more confusing for people.

I agree that msleep() is more simple.  I think adding the
jiffies_to_msec() substantially reduces that simplicity.

> 
> When it comes to rounding and possibility to sleep more than 1 tick, it
> really does not matter here, we do not need to guarantee exact sleeping
> time.
> 
> Therefore i proposed to switch to the msleep().

If, as you say, the precision doesn't matter that much, then maybe
   msleep(0)
which would sleep to the start of the next jiffy.  Does that look a bit
weird?  If so, the msleep(1) would be ok.

However now that I've thought about some more, I'd much prefer we
introduce something like
    memalloc_retry_wait();

and use that everywhere that a memory allocation is retried.
I'm not convinced that we need to wait at all - at least, not when
__GFP_DIRECT_RECLAIM is used, as in that case alloc_page will either
  - succeed
  - make some progress a reclaiming or
  - sleep

However I'm not 100% certain, and the behaviour might change in the
future.  So having one place (the definition of memalloc_retry_wait())
where we can change the sleeping behaviour if the alloc_page behavour
changes, would be ideal.  Maybe memalloc_retry_wait() could take a
gfpflags arg.

Thanks,
NeilBrown