Re: [patch v2 1/5] mm: add nofail variants of kmalloc kcalloc and kzalloc

2010-09-06 Thread David Rientjes 
On Wed, 1 Sep 2010, David Rientjes wrote:

 Add kmalloc_nofail(), kcalloc_nofail(), and kzalloc_nofail().  These
 functions are equivalent to kmalloc(), kcalloc(), and kzalloc(),
 respectively, except that they will never return NULL and instead loop
 forever trying to allocate memory.
 
 If the first allocation attempt fails because the page allocator doesn't 
 implicitly loop, a warning will be emitted, including a call trace.
 Subsequent failures will suppress this warning.
 
 These were added as helper functions for documentation and auditability.
 No future callers should be added.
 

Are there any objections to merging this series through -mm with the 
exception of the fifth patch for ntfs?  That particular patch needs to 
have its WARN_ON_ONCE() condition rewritten since it fallbacks to 
vmalloc for high order allocs.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch v2 1/5] mm: add nofail variants of kmalloc kcalloc and kzalloc

2010-09-02 Thread Jiri Slaby 
On 09/02/2010 03:02 AM, David Rientjes wrote:
 --- a/include/linux/slab.h
 +++ b/include/linux/slab.h
 @@ -334,6 +334,57 @@ static inline void *kzalloc_node(size_t size, gfp_t 
 flags, int node)
   return kmalloc_node(size, flags | __GFP_ZERO, node);
  }
  
 +/**
 + * kmalloc_nofail - infinitely loop until kmalloc() succeeds.
 + * @size: how many bytes of memory are required.
 + * @flags: the type of memory to allocate (see kmalloc).
 + *
 + * NOTE: no new callers of this function should be implemented!
 + * All memory allocations should be failable whenever possible.
 + */
 +static inline void *kmalloc_nofail(size_t size, gfp_t flags)
 +{
 + void *ret;
 +
 + for (;;) {
 + ret = kmalloc(size, flags);
 + if (ret)
 + return ret;
 + WARN_ON_ONCE(get_order(size)  PAGE_ALLOC_COSTLY_ORDER);

This doesn't work as you expect. kmalloc will warn every time it fails.
__GFP_NOFAIL used to disable the warning. Actually what's wrong with
__GFP_NOFAIL? I cannot find a reason in the changelogs why the patches
are needed.

 + }



-- 
js
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch v2 1/5] mm: add nofail variants of kmalloc kcalloc and kzalloc

