Re: [PATCH 2/3] zram: try vmalloc() after kmalloc()

2015-11-24 Thread Minchan Kim
On Tue, Nov 24, 2015 at 05:07:34PM +0900, Sergey Senozhatsky wrote:
> On (11/24/15 16:57), Minchan Kim wrote:
> [..]
> > > hm, ok, may be.
> > > but the question whether we want to waste pages on additional streams
> > > (especially, e.g. if we already have, say, 10 streams) is still valid.
> > > a more intuitive here is to release some unneeded streams, not to increase
> > > our pressure allocating new ones. well, at least it seems to be so.
> > > those pages can be used by zsmalloc, for example.
> > 
> > I think your claim make sense if the failure comes from high memory
> > pressure but order-3 alloc failure even if there are lots of order-0
> > free pages in my experience is easy to encouter so I think it doesn't
> > mean memory pressure but just memory fragmentation.
> 
> hm, yes, fragmentation can be one of the reasons.
> 
> > > > > ... and add GFP_NOIO to both kzalloc() and __vmalloc().
> > > > 
> > > > I can add it. The harmness is really ignorable but as I mentioned
> > > > at reply of Andrew, what's the benefit with GFP_NOIO?
> > > > We couldn't make forward progress with __GFP_RECLAIM in reclaim
> > > > context.
> > > 
> > > aha, I probably missed that out.
> > > (well, and, technically, things can change).
> > 
> > My speaking came from MM internal knowledge so I accept your concern.
> > if you prefer like GFP_NOIO, I will use it in next spin which
> > makes reader less confused.
> 
> ok, I found your comment
> 
> : It would be void *most of time* because it is called in reclaim context
> : and reclaim path bails out to avoid recursion of direct reclaim
> : by PF_MEMALLOC without trying reclaim.
> : However, the reason I said *most of time* is we has another context
> : the funcion could be called.
> 
> well, we also allocate streams from sysfs store and during 'normal' IO
> (e.g. from fs). wouldn't GFP_NOIO be helpful there?

In [3/3], I used GFP_KERNEL for sysfs_store but with FS, you're
absoultely right. I have no reason not to use GFP_NOIO in there.
Thanks for the review!

> 
>   -ss

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


Re: [PATCH 2/3] zram: try vmalloc() after kmalloc()

2015-11-24 Thread Sergey Senozhatsky
On (11/24/15 16:57), Minchan Kim wrote:
[..]
> > hm, ok, may be.
> > but the question whether we want to waste pages on additional streams
> > (especially, e.g. if we already have, say, 10 streams) is still valid.
> > a more intuitive here is to release some unneeded streams, not to increase
> > our pressure allocating new ones. well, at least it seems to be so.
> > those pages can be used by zsmalloc, for example.
> 
> I think your claim make sense if the failure comes from high memory
> pressure but order-3 alloc failure even if there are lots of order-0
> free pages in my experience is easy to encouter so I think it doesn't
> mean memory pressure but just memory fragmentation.

hm, yes, fragmentation can be one of the reasons.

> > > > ... and add GFP_NOIO to both kzalloc() and __vmalloc().
> > > 
> > > I can add it. The harmness is really ignorable but as I mentioned
> > > at reply of Andrew, what's the benefit with GFP_NOIO?
> > > We couldn't make forward progress with __GFP_RECLAIM in reclaim
> > > context.
> > 
> > aha, I probably missed that out.
> > (well, and, technically, things can change).
> 
> My speaking came from MM internal knowledge so I accept your concern.
> if you prefer like GFP_NOIO, I will use it in next spin which
> makes reader less confused.

ok, I found your comment

: It would be void *most of time* because it is called in reclaim context
: and reclaim path bails out to avoid recursion of direct reclaim
: by PF_MEMALLOC without trying reclaim.
: However, the reason I said *most of time* is we has another context
: the funcion could be called.

well, we also allocate streams from sysfs store and during 'normal' IO
(e.g. from fs). wouldn't GFP_NOIO be helpful there?

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


Re: [PATCH 2/3] zram: try vmalloc() after kmalloc()

