| Message ID | 20240112210834.8035-3-gregory.price@memverge.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/mempolicy: weighted interleave mempolicy with sysfs extension | expand |
Gregory Price <gourry.memverge@gmail.com> writes: > move the use of barrier() to force policy->nodemask onto the stack into > a function `read_once_policy_nodemask` so that it may be re-used. > > Suggested-by: Huang Ying <ying.huang@intel.com> > Signed-off-by: Gregory Price <gregory.price@memverge.com> > --- > mm/mempolicy.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index 5da4fd79fd18..0abd3a3394ef 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -1907,6 +1907,20 @@ unsigned int mempolicy_slab_node(void) > } > } > > +static unsigned int read_once_policy_nodemask(struct mempolicy *pol, > + nodemask_t *mask) It may be more useful if we define this as memcpy_once(). That can be used not only for nodemask, but also other data structure. > +{ > + /* > + * barrier stabilizes the nodemask locally so that it can be iterated > + * over safely without concern for changes. Allocators validate node > + * selection does not violate mems_allowed, so this is safe. > + */ > + barrier(); > + __builtin_memcpy(mask, &pol->nodes, sizeof(nodemask_t)); We don't use __builtin_memcpy() in kernel itself directly. Although it is used in kernel tools. So, I think it's better to use memcpy() here. > + barrier(); > + return nodes_weight(*mask); > +} > + > /* > * Do static interleaving for interleave index @ilx. Returns the ilx'th > * node in pol->nodes (starting from ilx=0), wrapping around if ilx > @@ -1914,20 +1928,12 @@ unsigned int mempolicy_slab_node(void) > */ > static unsigned int interleave_nid(struct mempolicy *pol, pgoff_t ilx) > { > - nodemask_t nodemask = pol->nodes; > + nodemask_t nodemask; > unsigned int target, nnodes; > int i; > int nid; > - /* > - * The barrier will stabilize the nodemask in a register or on > - * the stack so that it will stop changing under the code. > - * > - * Between first_node() and next_node(), pol->nodes could be changed > - * by other threads. So we put pol->nodes in a local stack. > - */ > - barrier(); > > - nnodes = nodes_weight(nodemask); > + nnodes = read_once_policy_nodemask(pol, &nodemask); > if (!nnodes) > return numa_node_id(); > target = ilx % nnodes; -- Best Regards, Huang, Ying
On Mon, Jan 15, 2024 at 12:13:06PM +0800, Huang, Ying wrote: > Gregory Price <gourry.memverge@gmail.com> writes: > > > > > +static unsigned int read_once_policy_nodemask(struct mempolicy *pol, > > + nodemask_t *mask) > > It may be more useful if we define this as memcpy_once(). That can be > used not only for nodemask, but also other data structure. > Seemed better to do this is an entirely separate patch line to avoid scope creep on reviews and such. > > + barrier(); > > + __builtin_memcpy(mask, &pol->nodes, sizeof(nodemask_t)); > > We don't use __builtin_memcpy() in kernel itself directly. Although it > is used in kernel tools. So, I think it's better to use memcpy() here. > ack. ~Gregory
diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 5da4fd79fd18..0abd3a3394ef 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -1907,6 +1907,20 @@ unsigned int mempolicy_slab_node(void) } } +static unsigned int read_once_policy_nodemask(struct mempolicy *pol, + nodemask_t *mask) +{ + /* + * barrier stabilizes the nodemask locally so that it can be iterated + * over safely without concern for changes. Allocators validate node + * selection does not violate mems_allowed, so this is safe. + */ + barrier(); + __builtin_memcpy(mask, &pol->nodes, sizeof(nodemask_t)); + barrier(); + return nodes_weight(*mask); +} + /* * Do static interleaving for interleave index @ilx. Returns the ilx'th * node in pol->nodes (starting from ilx=0), wrapping around if ilx @@ -1914,20 +1928,12 @@ unsigned int mempolicy_slab_node(void) */ static unsigned int interleave_nid(struct mempolicy *pol, pgoff_t ilx) { - nodemask_t nodemask = pol->nodes; + nodemask_t nodemask; unsigned int target, nnodes; int i; int nid; - /* - * The barrier will stabilize the nodemask in a register or on - * the stack so that it will stop changing under the code. - * - * Between first_node() and next_node(), pol->nodes could be changed - * by other threads. So we put pol->nodes in a local stack. - */ - barrier(); - nnodes = nodes_weight(nodemask); + nnodes = read_once_policy_nodemask(pol, &nodemask); if (!nnodes) return numa_node_id(); target = ilx % nnodes;
move the use of barrier() to force policy->nodemask onto the stack into a function `read_once_policy_nodemask` so that it may be re-used. Suggested-by: Huang Ying <ying.huang@intel.com> Signed-off-by: Gregory Price <gregory.price@memverge.com> --- mm/mempolicy.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)