2010-09-02 Thread Jan Kara 
On Thu 02-09-10 09:59:13, Jiri Slaby wrote:
 On 09/02/2010 03:02 AM, David Rientjes wrote:
  --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -334,6 +334,57
  @@ static inline void *kzalloc_node(size_t size, gfp_t flags, int node)
  return kmalloc_node(size, flags | __GFP_ZERO, node); }
   
  +/** + * kmalloc_nofail - infinitely loop until kmalloc() succeeds.  +
  * @size: how many bytes of memory are required.  + * @flags: the type
  of memory to allocate (see kmalloc).  + * + * NOTE: no new callers of
  this function should be implemented!  + * All memory allocations should
  be failable whenever possible.  + */ +static inline void
  *kmalloc_nofail(size_t size, gfp_t flags) +{ +  void *ret; + +  for
  (;;) { +ret = kmalloc(size, flags); +   if (ret) +
  return ret; +   WARN_ON_ONCE(get_order(size) 
  PAGE_ALLOC_COSTLY_ORDER);
 
 This doesn't work as you expect. kmalloc will warn every time it fails.
 __GFP_NOFAIL used to disable the warning. Actually what's wrong with
 __GFP_NOFAIL? I cannot find a reason in the changelogs why the patches
 are needed.
  David should probably add the reasoning to the changelogs so that he
doesn't have to explain again and again ;). But if I understood it
correctly, the concern is that the looping checks slightly impact fast path
of the callers which do not need it. Generally, also looping for a long
time inside allocator isn't a nice thing but some callers aren't able to do
better for now to the patch is imperfect in this sence...

Honza
-- 
Jan Kara j...@suse.cz
SUSE Labs, CR
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch v2 1/5] mm: add nofail variants of kmalloc kcalloc and kzalloc

2010-09-02 Thread Neil Brown 
On Thu, 2 Sep 2010 16:51:41 +0200
Jan Kara j...@suse.cz wrote:

 On Thu 02-09-10 09:59:13, Jiri Slaby wrote:
  On 09/02/2010 03:02 AM, David Rientjes wrote:
   --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -334,6 +334,57
   @@ static inline void *kzalloc_node(size_t size, gfp_t flags, int node)
   return kmalloc_node(size, flags | __GFP_ZERO, node); }

   +/** + * kmalloc_nofail - infinitely loop until kmalloc() succeeds.  +
   * @size: how many bytes of memory are required.  + * @flags: the type
   of memory to allocate (see kmalloc).  + * + * NOTE: no new callers of
   this function should be implemented!  + * All memory allocations should
   be failable whenever possible.  + */ +static inline void
   *kmalloc_nofail(size_t size, gfp_t flags) +{ +void *ret; + +  for
   (;;) { +  ret = kmalloc(size, flags); +   if (ret) +
   return ret; + WARN_ON_ONCE(get_order(size) 
   PAGE_ALLOC_COSTLY_ORDER);
  
  This doesn't work as you expect. kmalloc will warn every time it fails.
  __GFP_NOFAIL used to disable the warning. Actually what's wrong with
  __GFP_NOFAIL? I cannot find a reason in the changelogs why the patches
  are needed.
   David should probably add the reasoning to the changelogs so that he
 doesn't have to explain again and again ;). But if I understood it
 correctly, the concern is that the looping checks slightly impact fast path
 of the callers which do not need it. Generally, also looping for a long
 time inside allocator isn't a nice thing but some callers aren't able to do
 better for now to the patch is imperfect in this sence...


I'm actually a bit confused about this too.
I thought David said he was removing a branch on the *slow* path - which make
sense as you wouldn't even test NOFAIL until you had a failure.
Why are branches on the slow-path an issue??
This is an important question to me because I still hope to see the
swap-over-nfs patches merged eventually and they add a branch on the slow
path (if I remember correctly).

NeilBrown
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch v2 1/5] mm: add nofail variants of kmalloc kcalloc and kzalloc

2010-09-01 Thread David Rientjes 
Add kmalloc_nofail(), kcalloc_nofail(), and kzalloc_nofail().  These
functions are equivalent to kmalloc(), kcalloc(), and kzalloc(),
respectively, except that they will never return NULL and instead loop
forever trying to allocate memory.

If the first allocation attempt fails because the page allocator doesn't 
implicitly loop, a warning will be emitted, including a call trace.
Subsequent failures will suppress this warning.

These were added as helper functions for documentation and auditability.
No future callers should be added.

Signed-off-by: David Rientjes rient...@google.com
---
 drivers/md/dm-region-hash.c |2 +-
 fs/btrfs/inode.c|2 +-
 fs/gfs2/log.c   |2 +-
 fs/gfs2/rgrp.c  |   18 +--
 fs/jbd/transaction.c|   11 ++--
 fs/reiserfs/journal.c   |3 +-
 include/linux/slab.h|   51 +++
 7 files changed, 64 insertions(+), 25 deletions(-)

diff --git a/drivers/md/dm-region-hash.c b/drivers/md/dm-region-hash.c
--- a/drivers/md/dm-region-hash.c
+++ b/drivers/md/dm-region-hash.c
@@ -290,7 +290,7 @@ static struct dm_region *__rh_alloc(struct dm_region_hash 
*rh, region_t region)
 
nreg = mempool_alloc(rh-region_pool, GFP_ATOMIC);
if (unlikely(!nreg))
-   nreg = kmalloc(sizeof(*nreg), GFP_NOIO | __GFP_NOFAIL);
+   nreg = kmalloc_nofail(sizeof(*nreg), GFP_NOIO);
 
nreg-state = rh-log-type-in_sync(rh-log, region, 1) ?
  DM_RH_CLEAN : DM_RH_NOSYNC;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1967,7 +1967,7 @@ void btrfs_add_delayed_iput(struct inode *inode)
if (atomic_add_unless(inode-i_count, -1, 1))
return;
 
-   delayed = kmalloc(sizeof(*delayed), GFP_NOFS | __GFP_NOFAIL);
+   delayed = kmalloc_nofail(sizeof(*delayed), GFP_NOFS);
delayed-inode = inode;
 
spin_lock(fs_info-delayed_iput_lock);
diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -709,7 +709,7 @@ void gfs2_log_flush(struct gfs2_sbd *sdp, struct gfs2_glock 
*gl)
}
trace_gfs2_log_flush(sdp, 1);
 
