All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: Dan Streetman <ddstreet@ieee.org>
Cc: Yu Zhao <yuzhao@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Seth Jennings <sjenning@redhat.com>,
	Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	Minchan Kim <minchan@kernel.org>, Nitin Gupta <ngupta@vflare.org>,
	Linux-MM <linux-mm@kvack.org>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Dan Streetman <dan.streetman@canonical.com>
Subject: Re: [PATCH] mm/zswap: use workqueue to destroy pool
Date: Fri, 29 Apr 2016 09:25:06 +0900	[thread overview]
Message-ID: <20160429002506.GA4920@swordfish> (raw)
In-Reply-To: <1461704891-15272-1-git-send-email-ddstreet@ieee.org>

On (04/26/16 17:08), Dan Streetman wrote:
> Add a work_struct to struct zswap_pool, and change __zswap_pool_empty
> to use the workqueue instead of using call_rcu().
> 
> When zswap destroys a pool no longer in use, it uses call_rcu() to
> perform the destruction/freeing.  Since that executes in softirq
> context, it must not sleep.  However, actually destroying the pool
> involves freeing the per-cpu compressors (which requires locking the
> cpu_add_remove_lock mutex) and freeing the zpool, for which the
> implementation may sleep (e.g. zsmalloc calls kmem_cache_destroy,
> which locks the slab_mutex).  So if either mutex is currently taken,
> or any other part of the compressor or zpool implementation sleeps, it
> will result in a BUG().
> 
> It's not easy to reproduce this when changing zswap's params normally.
> In testing with a loaded system, this does not fail:
> 
> $ cd /sys/module/zswap/parameters
> $ echo lz4 > compressor ; echo zsmalloc > zpool
> 
> nor does this:
> 
> $ while true ; do
> > echo lzo > compressor ; echo zbud > zpool
> > sleep 1
> > echo lz4 > compressor ; echo zsmalloc > zpool
> > sleep 1
> > done
> 
> although it's still possible either of those might fail, depending on
> whether anything else besides zswap has locked the mutexes.
> 
> However, changing a parameter with no delay immediately causes the
> schedule while atomic BUG:
> 
> $ while true ; do
> > echo lzo > compressor ; echo lz4 > compressor
> > done
> 
> This is essentially the same as Yu Zhao's proposed patch to zsmalloc,
> but moved to zswap, to cover compressor and zpool freeing.
> 
> Fixes: f1c54846ee45 ("zswap: dynamic pool creation")
> Reported-by: Yu Zhao <yuzhao@google.com>
> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
> Cc: Dan Streetman <dan.streetman@canonical.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: Dan Streetman <ddstreet@ieee.org>
Cc: Yu Zhao <yuzhao@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Seth Jennings <sjenning@redhat.com>,
	Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	Minchan Kim <minchan@kernel.org>, Nitin Gupta <ngupta@vflare.org>,
	Linux-MM <linux-mm@kvack.org>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Dan Streetman <dan.streetman@canonical.com>
Subject: Re: [PATCH] mm/zswap: use workqueue to destroy pool
Date: Fri, 29 Apr 2016 09:25:06 +0900	[thread overview]
Message-ID: <20160429002506.GA4920@swordfish> (raw)
In-Reply-To: <1461704891-15272-1-git-send-email-ddstreet@ieee.org>

