FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Cluster Development

 
 
LinkBack Thread Tools
 
Old 08-24-2010, 12:15 PM
Jan Kara
 
Default mm: add nofail variants of kmalloc kcalloc and kzalloc

On Tue 24-08-10 03:50:19, 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, 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.
This looks like a cleaner approach than the previous one. You can add
Acked-by: Jan Kara <jack@suse.cz>
for the JBD part.

Honza
> Signed-off-by: David Rientjes <rientjes@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 | 55 +++++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 68 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 = kzalloc(sizeof(*new_transaction),
> - GFP_NOFS|__GFP_NOFAIL);
> - if (!new_transaction) {
> - ret = -ENOMEM;
> - goto out;
> - }
> - }
> + if (!journal->j_running_transaction)
> + new_transaction = kzalloc_nofail(sizeof(*new_transaction),
> + GFP_NOFS);
>
> jbd_debug(3, "New handle %p going live.
", handle);
>
> diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
> --- a/fs/reiserfs/journal.c
> +++ b/fs/reiserfs/journal.c
> @@ -2593,8 +2593,7 @@ static int journal_read(struct super_block *sb)
> static struct reiserfs_journal_list *alloc_journal_list(struct super_block *s)
> {
> struct reiserfs_journal_list *jl;
> - jl = kzalloc(sizeof(struct reiserfs_journal_list),
> - GFP_NOFS | __GFP_NOFAIL);
> + jl = kzalloc_nofail(sizeof(struct reiserfs_journal_list), GFP_NOFS);
> INIT_LIST_HEAD(&jl->j_list);
> INIT_LIST_HEAD(&jl->j_working_list);
> INIT_LIST_HEAD(&jl->j_tail_bh_list);
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -334,6 +334,61 @@ 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_ONCE(1, "Out of memory, no fallback implemented "
> + "(size=%lu flags=0x%x)
",
> + size, flags);
> + }
> +}
> +
> +/**
> + * kcalloc_nofail - infinitely loop until kcalloc() succeeds.
> + * @n: number of elements.
> + * @size: element size.
> + * @flags: the type of memory to allocate (see kcalloc).
> + *
> + * NOTE: no new callers of this function should be implemented!
> + * All memory allocations should be failable whenever possible.
> + */
> +static inline void *kcalloc_nofail(size_t n, size_t size, gfp_t flags)
> +{
> + void *ret;
> +
> + for (; {
> + ret = kcalloc(n, size, flags);
> + if (ret)
> + return ret;
> + WARN_ONCE(1, "Out of memory, no fallback implemented "
> + "(n=%lu size=%lu flags=0x%x)
",
> + n, size, flags);
> + }
> +}
> +
> +/**
> + * kzalloc_nofail - infinitely loop until kzalloc() succeeds.
> + * @size: how many bytes of memory are required.
> + * @flags: the type of memory to allocate (see kzalloc).
> + */
> +static inline void *kzalloc_nofail(size_t size, gfp_t flags)
> +{
> + return kmalloc_nofail(size, flags | __GFP_ZERO);
> +}
> +
> void __init kmem_cache_init_late(void);
>
> #endif /* _LINUX_SLAB_H */
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
 
Old 09-02-2010, 02:51 PM
Jan Kara
 
Default mm: add nofail variants of kmalloc kcalloc and kzalloc

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 <jack@suse.cz>
SUSE Labs, CR
 
Old 09-02-2010, 09:15 PM
Neil Brown
 
Default mm: add nofail variants of kmalloc kcalloc and kzalloc

On Thu, 2 Sep 2010 16:51:41 +0200
Jan Kara <jack@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
 

Thread Tools




All times are GMT. The time now is 05:26 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org