Re: [PATCH v4 00/11] mempolicy2, mbind2, and weighted interleave

Gregory Price [off-list ref] writes:

On Tue, Dec 19, 2023 at 11:04:05AM +0800, Huang, Ying wrote:

quoted

Gregory Price [off-list ref] writes:

quoted

This patch set extends the mempolicy interface to enable new
mempolicies which may require extended data to operate.

MPOL_WEIGHTED_INTERLEAVE is included as an example extension.

Per my understanding, it's better to describe why we need this patchset
at the beginning.  Per my understanding, weighted interleave is used to
expand DRAM bandwidth for workloads with real high memory bandwidth
requirements.  Without it, DRAM bandwidth will be saturated, which leads
to poor performance.

Will add more details, thanks.

quoted

quoted

struct mempolicy_args {
    unsigned short mode;            /* policy mode */
    unsigned short mode_flags;      /* policy mode flags */
    int home_node;                  /* mbind: use MPOL_MF_HOME_NODE */
    nodemask_t *policy_nodes;       /* get/set/mbind */
    unsigned char *il_weights;      /* for mode MPOL_WEIGHTED_INTERLEAVE */
    int policy_node;                /* get: policy node information */
};

Because we use more and more parameters to describe the mempolicy, I
think it's a good idea to replace some parameters with struct.  But I
don't think it's a good idea to put unrelated stuff into the struct.
For example,

struct mempolicy_param {
    unsigned short mode;            /* policy mode */
    unsigned short mode_flags;      /* policy mode flags */
    int home_node;                  /* mbind: use MPOL_MF_HOME_NODE */
    nodemask_t *policy_nodes;
    unsigned char *il_weights;      /* for mode MPOL_WEIGHTED_INTERLEAVE */
};

describe the parameters to create the mempolicy.  It can be used by
set/get_mempolicy() and mbind().  So, I think that it's a good
abstraction.  But "policy_node" has nothing to do with set_mempolicy()
and mbind().  So I think that we shouldn't add it into the struct.  It's
totally OK to use different parameters for different functions.  For
example,

long do_set_mempolicy(struct mempolicy_param *mparam);
long do_mbind(unsigned long start, unsigned long len,
                struct mempolicy_param *mparam, unsigned long flags);
long do_get_task_mempolicy(struct mempolicy_param *mparam, int
                *policy_node);

This isn't the full list.  My point is to use separate parameter for
something specific for some function.

this is the internal structure, but i get the point, we can drop it from
the structure and extend the arg list internally.

I'd originally thought to just remove the policy_node stuff all
together from get_mempolicy2().  Do you prefer to have a separate struct
for set/get interfaces so that the get interface struct can be extended?

All the MPOL_F_NODE "alternate data fetch" mechanisms from
get_mempolicy() feel like more of a wart than a feature.  And presently
the only data returned in policy_node is the next allocation node for
interleave.  That's not even particularly useful, so I'm of a mind to
remove it.

Assuming we remove policy_node altogether... do we still break up the
set/get interface into separate structures to avoid this in the future?

I have no much experience at ABI definition.  So, I want to get guidance
from more experienced people on this.

Is it good to implement all functionality of get_mempolicy() with
get_mempolicy2(), so we can deprecate get_mempolicy() and remove it
finally?  So, users don't need to use 2 similar syscalls?

And, IIUC, we will not get policy_node, addr_node, and policy config at
the same time, is it better to use a union instead of struct in
get_mempolicy2()?

quoted

quoted

struct mpol_args {
        /* Basic mempolicy settings */
        __u16 mode;
        __u16 mode_flags;
        __s32 home_node;
        __aligned_u64 pol_nodes;
        __aligned_u64 *il_weights;      /* of size pol_maxnodes */
        __u64 pol_maxnodes;
        __s32 policy_node;
};

Same as my idea above.  I think we shouldn't add policy_node for
set_mempolicy2()/mbind2().  That will make users confusing.  We can use
a different struct for get_mempolicy2().

See above.

--
Best Regards,
Huang, Ying