Date: Mon, 23 Nov 2015 13:18:47 +0900
From: Minchan Kim <minchan@kernel.org>
To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: Kyeongdon Kim <kyeongdon.kim@lge.com>,
        Andrew Morton <akpm@linux-foundation.org>, ngupta@vflare.org,
        linux-kernel@vger.kernel.org, sergey.senozhatsky@gmail.com
Subject: Re: [PATCH v2] zram: Prevent page allocation failure during
 zcomp_strm_alloc
Message-ID: <20151123041847.GA23030@blaptop>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2778
Lines: 69

New thread

On Mon, Nov 23, 2015 at 12:14:00PM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> On (11/23/15 11:15), Minchan Kim wrote:
> [..]
> > >  static void *zcomp_lz4_create(void)
> > >  {
> > > -	return kzalloc(LZ4_MEM_COMPRESS, GFP_KERNEL);
> > > +	void *ret;
> > > +
> > > +	ret = kzalloc(LZ4_MEM_COMPRESS,
> > > +			__GFP_NORETRY|__GFP_NOWARN|__GFP_NOMEMALLOC);
> > > +	if (!ret)
> > > +		ret = vzalloc(LZ4_MEM_COMPRESS);
> > 
> > One thing I feel bad smell is that call vzalloc with GFP_KERNEL.
> > This function can be called in direct reclaim path with holding
> > fs lock and GFP_KERNEL can enter recursive reclaim path so
> > lockdep would complain theoretically if I don't miss something.
> > 
> 
> yes, GFP_KERNEL looks a bit fragile to me too. And may be zcomp_strm_alloc()
> and comp->backend->create() deserve GFP_NOFS. I believe I sent a patch doing
> this a while ago: https://lkml.org/lkml/2015/6/16/465

Sorry, I have missed that.
It's worth to fix that you proved it that could happen.
But when I read your patch, GFP_NOIO instead GFP_NOFS would
better way. Could you resend it?

> 
> > If it is true, we should fix several allocation flags in
> > zcomp_strm_alloc. I just want to record this warning for the future
> > in this thread so someone who is finding for the contribution
> > material will prove and fix it. :)
> 
> I can re-send the patch.
> 
> And, in case if you missed it, what's your opinion on the idea of
> reducing ->max_strm if we can't allocate new streams. Here:
> http://marc.info/?l=linux-kernel&m=144798049429861

Hmm, auto backoff max_comp_streams with warn to user, I'm not sure
we really need it. Alloc failure is depenent on the workload and
timing so although there are a fail in t1, it could be successful
in t2 so if we *really* want to introduce auto backoff, the logic
should include advance routine as well as backoff. With that,
I want to handle it traparently without notice to user.

*HOWEVER*, I think the problem Kyeongdon said came from warning and
memset by __GFP_ZERO flood. Other overheads for alloc/free would be
not heavy because they are higly optimized path in the MM part.
About reclaiming part, it should be almost no problem because
reclaimer cannot make forward progress by deadlock so it doesn't
scan any LRU.

So, Kyeongdon's patch will remove warning overhead and likely to
make zcomp_stram_alloc successful with vmalloc so I want to
roll it out first. And let's add a WARN_ON_ONCE to detect of
failure and rethink it when we receive such report.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/