From: Lee Schermerhorn <Lee.Schermerhorn@hp.com>
To: David Rientjes <rientjes@google.com>
Cc: Mel Gorman <mel@csn.ul.ie>,
linux-mm@kvack.org, linux-numa@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
Nishanth Aravamudan <nacc@us.ibm.com>,
Adam Litke <agl@us.ibm.com>, Andy Whitcroft <apw@canonical.com>,
eric.whitney@hp.com
Subject: Re: [PATCH 4/5] hugetlb: add per node hstate attributes
Date: Fri, 28 Aug 2009 08:56:52 -0400 [thread overview]
Message-ID: <1251464212.9989.52.camel@useless.americas.hpqcorp.net> (raw)
In-Reply-To: <alpine.DEB.2.00.0908271228200.14815@chino.kir.corp.google.com>
On Thu, 2009-08-27 at 12:35 -0700, David Rientjes wrote:
> On Wed, 26 Aug 2009, Lee Schermerhorn wrote:
>
> > > I think it would probably be better to use the generic NODEMASK_ALLOC()
> > > interface by requiring it to pass the entire type (including "struct") as
> > > part of the first parameter. Then it automatically takes care of
> > > dynamically allocating large nodemasks vs. allocating them on the stack.
> > >
> > > Would it work by redefining NODEMASK_ALLOC() in the NODES_SHIFT > 8 case
> > > to be this:
> > >
> > > #define NODEMASK_ALLOC(x, m) x *m = kmalloc(sizeof(*m), GFP_KERNEL);
> > >
> > > and converting NODEMASK_SCRATCH(x) to NODEMASK_ALLOC(struct
> > > nodemask_scratch, x), and then doing this in your code:
> > >
> > > NODEMASK_ALLOC(nodemask_t, nodes_allowed);
> > > if (nodes_allowed)
> > > *nodes_allowed = nodemask_of_node(node);
> > >
> > > The NODEMASK_{ALLOC,SCRATCH}() interface is in its infancy so it can
> > > probably be made more general to handle cases like this.
> >
> > I just don't know what that would accomplish. Heck, I'm not all that
> > happy with the alloc_nodemask_from_node() because it's allocating both a
> > hidden nodemask_t and a pointer thereto on the stack just to return a
> > pointer to a kmalloc()ed nodemask_t--which is what I want/need here.
> >
> > One issue I have with NODEMASK_ALLOC() [and nodemask_of_node(), et al]
> > is that it declares the pointer variable as well as initializing it,
> > perhaps with kmalloc(), ... Indeed, it's purpose is to replace on
> > stack nodemask declarations.
> >
>
> Right, which is why I suggest we only have one such interface to
> dynamically allocate nodemasks when NODES_SHIFT > 8. That's what defines
> NODEMASK_ALLOC() as being special: it's taking NODES_SHIFT into
> consideration just like CPUMASK_ALLOC() would take NR_CPUS into
> consideration. Your use case is the intended purpose of NODEMASK_ALLOC()
> and I see no reason why your code can't use the same interface with some
> modification and it's in the best interest of a maintainability to not
> duplicate specialized cases where pre-existing interfaces can be used (or
> improved, in this case).
>
> > So, to use it at the start of, e.g., set_max_huge_pages() where I can
> > safely use it throughout the function, I'll end up allocating the
> > nodes_allowed mask on every call, whether or not a node is specified or
> > there is a non-default mempolicy. If it turns out that no node was
> > specified and we have default policy, we need to free the mask and NULL
> > out nodes_allowed up front so that we get default behavior. That seems
> > uglier to me that only allocating the nodemask when we know we need one.
> >
>
> Not with my suggested code of disabling local irqs, getting a reference to
> the mempolicy so it can't be freed, reenabling, and then only using
> NODEMASK_ALLOC() in the switch statement on mpol->mode for MPOL_PREFERRED.
>
> > I'm not opposed to using a generic function/macro where one exists that
> > suits my purposes. I just don't see one. I tried to create
> > one--alloc_nodemask_from_node(), and to keep Mel happy, I tried to reuse
> > nodemask_from_node() to initialize it. I'm really not happy with the
> > results--because of those extra, hidden stack variables. I could
> > eliminate those by creating a out of line function, but there's no good
> > place to put a generic nodemask function--no nodemask.c.
> >
>
> Using NODEMASK_ALLOC(nodes_allowed) wouldn't really be a hidden stack
> variable, would it? I think most developers would assume that it is
> some automatic variable called `nodes_allowed' since it's later referenced
> (and only needs to be in the case of MPOL_PREFERRED if my mpol_get()
> solution with disabled local irqs is used).
David:
I'm going to repost my series with the version of
alloc_nodemask_of_node() that I sent our yesterday. My entire
implementation is based on nodes_allowed, in set_max_huge_pages() being
a pointer to a nodemask. nodes_allowed must be NULL for default
behavior [NO_NODEID_SPECIFIED && default mempolicy]. It only gets
allocated when nid >0 or task has non-default memory policy. This seems
to work fairly well for both the mempolicy based constraint and the per
node attributes. Please take a look at this series. If you want to
propose a patch to rework the nodes_allowed allocation, have at it. I'm
satisfied with the current implementation.
Now, we have a couple of options: Mel said he's willing to proceed with
the mempolicy based constraint and leave the per node attributes to a
follow up submit. If you want to take over the per node attributes
feature and rework it, I can extract it from the series, including the
doc update and turn it over to you. Or, we can try to submit the
current implementation and follow up with patches to rework the generic
nodemask support as you propose.
Let me know how you want to proceed.
Lee
--
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:[~2009-08-28 12:56 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-08-24 19:24 [PATCH 0/5] hugetlb: numa control of persistent huge pages alloc/free Lee Schermerhorn
2009-08-24 19:25 ` [PATCH 1/5] hugetlb: rework hstate_next_node_* functions Lee Schermerhorn
2009-08-25 8:10 ` David Rientjes
2009-08-24 19:26 ` [PATCH 2/5] hugetlb: add nodemask arg to huge page alloc, free and surplus adjust fcns Lee Schermerhorn
2009-08-25 8:16 ` David Rientjes
2009-08-25 20:49 ` Lee Schermerhorn
2009-08-25 21:59 ` David Rientjes
2009-08-26 9:58 ` Mel Gorman
2009-08-24 19:27 ` [PATCH 3/5] hugetlb: derive huge pages nodes allowed from task mempolicy Lee Schermerhorn
2009-08-25 8:47 ` David Rientjes
2009-08-25 20:49 ` Lee Schermerhorn
2009-08-27 19:40 ` David Rientjes
2009-08-25 10:22 ` Mel Gorman
2009-08-24 19:29 ` [PATCH 4/5] hugetlb: add per node hstate attributes Lee Schermerhorn
2009-08-25 10:19 ` Mel Gorman
2009-08-25 20:49 ` Lee Schermerhorn
2009-08-26 10:11 ` Mel Gorman
2009-08-26 18:02 ` Lee Schermerhorn
2009-08-26 19:47 ` David Rientjes
2009-08-26 20:46 ` Lee Schermerhorn
2009-08-27 9:52 ` Mel Gorman
2009-08-27 19:35 ` David Rientjes
2009-08-28 12:56 ` Lee Schermerhorn [this message]
2009-08-26 18:04 ` Lee Schermerhorn
2009-08-27 10:23 ` Mel Gorman
2009-08-27 16:52 ` Lee Schermerhorn
2009-08-28 10:09 ` Mel Gorman
2009-08-25 13:35 ` Mel Gorman
2009-08-25 20:49 ` Lee Schermerhorn
2009-08-26 10:12 ` Mel Gorman
2009-08-24 19:30 ` [PATCH 5/5] hugetlb: update hugetlb documentation for mempolicy based management Lee Schermerhorn
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=1251464212.9989.52.camel@useless.americas.hpqcorp.net \
--to=lee.schermerhorn@hp.com \
--cc=agl@us.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=apw@canonical.com \
--cc=eric.whitney@hp.com \
--cc=linux-mm@kvack.org \
--cc=linux-numa@vger.kernel.org \
--cc=mel@csn.ul.ie \
--cc=nacc@us.ibm.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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).