2015-11-23 Thread Minchan Kim
On Tue, Nov 24, 2015 at 04:36:36PM +0900, Sergey Senozhatsky wrote:
> On (11/24/15 16:03), Minchan Kim wrote:
> > 
> > Good question.
> > 
> > Actually, failure of allocation came from backend->create as Kyeongdon's
> > comment because it requires order-3 allocation which is very fragile
> > in embedded system recenlty(Android, webOS. That's why Joonsoo are solving
> > the trouble by fixing compaction part). Otherwise, other kmalloc/vmalloc
> > stuff in our example would be almost no trouble in real practice(Of course,
> > if you says it might, you're absolutely right. It could fail but I think
> > it's *really* rare in real practice).
> > 
> > More concern is order-1 allocation rather than kmalloc/vmalloc.
> > I've got lots of allocation failure reports from product team until now
> > and frankly speaking, I don't get such order-1 fail report until now.
> > I guess the reason is that system is almost trobule due to heavy 
> > fragmentation
> > before the notice such failure.
> > 
> > So, I think if we solve order-3 allocation in backend->create,
> > above problem will be almost solved.
> 
> hm, ok, may be.
> but the question whether we want to waste pages on additional streams
> (especially, e.g. if we already have, say, 10 streams) is still valid.
> a more intuitive here is to release some unneeded streams, not to increase
> our pressure allocating new ones. well, at least it seems to be so.
> those pages can be used by zsmalloc, for example.

I think your claim make sense if the failure comes from high memory
pressure but order-3 alloc failure even if there are lots of order-0
free pages in my experience is easy to encouter so I think it doesn't
mean memory pressure but just memory fragmentation.

> 
> > > and I'd prefer it to be a bit different -- use likely path first and
> > > avoid an assignment in unlikely path.
> > 
> > Personally, I like one return case unless other is really better for
> > performance. I have trained it for Andrew, I belive. :)
> > But if you don't like this by performance reason, I will add unlikely
> > for vmalloc path. If you hate it just by personal preferenece, please
> > I want to stick my code.
> 
> no, I don't hate it.

Thanks!

> 
> > > ... and add GFP_NOIO to both kzalloc() and __vmalloc().
> > 
> > I can add it. The harmness is really ignorable but as I mentioned
> > at reply of Andrew, what's the benefit with GFP_NOIO?
> > We couldn't make forward progress with __GFP_RECLAIM in reclaim
> > context.
> 
> aha, I probably missed that out.
> (well, and, technically, things can change).

My speaking came from MM internal knowledge so I accept your concern.
if you prefer like GFP_NOIO, I will use it in next spin which
makes reader less confused.
Thanks!

> 
>   -ss

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


Re: [PATCH 2/3] zram: try vmalloc() after kmalloc()

2015-11-23 Thread Sergey Senozhatsky
On (11/24/15 16:03), Minchan Kim wrote:
> 
> Good question.
> 
> Actually, failure of allocation came from backend->create as Kyeongdon's
> comment because it requires order-3 allocation which is very fragile
> in embedded system recenlty(Android, webOS. That's why Joonsoo are solving
> the trouble by fixing compaction part). Otherwise, other kmalloc/vmalloc
> stuff in our example would be almost no trouble in real practice(Of course,
> if you says it might, you're absolutely right. It could fail but I think
> it's *really* rare in real practice).
> 
> More concern is order-1 allocation rather than kmalloc/vmalloc.
> I've got lots of allocation failure reports from product team until now
> and frankly speaking, I don't get such order-1 fail report until now.
> I guess the reason is that system is almost trobule due to heavy fragmentation
> before the notice such failure.
> 
> So, I think if we solve order-3 allocation in backend->create,
> above problem will be almost solved.

hm, ok, may be.
but the question whether we want to waste pages on additional streams
(especially, e.g. if we already have, say, 10 streams) is still valid.
a more intuitive here is to release some unneeded streams, not to increase
our pressure allocating new ones. well, at least it seems to be so.
those pages can be used by zsmalloc, for example.

> > and I'd prefer it to be a bit different -- use likely path first and
> > avoid an assignment in unlikely path.
> 
> Personally, I like one return case unless other is really better for
> performance. I have trained it for Andrew, I belive. :)
> But if you don't like this by performance reason, I will add unlikely
> for vmalloc path. If you hate it just by personal preferenece, please
> I want to stick my code.

