Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1275044 > unrolled thread

Re: [PATCH v2] zram: Prevent page allocation failure during zcomp_strm_alloc

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH v2] zram: Prevent page allocation failure during  zcomp_strm_alloc Minchan Kim <minchan@kernel.org> - 2015-11-23 05:20 +0100
    Re: [PATCH v2] zram: Prevent page allocation failure during  zcomp_strm_alloc Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-11-23 08:50 +0100
      Re: [PATCH v2] zram: Prevent page allocation failure during  zcomp_strm_alloc Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-11-23 09:20 +0100

#1275044 — Re: [PATCH v2] zram: Prevent page allocation failure during zcomp_strm_alloc

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/

[toc] | [next] | [standalone]

#1275080

On (11/23/15 13:18), Minchan Kim wrote:
[..]
> > 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?

no problem.

agree. we also would want to switch from vzalloc() to
	__vmalloc_node_flags(size, NUMA_NO_NODE,
			GFP_NOIO | __GFP_HIGHMEM |  __GFP_ZERO)

in fallbacks. I'll send the patch later today.

> > 
> > > 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.

yes. auto roll-back is important (that's why I mentioned it). the idea
is to avoid stealing of pages for streams. for example, in case of low
memory decrease ->max_strm to
	MAX(->avail_strm, ->max_strm, online cpus() / 2)

and even probably release some idle streams (um... as a reaction
to shrinker call???). or at least prevent setting of ->max_sgtrm to
some unreasonably huge values... "> 4 * NR_CPUS", for instance. but
this is not so critical.

> 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.

	-ss
--
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/

[toc] | [prev] | [next] | [standalone]

#1275105

On (11/23/15 16:43), Sergey Senozhatsky wrote:
[..]
> agree. we also would want to switch from vzalloc() to
> 	__vmalloc_node_flags(size, NUMA_NO_NODE,
> 			GFP_NOIO | __GFP_HIGHMEM |  __GFP_ZERO)

[..]
> > 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.

hm... for k{z,m}alloc() it does reduce the warning overhead, but not
for vmalloc() -> warn_alloc_failed() [in theory]. So I guess, I'll
change vmalloc() to

	__vmalloc(XXX,
		GFP_NOIO | __GFP_NOWARN | __GFP_HIGHMEM | __GFP_ZERO,
		PAGE_KERNEL);

/* passing __GFP_NOWARN */.

	-ss
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web