From: Michal Hocko <mhocko@kernel.org>
To: Mikulas Patocka <mpatocka@redhat.com>
Cc: Ondrej Kozina <okozina@redhat.com>,
Jerome Marchand <jmarchan@redhat.com>,
Stanislav Kozina <skozina@redhat.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
dm-devel@redhat.com
Subject: Re: System freezes after OOM
Date: Thu, 14 Jul 2016 16:59:37 +0200 [thread overview]
Message-ID: <20160714145937.GB12289@dhcp22.suse.cz> (raw)
In-Reply-To: <alpine.LRH.2.02.1607140952550.1102@file01.intranet.prod.int.rdu2.redhat.com>
On Thu 14-07-16 10:00:16, Mikulas Patocka wrote:
>
>
> On Thu, 14 Jul 2016, Michal Hocko wrote:
>
> > On Wed 13-07-16 11:02:15, Mikulas Patocka wrote:
>
> > > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> > > > index 4f3cb3554944..0b806810efab 100644
> > > > --- a/drivers/md/dm-crypt.c
> > > > +++ b/drivers/md/dm-crypt.c
> > > > @@ -1392,11 +1392,14 @@ static void kcryptd_async_done(struct crypto_async_request *async_req,
> > > > static void kcryptd_crypt(struct work_struct *work)
> > > > {
> > > > struct dm_crypt_io *io = container_of(work, struct dm_crypt_io, work);
> > > > + unsigned int pflags = current->flags;
> > > >
> > > > + current->flags |= PF_LESS_THROTTLE;
> > > > if (bio_data_dir(io->base_bio) == READ)
> > > > kcryptd_crypt_read_convert(io);
> > > > else
> > > > kcryptd_crypt_write_convert(io);
> > > > + tsk_restore_flags(current, pflags, PF_LESS_THROTTLE);
> > > > }
> > > >
> > > > static void kcryptd_queue_crypt(struct dm_crypt_io *io)
> > >
> > > ^^^ That fixes just one specific case - but there may be other threads
> > > doing mempool allocations in the device mapper subsystem - and you would
> > > need to mark all of them.
> >
> > Now that I am thinking about it some more. Are there any mempool users
> > which would actually want to be throttled? I would expect mempool users
> > are necessary to push IO through and throttle them sounds like a bad
> > decision in the first place but there might be other mempool users which
> > could cause issues. Anyway how about setting PF_LESS_THROTTLE
> > unconditionally inside mempool_alloc? Something like the following:
> >
> > diff --git a/mm/mempool.c b/mm/mempool.c
> > index 8f65464da5de..e21fb632983f 100644
> > --- a/mm/mempool.c
> > +++ b/mm/mempool.c
> > @@ -310,7 +310,8 @@ EXPORT_SYMBOL(mempool_resize);
> > */
> > void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
> > {
> > - void *element;
> > + unsigned int pflags = current->flags;
> > + void *element = NULL;
> > unsigned long flags;
> > wait_queue_t wait;
> > gfp_t gfp_temp;
> > @@ -327,6 +328,12 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
> >
> > gfp_temp = gfp_mask & ~(__GFP_DIRECT_RECLAIM|__GFP_IO);
> >
> > + /*
> > + * Make sure that the allocation doesn't get throttled during the
> > + * reclaim
> > + */
> > + if (gfpflags_allow_blocking(gfp_mask))
> > + current->flags |= PF_LESS_THROTTLE;
> > repeat_alloc:
> > if (likely(pool->curr_nr)) {
> > /*
> > @@ -339,7 +346,7 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
> >
> > element = pool->alloc(gfp_temp, pool->pool_data);
> > if (likely(element != NULL))
> > - return element;
> > + goto out;
> >
> > spin_lock_irqsave(&pool->lock, flags);
> > if (likely(pool->curr_nr)) {
> > @@ -352,7 +359,7 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
> > * for debugging.
> > */
> > kmemleak_update_trace(element);
> > - return element;
> > + goto out;
> > }
> >
> > /*
> > @@ -369,7 +376,7 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
> > /* We must not sleep if !__GFP_DIRECT_RECLAIM */
> > if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) {
> > spin_unlock_irqrestore(&pool->lock, flags);
> > - return NULL;
> > + goto out;
> > }
> >
> > /* Let's wait for someone else to return an element to @pool */
> > @@ -386,6 +393,10 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
> >
> > finish_wait(&pool->wait, &wait);
> > goto repeat_alloc;
> > +out:
> > + if (gfpflags_allow_blocking(gfp_mask))
> > + tsk_restore_flags(current, pflags, PF_LESS_THROTTLE);
> > + return element;
> > }
> > EXPORT_SYMBOL(mempool_alloc);
> >
>
> But it needs other changes to honor the PF_LESS_THROTTLE flag:
>
> static int current_may_throttle(void)
> {
> return !(current->flags & PF_LESS_THROTTLE) ||
> current->backing_dev_info == NULL ||
> bdi_write_congested(current->backing_dev_info);
> }
> --- if you set PF_LESS_THROTTLE, current_may_throttle may still return
> true if one of the other conditions is met.
That is true but doesn't that mean that the device is congested and
waiting a bit is the right thing to do?
> shrink_zone_memcg calls throttle_vm_writeout without checking
> PF_LESS_THROTTLE at all.
Yes it doesn't call it because it relies on
global_dirty_limits()->domain_dirty_limits() to DTRT. It will give the
caller with PF_LESS_THROTTLE some boost wrt. all other writers.
--
Michal Hocko
SUSE Labs
next prev parent reply other threads:[~2016-07-14 14:59 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <57837CEE.1010609@redhat.com>
[not found] ` <f80dc690-7e71-26b2-59a2-5a1557d26713@redhat.com>
[not found] ` <9be09452-de7f-d8be-fd5d-4a80d1cd1ba3@redhat.com>
2016-07-11 15:43 ` System freezes after OOM Mikulas Patocka
2016-07-12 6:49 ` Michal Hocko
2016-07-12 23:44 ` Mikulas Patocka
2016-07-13 8:35 ` Jerome Marchand
2016-07-13 11:14 ` Michal Hocko
2016-07-13 14:21 ` Mikulas Patocka
2016-07-13 11:10 ` Michal Hocko
2016-07-13 12:50 ` Michal Hocko
2016-07-13 13:44 ` Milan Broz
2016-07-13 15:21 ` Mikulas Patocka
2016-07-14 9:09 ` Michal Hocko
2016-07-14 9:46 ` Milan Broz
2016-07-13 15:02 ` Mikulas Patocka
2016-07-14 10:51 ` [dm-devel] " Ondrej Kozina
2016-07-14 12:51 ` Michal Hocko
2016-07-14 14:00 ` Mikulas Patocka
2016-07-14 14:59 ` Michal Hocko [this message]
2016-07-14 15:25 ` Ondrej Kozina
2016-07-14 17:35 ` Mikulas Patocka
2016-07-15 8:35 ` Michal Hocko
2016-07-15 12:11 ` Mikulas Patocka
2016-07-15 12:22 ` Michal Hocko
2016-07-15 17:02 ` Mikulas Patocka
2016-07-18 7:22 ` Michal Hocko
2016-07-14 14:08 ` Ondrej Kozina
2016-07-14 15:31 ` Michal Hocko
2016-07-14 17:07 ` Ondrej Kozina
2016-07-14 17:36 ` Michal Hocko
2016-07-14 17:39 ` Michal Hocko
2016-07-15 11:42 ` Tetsuo Handa
2016-07-13 13:19 ` Tetsuo Handa
2016-07-13 13:39 ` Michal Hocko
2016-07-13 14:18 ` Mikulas Patocka
2016-07-13 14:56 ` Michal Hocko
2016-07-13 15:11 ` Mikulas Patocka
2016-07-13 23:53 ` David Rientjes
2016-07-14 11:01 ` Tetsuo Handa
2016-07-14 12:29 ` Mikulas Patocka
2016-07-14 20:26 ` David Rientjes
2016-07-14 21:40 ` Tetsuo Handa
2016-07-14 22:04 ` David Rientjes
2016-07-15 11:25 ` Mikulas Patocka
2016-07-15 21:21 ` David Rientjes
2016-07-14 12:27 ` Mikulas Patocka
2016-07-14 20:22 ` David Rientjes
2016-07-15 11:21 ` Mikulas Patocka
2016-07-15 21:25 ` David Rientjes
2016-07-15 21:39 ` Mikulas Patocka
2016-07-15 21:58 ` David Rientjes
2016-07-15 23:53 ` Mikulas Patocka
2016-07-18 15:14 ` Johannes Weiner
2016-07-14 15:29 ` Michal Hocko
2016-07-14 20:38 ` David Rientjes
2016-07-15 7:22 ` Michal Hocko
2016-07-15 8:23 ` Michal Hocko
2016-07-15 12:00 ` Mikulas Patocka
2016-07-15 21:47 ` David Rientjes
2016-07-18 7:39 ` Michal Hocko
2016-07-18 21:03 ` David Rientjes
2016-07-14 0:01 ` David Rientjes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160714145937.GB12289@dhcp22.suse.cz \
--to=mhocko@kernel.org \
--cc=dm-devel@redhat.com \
--cc=jmarchan@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mpatocka@redhat.com \
--cc=okozina@redhat.com \
--cc=skozina@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).