no, I don't hate it.

> > ... and add GFP_NOIO to both kzalloc() and __vmalloc().
> 
> I can add it. The harmness is really ignorable but as I mentioned
> at reply of Andrew, what's the benefit with GFP_NOIO?
> We couldn't make forward progress with __GFP_RECLAIM in reclaim
> context.

aha, I probably missed that out.
(well, and, technically, things can change).

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


Re: [PATCH 2/3] zram: try vmalloc() after kmalloc()

2015-11-23 Thread Minchan Kim
On Tue, Nov 24, 2015 at 03:42:49PM +0900, Sergey Senozhatsky wrote:
> On (11/24/15 15:12), Sergey Senozhatsky wrote:
> > [..]
> > >  static void *zcomp_lz4_create(void)
> > >  {
> > > -   return kzalloc(LZ4_MEM_COMPRESS, GFP_KERNEL);
> > > +   void *ret;
> > > +
> > > +   /*
> > > +* This function could be called in swapout/fs write path
> > > +* so we couldn't use GFP_FS|IO. And it assumes we already
> > > +* have at least one stream in zram initialization so we
> > > +* don't do best effort to allocate more stream in here.
> > > +* A default stream will work well without further multiple
> > > +* stream. That's why we use  __GFP_NORETRY|NOWARN|NOMEMALLOC.
> > > +*/
> > > +   ret = kzalloc(LZ4_MEM_COMPRESS,
> > > +   __GFP_NORETRY|__GFP_NOWARN|__GFP_NOMEMALLOC);
> > > +   if (!ret)
> > > +   ret = __vmalloc(LZ4_MEM_COMPRESS,
> > > +   __GFP_NORETRY|__GFP_NOWARN|
> > > +   __GFP_NOMEMALLOC|__GFP_ZERO,
> > > +   PAGE_KERNEL);
> > > +   return ret;
> > >  }
> > [..]
> > 
> > so this change is still questionable. is there a real value in having
> > a vmalloc() fallback in the middle of allocations sequence:
> > 
> > zstrm = kmalloc(sizeof(*zstrm), GFP_NOIO);
> > ^^^ ok, can fail here
> > 
> 
> oh, and by the way, we still can get a warning from this guy. so NOWARN in
> ->create() only is not enough, should be propogated to zstrm alloc too.
> so is __get_free_pages() (that was not in the original patch).
> 
> so pass nowarn everywhere.
> 
> perhaps something like (__GFP_NORETRY... hm...):

Yep. zcomp_strm_alloc has several allocation side and they have used
different flags so in our discussion, there are a few time mistakes to
miss so I want to pass gfp_t from client to zcomp_strm_alloc and
all of allocation sites in zcomp_strm_alloc use the flag consistenly
like my [3/3].

Thanks.

