From: Vlastimil Babka <vbabka@suse.cz>
To: Mel Gorman <mgorman@techsingularity.net>,
Johannes Weiner <hannes@cmpxchg.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Rik van Riel <riel@redhat.com>,
David Rientjes <rientjes@google.com>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
Michal Hocko <mhocko@kernel.org>, Linux-MM <linux-mm@kvack.org>,
LKML <linux-kernel@vger.kernel.org>,
Oleg Drokin <oleg.drokin@intel.com>,
Andreas Dilger <andreas.dilger@intel.com>
Subject: Re: [PATCH 05/10] mm, page_alloc: Distinguish between being unable to sleep, unwilling to sleep and avoiding waking kswapd
Date: Wed, 30 Sep 2015 14:26:24 +0200 [thread overview]
Message-ID: <560BD4F0.3080402@suse.cz> (raw)
In-Reply-To: <20150929133547.GI3068@techsingularity.net>
[+CC lustre maintainers]
On 09/29/2015 03:35 PM, Mel Gorman wrote:
>>> Ok, I'll add a TODO to create a patch that removes GFP_IOFS entirely. It
>>> can be tacked on to the end of the series.
>>
>> Okay, that makes sense to me. Thanks!
>>
>
> This?
Thanks for adding this, I think I also pointed this GFP_IOFS oddness in
earlier versions.
> ---8<---
> mm: page_alloc: Remove GFP_IOFS
>
> GFP_IOFS was intended to be shorthand for clearing two flags, not a
> set of allocation flags. There is only one user of this flag combination
> now and there appears to be no reason why Lustre had to be protected
Looks like a mistake to me. __GFP_IO | __GFP_FS have no effect without
(former) __GFP_WAIT, so I doubt __GFP_WAIT was omitted on purpose, while
leaving the other two. The naming of GFP_IOFS suggested it was to be
used in allocations, leading to the mistake.
But I see you also converted several instances of GFP_NOFS to
GFP_KERNEL. Is that correct? This is a filesystem driver after all...
> from reclaim stalls. As none of the sites appear to be atomic, this
> patch simply deletes GFP_IOFS and converts Lustre to using GFP_KERNEL.
>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> ---
> drivers/staging/lustre/lnet/lnet/router.c | 2 +-
> drivers/staging/lustre/lnet/selftest/conrpc.c | 2 +-
> drivers/staging/lustre/lnet/selftest/rpc.c | 2 +-
> drivers/staging/lustre/lustre/libcfs/module.c | 2 +-
> drivers/staging/lustre/lustre/libcfs/tracefile.c | 2 +-
> drivers/staging/lustre/lustre/llite/remote_perm.c | 2 +-
> drivers/staging/lustre/lustre/mgc/mgc_request.c | 10 +++++-----
> drivers/staging/lustre/lustre/obdecho/echo_client.c | 2 +-
> drivers/staging/lustre/lustre/osc/osc_cache.c | 2 +-
> include/linux/gfp.h | 1 -
> 10 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/lustre/lnet/lnet/router.c b/drivers/staging/lustre/lnet/lnet/router.c
> index 4fbae5ef44a9..dad9816dfee7 100644
> --- a/drivers/staging/lustre/lnet/lnet/router.c
> +++ b/drivers/staging/lustre/lnet/lnet/router.c
> @@ -1246,7 +1246,7 @@ lnet_new_rtrbuf(lnet_rtrbufpool_t *rbp, int cpt)
> for (i = 0; i < npages; i++) {
> page = alloc_pages_node(
> cfs_cpt_spread_node(lnet_cpt_table(), cpt),
> - __GFP_ZERO | GFP_IOFS, 0);
> + GFP_KERNEL | __GFP_ZERO, 0);
> if (page == NULL) {
> while (--i >= 0)
> __free_page(rb->rb_kiov[i].kiov_page);
> diff --git a/drivers/staging/lustre/lnet/selftest/conrpc.c b/drivers/staging/lustre/lnet/selftest/conrpc.c
> index a1a4e08f7391..3fc37de8d304 100644
> --- a/drivers/staging/lustre/lnet/selftest/conrpc.c
> +++ b/drivers/staging/lustre/lnet/selftest/conrpc.c
> @@ -861,7 +861,7 @@ lstcon_testrpc_prep(lstcon_node_t *nd, int transop, unsigned feats,
> bulk->bk_iovs[i].kiov_offset = 0;
> bulk->bk_iovs[i].kiov_len = len;
> bulk->bk_iovs[i].kiov_page =
> - alloc_page(GFP_IOFS);
> + alloc_page(GFP_KERNEL);
>
> if (bulk->bk_iovs[i].kiov_page == NULL) {
> lstcon_rpc_put(*crpc);
> diff --git a/drivers/staging/lustre/lnet/selftest/rpc.c b/drivers/staging/lustre/lnet/selftest/rpc.c
> index 6ae133138b17..aa0f88fbb221 100644
> --- a/drivers/staging/lustre/lnet/selftest/rpc.c
> +++ b/drivers/staging/lustre/lnet/selftest/rpc.c
> @@ -146,7 +146,7 @@ srpc_alloc_bulk(int cpt, unsigned bulk_npg, unsigned bulk_len, int sink)
> int nob;
>
> pg = alloc_pages_node(cfs_cpt_spread_node(lnet_cpt_table(), cpt),
> - GFP_IOFS, 0);
> + GFP_KERNEL, 0);
> if (pg == NULL) {
> CERROR("Can't allocate page %d of %d\n", i, bulk_npg);
> srpc_free_bulk(bk);
> diff --git a/drivers/staging/lustre/lustre/libcfs/module.c b/drivers/staging/lustre/lustre/libcfs/module.c
> index 806f9747a3a2..303143f28c06 100644
> --- a/drivers/staging/lustre/lustre/libcfs/module.c
> +++ b/drivers/staging/lustre/lustre/libcfs/module.c
> @@ -321,7 +321,7 @@ static int libcfs_ioctl(struct cfs_psdev_file *pfile, unsigned long cmd, void *a
> struct libcfs_ioctl_data *data;
> int err = 0;
>
> - LIBCFS_ALLOC_GFP(buf, 1024, GFP_IOFS);
> + LIBCFS_ALLOC_GFP(buf, 1024, GFP_KERNEL);
> if (buf == NULL)
> return -ENOMEM;
>
> diff --git a/drivers/staging/lustre/lustre/libcfs/tracefile.c b/drivers/staging/lustre/lustre/libcfs/tracefile.c
> index effa2af58c13..a7d72f69c4eb 100644
> --- a/drivers/staging/lustre/lustre/libcfs/tracefile.c
> +++ b/drivers/staging/lustre/lustre/libcfs/tracefile.c
> @@ -810,7 +810,7 @@ int cfs_trace_allocate_string_buffer(char **str, int nob)
> if (nob > 2 * PAGE_CACHE_SIZE) /* string must be "sensible" */
> return -EINVAL;
>
> - *str = kmalloc(nob, GFP_IOFS | __GFP_ZERO);
> + *str = kmalloc(nob, GFP_KERNEL | __GFP_ZERO);
This could use kzalloc.
> if (*str == NULL)
> return -ENOMEM;
>
> diff --git a/drivers/staging/lustre/lustre/llite/remote_perm.c b/drivers/staging/lustre/lustre/llite/remote_perm.c
> index 39022ea88b5f..b27f016c3dd4 100644
> --- a/drivers/staging/lustre/lustre/llite/remote_perm.c
> +++ b/drivers/staging/lustre/lustre/llite/remote_perm.c
> @@ -84,7 +84,7 @@ static struct hlist_head *alloc_rmtperm_hash(void)
>
> OBD_SLAB_ALLOC_GFP(hash, ll_rmtperm_hash_cachep,
> REMOTE_PERM_HASHSIZE * sizeof(*hash),
> - GFP_IOFS);
> + GFP_KERNEL);
> if (!hash)
> return NULL;
>
> diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
> index 019ee2f256aa..79551319d754 100644
> --- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
> +++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
> @@ -198,7 +198,7 @@ struct config_llog_data *do_config_log_add(struct obd_device *obd,
> CDEBUG(D_MGC, "do adding config log %s:%p\n", logname,
> cfg ? cfg->cfg_instance : NULL);
>
> - cld = kzalloc(sizeof(*cld) + strlen(logname) + 1, GFP_NOFS);
> + cld = kzalloc(sizeof(*cld) + strlen(logname) + 1, GFP_KERNEL);
> if (!cld)
> return ERR_PTR(-ENOMEM);
>
> @@ -1127,7 +1127,7 @@ static int mgc_apply_recover_logs(struct obd_device *mgc,
> LASSERT(cfg->cfg_instance != NULL);
> LASSERT(cfg->cfg_sb == cfg->cfg_instance);
>
> - inst = kzalloc(PAGE_CACHE_SIZE, GFP_NOFS);
> + inst = kzalloc(PAGE_CACHE_SIZE, GFP_KERNEL);
> if (!inst)
> return -ENOMEM;
>
> @@ -1334,14 +1334,14 @@ static int mgc_process_recover_log(struct obd_device *obd,
> if (cfg->cfg_last_idx == 0) /* the first time */
> nrpages = CONFIG_READ_NRPAGES_INIT;
>
> - pages = kcalloc(nrpages, sizeof(*pages), GFP_NOFS);
> + pages = kcalloc(nrpages, sizeof(*pages), GFP_KERNEL);
> if (pages == NULL) {
> rc = -ENOMEM;
> goto out;
> }
>
> for (i = 0; i < nrpages; i++) {
> - pages[i] = alloc_page(GFP_IOFS);
> + pages[i] = alloc_page(GFP_KERNEL);
> if (pages[i] == NULL) {
> rc = -ENOMEM;
> goto out;
> @@ -1492,7 +1492,7 @@ static int mgc_process_cfg_log(struct obd_device *mgc,
> if (cld->cld_cfg.cfg_sb)
> lsi = s2lsi(cld->cld_cfg.cfg_sb);
>
> - env = kzalloc(sizeof(*env), GFP_NOFS);
> + env = kzalloc(sizeof(*env), GFP_KERNEL);
> if (!env)
> return -ENOMEM;
>
> diff --git a/drivers/staging/lustre/lustre/obdecho/echo_client.c b/drivers/staging/lustre/lustre/obdecho/echo_client.c
> index 27bd170c3a28..7c8443644300 100644
> --- a/drivers/staging/lustre/lustre/obdecho/echo_client.c
> +++ b/drivers/staging/lustre/lustre/obdecho/echo_client.c
> @@ -1561,7 +1561,7 @@ static int echo_client_kbrw(struct echo_device *ed, int rw, struct obdo *oa,
> (oa->o_valid & OBD_MD_FLFLAGS) != 0 &&
> (oa->o_flags & OBD_FL_DEBUG_CHECK) != 0);
>
> - gfp_mask = ((ostid_id(&oa->o_oi) & 2) == 0) ? GFP_IOFS : GFP_HIGHUSER;
> + gfp_mask = ((ostid_id(&oa->o_oi) & 2) == 0) ? GFP_KERNEL : GFP_HIGHUSER;
>
> LASSERT(rw == OBD_BRW_WRITE || rw == OBD_BRW_READ);
> LASSERT(lsm != NULL);
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index c72035e048aa..6fa6bc6874ab 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -346,7 +346,7 @@ static struct osc_extent *osc_extent_alloc(struct osc_object *obj)
> {
> struct osc_extent *ext;
>
> - OBD_SLAB_ALLOC_PTR_GFP(ext, osc_extent_kmem, GFP_IOFS);
> + OBD_SLAB_ALLOC_PTR_GFP(ext, osc_extent_kmem, GFP_KERNEL);
> if (ext == NULL)
> return NULL;
>
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 60b2db94d49d..369227202ac2 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -134,7 +134,6 @@ struct vm_area_struct;
> #define GFP_USER (__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL)
> #define GFP_HIGHUSER (GFP_USER | __GFP_HIGHMEM)
> #define GFP_HIGHUSER_MOVABLE (GFP_HIGHUSER | __GFP_MOVABLE)
> -#define GFP_IOFS (__GFP_IO | __GFP_FS | __GFP_KSWAPD_RECLAIM)
> #define GFP_TRANSHUGE ((GFP_HIGHUSER_MOVABLE | __GFP_COMP | \
> __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_NOWARN) & \
> ~__GFP_KSWAPD_RECLAIM)
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2015-09-30 12:26 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-21 10:52 [PATCH 00/10] Remove zonelist cache and high-order watermark checking v4 Mel Gorman
2015-09-21 10:52 ` [PATCH 01/10] mm, page_alloc: Remove unnecessary parameter from zone_watermark_ok_safe Mel Gorman
2015-09-24 20:01 ` Johannes Weiner
2015-09-21 10:52 ` [PATCH 02/10] mm, page_alloc: Remove unnecessary recalculations for dirty zone balancing Mel Gorman
2015-09-24 20:05 ` Johannes Weiner
2015-09-21 10:52 ` [PATCH 03/10] mm, page_alloc: Remove unnecessary taking of a seqlock when cpusets are disabled Mel Gorman
2015-09-24 20:06 ` Johannes Weiner
2015-09-30 22:22 ` David Rientjes
2015-10-01 7:35 ` Vlastimil Babka
2015-09-21 10:52 ` [PATCH 04/10] mm, page_alloc: Use masks and shifts when converting GFP flags to migrate types Mel Gorman
2015-09-24 20:34 ` Johannes Weiner
2015-09-25 12:50 ` Mel Gorman
2015-09-25 13:56 ` Johannes Weiner
2015-09-21 10:52 ` [PATCH 05/10] mm, page_alloc: Distinguish between being unable to sleep, unwilling to sleep and avoiding waking kswapd Mel Gorman
2015-09-24 13:51 ` Michal Hocko
2015-09-24 20:55 ` Johannes Weiner
2015-09-25 12:51 ` Mel Gorman
2015-09-25 19:01 ` Johannes Weiner
2015-09-29 13:35 ` Mel Gorman
2015-09-30 12:26 ` Vlastimil Babka [this message]
2015-09-30 13:17 ` Mel Gorman
2015-10-01 3:04 ` Drokin, Oleg
2015-10-02 12:30 ` Mel Gorman
2015-09-21 10:52 ` [PATCH 06/10] mm, page_alloc: Rename __GFP_WAIT to __GFP_RECLAIM Mel Gorman
2015-09-25 19:03 ` Johannes Weiner
2015-09-28 23:55 ` Andrew Morton
2015-09-29 13:37 ` Mel Gorman
2015-10-01 8:39 ` Vlastimil Babka
2015-10-02 13:03 ` [PATCH] mm: page_alloc: Hide some GFP internals and document the bits and flag combinations -fix Mel Gorman
2015-10-01 14:06 ` [PATCH 06/10] mm, page_alloc: Rename __GFP_WAIT to __GFP_RECLAIM Michal Hocko
2015-09-30 22:25 ` David Rientjes
2015-09-21 10:52 ` [PATCH 07/10] mm, page_alloc: Delete the zonelist_cache Mel Gorman
2015-09-25 19:09 ` Johannes Weiner
2015-09-21 10:52 ` [PATCH 08/10] mm, page_alloc: Remove MIGRATE_RESERVE Mel Gorman
2015-09-21 10:52 ` [PATCH 09/10] mm, page_alloc: Reserve pageblocks for high-order atomic allocations on demand Mel Gorman
2015-09-24 13:50 ` Michal Hocko
2015-09-25 19:22 ` Johannes Weiner
2015-09-29 21:01 ` Andrew Morton
2015-09-30 8:27 ` Mel Gorman
2015-09-30 14:02 ` Vlastimil Babka
2015-09-21 12:03 ` [PATCH 10/10] mm, page_alloc: Only enforce watermarks for order-0 allocations Mel Gorman
2015-09-25 19:32 ` Johannes Weiner
2015-09-29 21:05 ` Andrew Morton
2015-09-30 8:46 ` Mel Gorman
2015-09-30 14:17 ` Vlastimil Babka
2015-09-30 15:12 ` Mel Gorman
2015-09-30 20:37 ` Andrew Morton
2015-09-30 14:11 ` Vlastimil Babka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=560BD4F0.3080402@suse.cz \
--to=vbabka@suse.cz \
--cc=akpm@linux-foundation.org \
--cc=andreas.dilger@intel.com \
--cc=hannes@cmpxchg.org \
--cc=iamjoonsoo.kim@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@techsingularity.net \
--cc=mhocko@kernel.org \
--cc=oleg.drokin@intel.com \
--cc=riel@redhat.com \
--cc=rientjes@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox