On Wed, 2008-02-13 at 01:36 -0800, David Rientjes wrote:
I was hoping David wouldn't cave on this point. ;-) I do agree with
David that if we ever end up with nr_modes + nr_mode_flags > 16, the API
will become too complex for myself, at least, to comprehend. I don't
feel too strongly either way, and I'll explain more below, but first:
I do agree with Paul that there is a danger of missing one of these in
converting existing code. In fact, I missed one in my ref counting
rework patch that I will ultimately rebase atop David's final version
[I'm already dependent on Mel Gorman's patch series]. To catch this, I
decided to rename the "policy" member to "mode". This also aligns the
struct better with the numa_memory_policy doc [I think of the "policy"
as the tuple: mode + optional nodemask]. For future code, I think we
could add comments to the struct definition warning that one should use
the wrappers to access the mode or mode flags.
However, let's step back and look at the usage of the mempolicy struct.
In earlier mail, David mentioned that it is passed around outside of
mempolicy.c. While this is true, the only place that I see where code
outside of mempolicy.c deals with the policy/mode and nodemask
separately--as opposed to using an opaque mempolicy struct--is in the
shmem mount option parsing. Everywhere else, as far as I can see, just
uses the struct mempolicy. Even slab/slub call into slab_node() in
mempolicy.c to extract the target node for the policy.
So, if David does accept Paul's plea to separate the mode and the mode
flags in the structure, I would suggest that we do this in mpol_new().
That is, in {sys|do}_{set_mempolicy|mbind}(), maintain the API format
with the mode flags ORed into the mode. mpol_new() can pull them apart
into different struct mempolicy members. The rest of mempolicy.c can
use them directly from the struct without the helpers. get_mempolicy()
will need to pack the mode and flags back together--perhaps with an
inverse helper function.
As for the shmem mount option parsing: For now, I'd suggest that we
keep the mode flags OR'd into the "policy" member of the shmem_sb_info
struct -- i.e., the "API format"--to preserve the mpol_new() interface.
At some point [no urgency, I think], I'd like to replace the
shmem_sb_info policy and nodemask members with a struct mempolicy or
pointer thereto, and instead of using mpol_new() to create the mempolicy
for new tmpfs inodes, use mpol_copy() [which should probably be renamed
to mpol_dup(), 'cause that's what it does]. I'd also like to move
shmem_parse_mpol() to mempolicy.c as a more general memory policy
parser. shmem_show_mpol() can probably be reworked to use mpol_to_str()
used by show_numa_map(). Again, no urgency. I'd rather see David's
and Mel's and my pending mempolicy patches settle down and get in first.
[Too many interdependent out of tree mempolicy patches, already :-(]
Note: I agree that the 'STATIC' flag overrides the argument checking
you refer to above. That's new behavior that the application has
indicated that it wants to use. However, methinks that MPOL_DEFAULT|
MPOL_F_STATIC_NODES doesn't make much sense.
Regards,
Lee
--
