On Tue 07-06-16 21:11:03, Tetsuo Handa wrote:
> On 2016/06/06 20:32, Michal Hocko wrote:
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 669fef1e2bb6..a4b0f18a69ab 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -707,7 +707,7 @@ static int vhost_memory_reg_sort_cmp(const void *p1, const void *p2)
> >
> > static void *vhost_kvzalloc(unsigned long size)
> > {
> > - void *n = kzalloc(size, GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT);
> > + void *n = kzalloc(size, GFP_KERNEL | __GFP_NOWARN | __GFP_RETRY_HARD);
>
> Remaining __GFP_REPEAT users are not always doing costly allocations.
Yes but...
> Sometimes they pass __GFP_REPEAT because the size is given from userspace.
> Thus, unconditional s/__GFP_REPEAT/__GFP_RETRY_HARD/g is not good.
Would that be a regression though? Strictly speaking the __GFP_REPEAT
documentation was explicit to not loop for ever. So nobody should have
expected nofail semantic pretty much by definition. The fact that our
previous implementation was not fully conforming to the documentation is
just an implementation detail. All the remaining users of __GFP_REPEAT
_have_ to be prepared for the allocation failure. So what exactly is the
problem with them?
> What I think more important is hearing from __GFP_REPEAT users how hard they
> want to retry. It is possible that they want to retry unless SIGKILL is
> delivered, but passing __GFP_NOFAIL is too hard, and therefore __GFP_REPEAT
> is used instead. It is possible that they use __GFP_NOFAIL || __GFP_KILLABLE
> if __GFP_KILLABLE were available. In my module (though I'm not using
> __GFP_REPEAT), I want to retry unless SIGKILL is delivered.
To be honest killability for a particular allocation request sounds
like a hack to me. Just consider the expected semantic. How do you
handle when one path uses explicit __GFP_KILLABLE while other path (from
the same syscall) is not... If anything this would have to be process
context wise.
[...]
> > /* Reclaim has failed us, start killing things */
> > page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
> > if (page)
> > @@ -3719,6 +3731,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> > /* Retry as long as the OOM killer is making progress */
> > if (did_some_progress) {
> > no_progress_loops = 0;
> > + passed_oom = true;
>
> This is too premature. did_some_progress != 0 after returning from
> __alloc_pages_may_oom() does not mean the OOM killer was invoked. It only means
> that mutex_trylock(&oom_lock) was attempted.
which means that we have reached the OOM condition and _somebody_ is
actaully handling the OOM on our behalf.
> It is possible that somebody else
> is on the way to call out_of_memory(). It is possible that the OOM reaper is
> about to start reaping memory. Giving up after 1 jiffie of sleep is too fast.
Sure this will always be racy. But the primary point is that we have
passed the OOM line and then passed through all the retries to get to
the same state again. This sounds like a pretty natural boundary to tell
we have tried hard enough to rather fail and let the caller handle the
fallback.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>