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

Gregory Price [off-list ref] writes:

On Mon, Dec 11, 2023 at 01:53:40PM +0800, Huang, Ying wrote:

quoted

Hi, Gregory,

Thanks for updated version!

Gregory Price [off-list ref] writes:

quoted

v2:
  changes / adds:
- flattened weight matrix to an array at requested of Ying Huang
- Updated ABI docs per Davidlohr Bueso request
- change uapi structure to use aligned/fixed-length members as
  Suggested-by: Arnd Bergmann [off-list ref]
- Implemented weight fetch logic in get_mempolicy2
- mbind2 was changed to take (iovec,len) as function arguments
  rather than add them to the uapi structure, since they describe
  where to apply the mempolicy - as opposed to being part of it.

    The sysfs structure is designed as follows.

      $ tree /sys/kernel/mm/mempolicy/
      /sys/kernel/mm/mempolicy/
      ├── possible_nodes
      └── weighted_interleave
          ├── nodeN
          │   └── weight
          └── nodeN+X
              └── weight

'mempolicy' is added to '/sys/kernel/mm/' as a control group for
the mempolicy subsystem.

Is it good to add 'mempolicy' in '/sys/kernel/mm/numa'?  The advantage
is that 'mempolicy' here is in fact "NUMA mempolicy".  The disadvantage
is one more directory nesting.  I have no strong opinion here.

i don't have a strong opinion here.

quoted

quoted

'possible_nodes' is added to 'mm/mempolicy' to help describe the
expected structures under mempolicy directorys. For example,
possible_nodes describes what nodeN directories wille exist under
the weighted_interleave directory.

We have '/sys/devices/system/node/possible' already.  Is this just a
duplication?  If so, why?  And, the possible nodes can be gotten via
contents of 'weighted_interleave' too.

I'll remove it

quoted

And it appears not necessary to make 'weighted_interleave/nodeN'
directory.  Why not just make it a file.

Originally I wasn't sure whether there would be more attributes, but
this is probably fine.  I'll change it.

quoted

And, can we add a way to reset weight to the default value?  For example
`echo > nodeN/weight` or `echo > nodeN`.

Seems reasonable.

quoted

quoted

=====================================================================
(Patches 7-10) set_mempolicy2, get_mempolicy2, mbind2

These interfaces are the 'extended' counterpart to their relatives.
They use the userland 'struct mpol_args' structure to communicate a
complete mempolicy configuration to the kernel.  This structure
looks very much like the kernel-internal 'struct mempolicy_args':

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

This looks unnecessarily complex.  I don't think that it's a good idea
to use exact same parameter for all 3 syscalls.

It is exactly as complex as mempolicy is.  Everything here is already
described in the existing interfaces (except il_weights).

quoted

For example, can we use something as below?

  long set_mempolicy2(int mode, const unsigned long *nodemask, unsigned int *il_weights,
                          unsigned long maxnode, unsigned long home_node,
                          unsigned long flags);

  long mbind2(unsigned long start, unsigned long len,
                          int mode, const unsigned long *nodemask, unsigned int *il_weights,
                          unsigned long maxnode, unsigned long home_node,
                          unsigned long flags);

Your definition of mbind2 is impossible.

Neither of these interfaces solve the extensibility issue.  If a new
policy which requires a new format of data arrives, we can look forward
to set_mempolicy3 and mbind3.

IIUC, we will not over-engineering too much.  It's hard to predict the
requirements in the future.

quoted

A struct may be defined to hold mempolicy iteself.

struct mpol {
        int mode;
        unsigned int home_node;
        const unsigned long *nodemask;
        unsigned int *il_weights;
        unsigned int maxnode;
};

addr could be pulled out for get_mempolicy2, so i will do that

'addr_node' and 'policy_node' are warts that came from the original
get_mempolicy.  Removing them increases the complexity of handling
arguments in the common get_mempolicy code.

I could probably just drop support for retrieving the addr_node from
get_mempolicy2, since it's already possible with get_mempolicy.  So I
will do that.

If it's necessary, we can add another struct for get_mempolicy2().  But
I don't think that it's necessary to add get_mempolicy2() specific
parameters for set_mempolicy2() or mbind2().

--
Best Regards,
Huang, Ying

On Tue, Dec 12, 2023 at 03:08:24PM +0800, Huang, Ying wrote:

Gregory Price [off-list ref] writes:

quoted

quoted

For example, can we use something as below?

  long set_mempolicy2(int mode, const unsigned long *nodemask, unsigned int *il_weights,
                          unsigned long maxnode, unsigned long home_node,
                          unsigned long flags);

  long mbind2(unsigned long start, unsigned long len,
                          int mode, const unsigned long *nodemask, unsigned int *il_weights,
                          unsigned long maxnode, unsigned long home_node,
                          unsigned long flags);

Your definition of mbind2 is impossible.

Neither of these interfaces solve the extensibility issue.  If a new
policy which requires a new format of data arrives, we can look forward
to set_mempolicy3 and mbind3.

IIUC, we will not over-engineering too much.  It's hard to predict the
requirements in the future.

Sure, but having the mempolicy struct at least gives us more flexibility
than the original interface.

quoted

quoted

A struct may be defined to hold mempolicy iteself.

struct mpol {
        int mode;
        unsigned int home_node;
        const unsigned long *nodemask;
        unsigned int *il_weights;
        unsigned int maxnode;
};

addr could be pulled out for get_mempolicy2, so i will do that

'addr_node' and 'policy_node' are warts that came from the original
get_mempolicy.  Removing them increases the complexity of handling
arguments in the common get_mempolicy code.

I could probably just drop support for retrieving the addr_node from
get_mempolicy2, since it's already possible with get_mempolicy.  So I
will do that.

If it's necessary, we can add another struct for get_mempolicy2().  But
I don't think that it's necessary to add get_mempolicy2() specific
parameters for set_mempolicy2() or mbind2().

After edits, the only parameter that doesn't have parity between
interfaces is `addr_node` and `policy_node`.  This was an unfortunate
wart on the original get_mempolicy() that multiplexed the output of
(*mode) based on whether MPOL_F_NODE was set.

Example:
if (MPOL_F_ADDR | MPOL_F_NODE), then get_mempolicy() would return
details about a VMA mempolicy + the node of that address in (*mode).

Right now in get_mempolicy2() I fetch this unconditionally instead of
requiring MPOL_F_NODE.  I did not want to multiplexing (*mode) output.

I see two options:
1) Get rid of MPOL_F_NODE functionality in get_mempolicy2()
   If a user wants that information, they can still use get_mempolicy()

2) Keep MPOL_F_NODE and mpol_args->addr_node/policy_node, but don't allow
   any future extensions that create this kind of situation.

I'm fine with either.  I originally aimed for get_mempolicy2() to be
all of get_mempolicy() features + new data, but that obviously isn't
required.

~Gregory