lkml.org 
[lkml]   [2008]   [Feb]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 3/4] mempolicy: add MPOL_F_STATIC_NODES flag
    On Wed, 13 Feb 2008, Lee Schermerhorn wrote:

    > > > 2) Those 'mpol_mode()' wrappers on all those mempolicy->policy
    > > > evaluations look dangerous to me. It looks like a code bug
    > > > waiting to happen. Unless I miss my guess, if you forget one of
    > > > those wrappers (or someone making a future change misses one), no
    > > > one will notice until sometime at runtime, someone makes use of the
    > > > new fangled MPOL_F_STATIC_NODES mode in some new situation, and we
    > > > end up executing code that we didn't expect to execute just then.
    > > >
    > > > I urge you to reconsider, and keep it so that the 'policy' field of struct
    > > > mempolicy naturally evaluates to just the policy. There should be just one
    > > > pair of places, on entry to, and exit from, the kernel, where the code is
    > > > aware of the need to pack the mode and the policy into a single word.
    > > >
    > >
    > > Ok.
    >
    > 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:
    >

    Hmm, I don't remember "caving" on this point; Paul asked me to reconsider
    and I said that I would. I think it's also helpful to have more
    discussion on the matter by including other people's opinions.

    > 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.
    >

    I considered this when I implemented it the way I did, and that was part
    of the reason I was in favor of enum mempolicy_mode so much: functions
    that had a formal of type 'enum mempolicy_mode' were only the mode and
    formals of type 'int' or 'unsigned short' were the combination. I even
    stated that difference in Documentation/vm/numa_memory_policy.txt in the
    first patchset.

    > 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.
    >

    Yes, struct shmem_sb_info stores the 'policy' member as well so it would
    need to include a new 'flags' member as well. And the interface between
    the two, mpol_shared_policy_init() would need another formal for the
    flags.

    > 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.
    >

    Yeah, it gets tricky. I'm not too sure about only pulling the mode and
    flags apart at mpol_new() time because perhaps, in the future, there will
    be flag and mode combinations that are only applicable for set_mempolicy()
    and not for mbind(), for example. I can imagine that someday we may add a
    flag that applies only to a task mempolicy and not to a VMA mempolicy.

    > 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.

    I don't like this because its not consistent. It increases the confusion
    of what contains a mode and what contains the combination. I think the
    safest thing to do, if we separate the mode and flags out in struct
    mempolicy is to do it everywhere. All helper functions will need
    additional formals similar to what I did in the first patchset (that was
    actually unpopular) and struct shmem_sb_info need to be modified to
    include the additional member.

    In other words, I'm all in favor of storing the mode and flags however we
    want, but I think it should be done consistenty throughout the tree.
    While mpol_mode() wrappers may not be favorable all over the place (and I
    didn't miss any), it may be easier than the alternative.

    David


    \
     
     \ /
      Last update: 2008-02-13 19:51    [W:8.355 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site