> 
> ---
> 
>  drivers/block/zram/zcomp.c | 6 --
>  drivers/block/zram/zcomp_lz4.c | 3 ++-
>  drivers/block/zram/zcomp_lzo.c | 3 ++-
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index c536177..c0d02d5 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -76,7 +76,8 @@ static void zcomp_strm_free(struct zcomp *comp, struct 
> zcomp_strm *zstrm)
>   */
>  static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
>  {
> - struct zcomp_strm *zstrm = kmalloc(sizeof(*zstrm), GFP_NOIO);
> + struct zcomp_strm *zstrm = kmalloc(sizeof(*zstrm), GFP_NOIO |
> + __GFP_NORETRY | __GFP_NOWARN | __GFP_NOMEMALLOC);
>   if (!zstrm)
>   return NULL;
>  
> @@ -85,7 +86,8 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp 
> *comp)
>* allocate 2 pages. 1 for compressed data, plus 1 extra for the
>* case when compressed size is larger than the original one
>*/
> - zstrm->buffer = (void *)__get_free_pages(GFP_NOIO | __GFP_ZERO, 1);
> + zstrm->buffer = (void *)__get_free_pages(GFP_NOIO | __GFP_NOWARN |
> + __GFP_ZERO, 1);
>   if (!zstrm->private || !zstrm->buffer) {
>   zcomp_strm_free(comp, zstrm);
>   zstrm = NULL;
> diff --git a/drivers/block/zram/zcomp_lz4.c b/drivers/block/zram/zcomp_lz4.c
> index ee44b51..dbeee93 100644
> --- a/drivers/block/zram/zcomp_lz4.c
> +++ b/drivers/block/zram/zcomp_lz4.c
> @@ -15,7 +15,8 @@
>  
>  static void *zcomp_lz4_create(void)
>  {
> - return kzalloc(LZ4_MEM_COMPRESS, GFP_NOIO);
> + return kzalloc(LZ4_MEM_COMPRESS, GFP_NOIO | __GFP_NORETRY |
> + __GFP_NOWARN | __GFP_NOMEMALLOC);
>  }
>  
>  static void zcomp_lz4_destroy(void *private)
> diff --git a/drivers/block/zram/zcomp_lzo.c b/drivers/block/zram/zcomp_lzo.c
> index 683ce04..19c0857 100644
> --- a/drivers/block/zram/zcomp_lzo.c
> +++ b/drivers/block/zram/zcomp_lzo.c
> @@ -15,7 +15,8 @@
>  
>  static void *lzo_create(void)
>  {
> - return kzalloc(LZO1X_MEM_COMPRESS, GFP_NOIO);
> + return kzalloc(LZO1X_MEM_COMPRESS, GFP_NOIO | __GFP_NORETRY |
> + __GFP_NOWARN | __GFP_NOMEMALLOC);
>  }
>  
>  static void lzo_destroy(void *private)

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


Re: [PATCH 2/3] zram: try vmalloc() after kmalloc()

2015-11-23 Thread Minchan Kim
On Tue, Nov 24, 2015 at 03:12:58PM +0900, Sergey Senozhatsky wrote:
> [..]
> >  static void *zcomp_lz4_create(void)
> >  {
> > -   return kzalloc(LZ4_MEM_COMPRESS, GFP_KERNEL);
> > +   void *ret;
> > +
> > +   /*
> > +* This function could be called in swapout/fs write path
> > +* so we couldn't use GFP_FS|IO. And it assumes we already
> > +* have at least one stream in zram initialization so we
> > +* don't do best effort to allocate more stream in here.
> > +* A default stream will work well without further multiple
> > +* stream. That's why we use  __GFP_NORETRY|NOWARN|NOMEMALLOC.
> > +*/
> > +   ret = kzalloc(LZ4_MEM_COMPRESS,
> > +   __GFP_NORETRY|__GFP_NOWARN|__GFP_NOMEMALLOC);
> > +   if (!ret)
> > +   ret = __vmalloc(LZ4_MEM_COMPRESS,
> > +   __GFP_NORETRY|__GFP_NOWARN|
> > +   __GFP_NOMEMALLOC|__GFP_ZERO,
> > +   PAGE_KERNEL);
> > +   return ret;
> >  }
> [..]
> 
> so this change is still questionable. is there a real value in having
> a vmalloc() fallback in the middle of allocations sequence:
> 
>   zstrm = kmalloc(sizeof(*zstrm), GFP_NOIO);
>   ^^^ ok, can fail here
> 
>   zstrm->zstrm->private = comp->backend->create();
>   ^^^ kzalloc() and vmalloc() fallback ??
> 
>   zstrm->buffer = (void *)__get_free_pages(GFP_NOIO | __GFP_ZERO, 1);
>   ^^^ can fail here again.
> 
> can you please comment on this?

Good question.

Actually, failure of allocation came from backend->create as Kyeongdon's
comment because it requires order-3 allocation which is very fragile
in embedded system recenlty(Android, webOS. That's why Joonsoo are solving
the trouble by fixing compaction part). Otherwise, other kmalloc/vmalloc
stuff in our example would be almost no trouble in real practice(Of course,
if you says it might, you're absolutely right. It could fail but I think
it's *really* rare in real practice).

More concern is order-1 allocation rather than kmalloc/vmalloc.
I've got lots of allocation failure reports from product team until now
and frankly speaking, I don't get such order-1 fail report until now.
I guess the reason is that system is almost trobule due to heavy fragmentation
before the notice such failure.

So, I think if we solve order-3 allocation in backend->create,
above problem will be almost solved.

> 
> 
> and I'd prefer it to be a bit different -- use likely path first and
> avoid an assignment in unlikely path.

Personally, I like one return case unless other is really better for
performance. I have trained it for Andrew, I belive. :)
But if you don't like this by performance reason, I will add unlikely
for vmalloc path. If you hate it just by personal preferenece, please
I want to stick my code.


> ... and add GFP_NOIO to both kzalloc() and __vmalloc().

I can add it. The harmness is really ignorable but as I mentioned
at reply of Andrew, what's the benefit with GFP_NOIO?
We couldn't make forward progress with __GFP_RECLAIM in reclaim
context.

> 
> and there is no __GFP_HIGHMEM in __vmalloc() call?

Good to have. Thanks for the hint!

Thanks.

> 
> something like this:
> 
> ---
> 
> 
>   ret = kzalloc(LZ4_MEM_COMPRESS, GFP_NOIO | __GFP_NORETRY |
>   __GFP_NOWARN | __GFP_NOMEMALLOC);
>   if (ret)
>   return ret;
> 
>   return __vmalloc(LZ4_MEM_COMPRESS,
>   GFP_NOIO | __GFP_NOWARN | __GFP_HIGHMEM | __GFP_ZERO,
>   PAGE_KERNEL);
> 
> 
>   -ss

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


Re: [PATCH 2/3] zram: try vmalloc() after kmalloc()

2015-11-23 Thread Sergey Senozhatsky
On (11/24/15 15:12), Sergey Senozhatsky wrote:
> [..]
> >  static void *zcomp_lz4_create(void)
> >  {
> > -   return kzalloc(LZ4_MEM_COMPRESS, GFP_KERNEL);
> > +   void *ret;
> > +
> > +   /*
> > +* This function could be called in swapout/fs write path
> > +* so we couldn't use GFP_FS|IO. And it assumes we already
> > +* have at least one stream in zram initialization so we
> > +* don't do best effort to allocate more stream in here.
> > +* A default stream will work well without further multiple
> > +* stream. That's why we use  __GFP_NORETRY|NOWARN|NOMEMALLOC.
> > +*/
> > +   ret = kzalloc(LZ4_MEM_COMPRESS,
> > +   __GFP_NORETRY|__GFP_NOWARN|__GFP_NOMEMALLOC);
> > +   if (!ret)
> > +   ret = __vmalloc(LZ4_MEM_COMPRESS,
> > +   __GFP_NORETRY|__GFP_NOWARN|
> > +   __GFP_NOMEMALLOC|__GFP_ZERO,
> > +   PAGE_KERNEL);
> > +   return ret;
> >  }
> [..]
> 
> so this change is still questionable. is there a real value in having
> a vmalloc() fallback in the middle of allocations sequence:
> 
>   zstrm = kmalloc(sizeof(*zstrm), GFP_NOIO);
>   ^^^ ok, can fail here
> 

oh, and by the way, we still can get a warning from this guy. so NOWARN in
->create() only is not enough, should be propogated to zstrm alloc too.
so is __get_free_pages() (that was not in the original patch).

so pass nowarn everywhere.

perhaps something like (__GFP_NORETRY... hm...):

---

 drivers/block/zram/zcomp.c | 6 --
 drivers/block/zram/zcomp_lz4.c | 3 ++-
 drivers/block/zram/zcomp_lzo.c | 3 ++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index c536177..c0d02d5 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -76,7 +76,8 @@ static void zcomp_strm_free(struct zcomp *comp, struct 
zcomp_strm *zstrm)
  */
 static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
 {
-   struct zcomp_strm *zstrm = kmalloc(sizeof(*zstrm), GFP_NOIO);
+   struct zcomp_strm *zstrm = kmalloc(sizeof(*zstrm), GFP_NOIO |
+   __GFP_NORETRY | __GFP_NOWARN | __GFP_NOMEMALLOC);
if (!zstrm)
return NULL;
 
@@ -85,7 +86,8 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
 * allocate 2 pages. 1 for compressed data, plus 1 extra for the
 * case when compressed size is larger than the original one
 */
-   zstrm->buffer = (void *)__get_free_pages(GFP_NOIO | __GFP_ZERO, 1);
+   zstrm->buffer = (void *)__get_free_pages(GFP_NOIO | __GFP_NOWARN |
+   __GFP_ZERO, 1);
if (!zstrm->private || !zstrm->buffer) {
zcomp_strm_free(comp, zstrm);
zstrm = NULL;
diff --git a/drivers/block/zram/zcomp_lz4.c b/drivers/block/zram/zcomp_lz4.c
index ee44b51..dbeee93 100644
--- a/drivers/block/zram/zcomp_lz4.c
+++ b/drivers/block/zram/zcomp_lz4.c
@@ -15,7 +15,8 @@
 
 static void *zcomp_lz4_create(void)
 {
-   return kzalloc(LZ4_MEM_COMPRESS, GFP_NOIO);
+   return kzalloc(LZ4_MEM_COMPRESS, GFP_NOIO | __GFP_NORETRY |
+   __GFP_NOWARN | __GFP_NOMEMALLOC);
 }
 
 static void zcomp_lz4_destroy(void *private)
diff --git a/drivers/block/zram/zcomp_lzo.c b/drivers/block/zram/zcomp_lzo.c
index 683ce04..19c0857 100644
--- a/drivers/block/zram/zcomp_lzo.c
+++ b/drivers/block/zram/zcomp_lzo.c
@@ -15,7 +15,8 @@
 
 static void *lzo_create(void)
 {
-   return kzalloc(LZO1X_MEM_COMPRESS, GFP_NOIO);
+   return kzalloc(LZO1X_MEM_COMPRESS, GFP_NOIO | __GFP_NORETRY |
+   __GFP_NOWARN | __GFP_NOMEMALLOC);
 }
 
 static void lzo_destroy(void *private)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 2/3] zram: try vmalloc() after kmalloc()

2015-11-23 Thread Sergey Senozhatsky
[..]
>  static void *zcomp_lz4_create(void)
>  {
> -   return kzalloc(LZ4_MEM_COMPRESS, GFP_KERNEL);
> +   void *ret;
> +
> +   /*
> +* This function could be called in swapout/fs write path
> +* so we couldn't use GFP_FS|IO. And it assumes we already
> +* have at least one stream in zram initialization so we
> +* don't do best effort to allocate more stream in here.
> +* A default stream will work well without further multiple
> +* stream. That's why we use  __GFP_NORETRY|NOWARN|NOMEMALLOC.
> +*/
> +   ret = kzalloc(LZ4_MEM_COMPRESS,
> +   __GFP_NORETRY|__GFP_NOWARN|__GFP_NOMEMALLOC);
> +   if (!ret)
> +   ret = __vmalloc(LZ4_MEM_COMPRESS,
> +   __GFP_NORETRY|__GFP_NOWARN|
> +   __GFP_NOMEMALLOC|__GFP_ZERO,
> +   PAGE_KERNEL);
> +   return ret;
>  }
[..]

so this change is still questionable. is there a real value in having
a vmalloc() fallback in the middle of allocations sequence:

zstrm = kmalloc(sizeof(*zstrm), GFP_NOIO);
^^^ ok, can fail here

zstrm->zstrm->private = comp->backend->create();
^^^ kzalloc() and vmalloc() fallback ??

zstrm->buffer = (void *)__get_free_pages(GFP_NOIO | __GFP_ZERO, 1);
^^^ can fail here again.

can you please comment on this?


and I'd prefer it to be a bit different -- use likely path first and
avoid an assignment in unlikely path.
... and add GFP_NOIO to both kzalloc() and __vmalloc().

and there is no __GFP_HIGHMEM in __vmalloc() call?

something like this:

---


ret = kzalloc(LZ4_MEM_COMPRESS, GFP_NOIO | __GFP_NORETRY |
__GFP_NOWARN | __GFP_NOMEMALLOC);
if (ret)
return ret;

return __vmalloc(LZ4_MEM_COMPRESS,
GFP_NOIO | __GFP_NOWARN | __GFP_HIGHMEM | __GFP_ZERO,
PAGE_KERNEL);


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