On (04/26/16 17:08), Dan Streetman wrote:
> Add a work_struct to struct zswap_pool, and change __zswap_pool_empty
> to use the workqueue instead of using call_rcu().
> 
> When zswap destroys a pool no longer in use, it uses call_rcu() to
> perform the destruction/freeing.  Since that executes in softirq
> context, it must not sleep.  However, actually destroying the pool
> involves freeing the per-cpu compressors (which requires locking the
> cpu_add_remove_lock mutex) and freeing the zpool, for which the
> implementation may sleep (e.g. zsmalloc calls kmem_cache_destroy,
> which locks the slab_mutex).  So if either mutex is currently taken,
> or any other part of the compressor or zpool implementation sleeps, it
> will result in a BUG().
> 
> It's not easy to reproduce this when changing zswap's params normally.
> In testing with a loaded system, this does not fail:
> 
> $ cd /sys/module/zswap/parameters
> $ echo lz4 > compressor ; echo zsmalloc > zpool
> 
> nor does this:
> 
> $ while true ; do
> > echo lzo > compressor ; echo zbud > zpool
> > sleep 1
> > echo lz4 > compressor ; echo zsmalloc > zpool
> > sleep 1
> > done
> 
> although it's still possible either of those might fail, depending on
> whether anything else besides zswap has locked the mutexes.
> 
> However, changing a parameter with no delay immediately causes the
> schedule while atomic BUG:
> 
> $ while true ; do
> > echo lzo > compressor ; echo lz4 > compressor
> > done
> 
> This is essentially the same as Yu Zhao's proposed patch to zsmalloc,
> but moved to zswap, to cover compressor and zpool freeing.
> 
> Fixes: f1c54846ee45 ("zswap: dynamic pool creation")
> Reported-by: Yu Zhao <yuzhao@google.com>
> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
> Cc: Dan Streetman <dan.streetman@canonical.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

  parent reply	other threads:[~2016-04-29  0:23 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-29 22:02 [PATCH] zsmalloc: use workqueue to destroy pool in zpool callback Yu Zhao
2016-03-30  0:54 ` Sergey Senozhatsky
     [not found] ` <20160329235950.GA19927@bbox>
2016-03-31  8:46   ` Sergey Senozhatsky
2016-03-31  8:46     ` Sergey Senozhatsky
2016-03-31 21:46     ` Yu Zhao
2016-03-31 21:46       ` Yu Zhao
2016-03-31 22:05       ` Dan Streetman
2016-03-31 22:05         ` Dan Streetman
2016-04-25 21:20         ` [PATCH] mm/zpool: use workqueue for zpool_destroy Dan Streetman
2016-04-25 21:20           ` Dan Streetman
2016-04-25 21:46           ` Andrew Morton
2016-04-25 21:46             ` Andrew Morton
2016-04-25 22:18           ` Yu Zhao
2016-04-25 22:18             ` Yu Zhao
2016-04-26  0:59             ` Sergey Senozhatsky
2016-04-26  0:59               ` Sergey Senozhatsky
2016-04-26 11:07             ` Dan Streetman
2016-04-26 11:07               ` Dan Streetman
2016-04-26 21:08           ` [PATCH] mm/zswap: use workqueue to destroy pool Dan Streetman
2016-04-26 21:08             ` Dan Streetman
2016-04-27  0:58             ` Sergey Senozhatsky
2016-04-27  0:58               ` Sergey Senozhatsky
2016-04-27 17:19               ` Dan Streetman
2016-04-27 17:19                 ` Dan Streetman
2016-04-28  1:40                 ` Sergey Senozhatsky
2016-04-28  1:40                   ` Sergey Senozhatsky
2016-04-28  4:09                   ` Sergey Senozhatsky
2016-04-28  4:09                     ` Sergey Senozhatsky
2016-04-28  8:21                   ` Dan Streetman
2016-04-28  8:21                     ` Dan Streetman
2016-04-28  9:13                 ` [PATCH] mm/zswap: provide unique zpool name Dan Streetman
2016-04-28  9:13                   ` Dan Streetman
2016-04-28 22:16                   ` Andrew Morton
2016-04-28 22:16                     ` Andrew Morton
2016-04-29  0:25                   ` Sergey Senozhatsky
2016-04-29  0:25                     ` Sergey Senozhatsky
2016-04-29  0:25             ` Sergey Senozhatsky [this message]
2016-04-29  0:25               ` [PATCH] mm/zswap: use workqueue to destroy pool Sergey Senozhatsky

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=20160429002506.GA4920@swordfish \
    --to=sergey.senozhatsky.work@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=dan.streetman@canonical.com \
    --cc=ddstreet@ieee.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=minchan@kernel.org \
    --cc=ngupta@vflare.org \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=sjenning@redhat.com \
    --cc=yuzhao@google.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.