-   ai = kzalloc(sizeof(struct gfs2_ail), GFP_NOFS | __GFP_NOFAIL);
+   ai = kzalloc_nofail(sizeof(struct gfs2_ail), GFP_NOFS);
INIT_LIST_HEAD(ai-ai_ail1_list);
INIT_LIST_HEAD(ai-ai_ail2_list);
 
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1440,8 +1440,8 @@ static struct gfs2_rgrpd *rgblk_free(struct gfs2_sbd 
*sdp, u64 bstart,
rgrp_blk++;
 
if (!bi-bi_clone) {
-   bi-bi_clone = kmalloc(bi-bi_bh-b_size,
-  GFP_NOFS | __GFP_NOFAIL);
+   bi-bi_clone = kmalloc_nofail(bi-bi_bh-b_size,
+  GFP_NOFS);
memcpy(bi-bi_clone + bi-bi_offset,
   bi-bi_bh-b_data + bi-bi_offset,
   bi-bi_len);
@@ -1759,9 +1759,6 @@ fail:
  * @block: the block
  *
  * Figure out what RG a block belongs to and add that RG to the list
- *
- * FIXME: Don't use NOFAIL
- *
  */
 
 void gfs2_rlist_add(struct gfs2_sbd *sdp, struct gfs2_rgrp_list *rlist,
@@ -1789,8 +1786,8 @@ void gfs2_rlist_add(struct gfs2_sbd *sdp, struct 
gfs2_rgrp_list *rlist,
if (rlist-rl_rgrps == rlist-rl_space) {
new_space = rlist-rl_space + 10;
 
-   tmp = kcalloc(new_space, sizeof(struct gfs2_rgrpd *),
- GFP_NOFS | __GFP_NOFAIL);
+   tmp = kcalloc_nofail(new_space, sizeof(struct gfs2_rgrpd *),
+ GFP_NOFS);
 
if (rlist-rl_rgd) {
memcpy(tmp, rlist-rl_rgd,
@@ -1811,17 +1808,14 @@ void gfs2_rlist_add(struct gfs2_sbd *sdp, struct 
gfs2_rgrp_list *rlist,
  * @rlist: the list of resource groups
  * @state: the lock state to acquire the RG lock in
  * @flags: the modifier flags for the holder structures
- *
- * FIXME: Don't use NOFAIL
- *
  */
 
 void gfs2_rlist_alloc(struct gfs2_rgrp_list *rlist, unsigned int state)
 {
unsigned int x;
 
-   rlist-rl_ghs = kcalloc(rlist-rl_rgrps, sizeof(struct gfs2_holder),
-   GFP_NOFS | __GFP_NOFAIL);
+   rlist-rl_ghs = kcalloc_nofail(rlist-rl_rgrps,
+   sizeof(struct gfs2_holder), GFP_NOFS);
for (x = 0; x  rlist-rl_rgrps; x++)
gfs2_holder_init(rlist-rl_rgd[x]-rd_gl,
state, 0,
diff --git a/fs/jbd/transaction.c b/fs/jbd/transaction.c
--- a/fs/jbd/transaction.c
+++ b/fs/jbd/transaction.c
@@ -98,14 +98,9 @@ static int start_this_handle(journal_t *journal, handle_t 
*handle)
}
 
 alloc_transaction:
-   if (!journal-j_running_transaction) {
-   new_transaction =