On Tue, Dec 19, 2023 at 11:04:05AM +0800, Huang, Ying wrote: > Gregory Price <gourry.memverge@xxxxxxxxx> writes: > > > 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. > > 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? > > 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. ~Gregory