Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Oct 2022 11:16:32 +0700
From:   Bagas Sanjaya <bagasdotme@gmail.com>
To:     Zhongkun He <hezhongkun.hzk@bytedance.com>
Cc:     corbet@lwn.net, mhocko@suse.com, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        linux-api@vger.kernel.org, linux-doc@vger.kernel.org,
        wuyun.abel@bytedance.com
Subject: Re: [RFC] mm: add new syscall pidfd_set_mempolicy()
Message-ID: <Y0Y/oGToVk3ags7h@debian.me>
References: <20221010094842.4123037-1-hezhongkun.hzk@bytedance.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="kr4kyCS+bdnKay0q"
Content-Disposition: inline
In-Reply-To: <20221010094842.4123037-1-hezhongkun.hzk@bytedance.com>
Precedence: bulk


--kr4kyCS+bdnKay0q
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Oct 10, 2022 at 05:48:42PM +0800, Zhongkun He wrote:
> There is usecase that System Management Software(SMS) want to give a
> memory policy to other processes to make better use of memory.
>=20

Better say "There are usecases when system management utilities
want to apply memory policy to processes to make better use of memory".

> The information about how to use memory is not known to the app.
> Instead, it is known to the userspace daemon(SMS), and that daemon
> will decide the memory usage policy based on different factors.
>=20

Better say "These utilities doesn't set memory usage policy, but
rather the job of reporting memory usage and setting the policy is
offloaded to an userspace daemon."

> To solve the issue, this patch introduces a new syscall
> pidfd_set_mempolicy(2).  it sets the NUMA memory policy of the thread
> specified in pidfd.
>=20

Better say "To solve the issue above, introduce new syscall
pidfd_set_mempolicy(2). The syscall sets NUMA memory policy for the
thread specified in pidfd".

> In current process context there is no locking because only the process
> accesses its own memory policy, so task_work is used in
> pidfd_set_mempolicy() to update the mempolicy of the process specified
> in pidfd, avoid using locks and race conditions.
>=20

Better say "In current process context there is no locking because
only processes access their own memory policy. For this reason, task_work
is used in pidfd_set_mempolicy() to set or update the mempolicy of process
specified in pid. Thuse, it avoids into race conditions."

> The API is as follows,
>=20
> 		long pidfd_set_mempolicy(int pidfd, int mode,
>                                      const unsigned long __user *nmask,
>                                      unsigned long maxnode,
>                                      unsigned int flags);
>=20
> Set's the [pidfd] task's "task/process memory policy". The pidfd argument
> is a PID file descriptor (see pidfd_open(2) man page) that specifies the
> process to which the mempolicy is to be applied. The flags argument is
> reserved for future use; currently, this argument must be specified as 0.
> Please see the set_mempolicy(2) man page for more details about
> other's arguments.
>=20

Why duplicating from the Documentation/ below?

> Suggested-by: Michal Hocko <mhocko@suse.com>
> Signed-off-by: Zhongkun He <hezhongkun.hzk@bytedance.com>
> ---
>  .../admin-guide/mm/numa_memory_policy.rst     | 21 ++++-
>  arch/alpha/kernel/syscalls/syscall.tbl        |  1 +
>  arch/arm/tools/syscall.tbl                    |  1 +
>  arch/arm64/include/asm/unistd.h               |  2 +-
>  arch/arm64/include/asm/unistd32.h             |  3 +-
>  arch/ia64/kernel/syscalls/syscall.tbl         |  1 +
>  arch/m68k/kernel/syscalls/syscall.tbl         |  1 +
>  arch/microblaze/kernel/syscalls/syscall.tbl   |  1 +
>  arch/mips/kernel/syscalls/syscall_n32.tbl     |  1 +
>  arch/mips/kernel/syscalls/syscall_n64.tbl     |  1 +
>  arch/mips/kernel/syscalls/syscall_o32.tbl     |  1 +
>  arch/parisc/kernel/syscalls/syscall.tbl       |  1 +
>  arch/powerpc/kernel/syscalls/syscall.tbl      |  1 +
>  arch/s390/kernel/syscalls/syscall.tbl         |  1 +
>  arch/sh/kernel/syscalls/syscall.tbl           |  1 +
>  arch/sparc/kernel/syscalls/syscall.tbl        |  1 +
>  arch/x86/entry/syscalls/syscall_32.tbl        |  1 +
>  arch/x86/entry/syscalls/syscall_64.tbl        |  1 +
>  arch/xtensa/kernel/syscalls/syscall.tbl       |  1 +
>  include/linux/mempolicy.h                     | 11 +++
>  include/linux/syscalls.h                      |  4 +
>  include/uapi/asm-generic/unistd.h             |  5 +-
>  kernel/sys_ni.c                               |  1 +
>  mm/mempolicy.c                                | 89 +++++++++++++++++++
>  24 files changed, 146 insertions(+), 6 deletions(-)
>=20
> diff --git a/Documentation/admin-guide/mm/numa_memory_policy.rst b/Docume=
ntation/admin-guide/mm/numa_memory_policy.rst
> index 5a6afecbb0d0..b864dd88b2d2 100644
> --- a/Documentation/admin-guide/mm/numa_memory_policy.rst
> +++ b/Documentation/admin-guide/mm/numa_memory_policy.rst
> @@ -408,9 +408,10 @@ follows:
>  Memory Policy APIs
>  =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> =20
> -Linux supports 4 system calls for controlling memory policy.  These APIS
> -always affect only the calling task, the calling task's address space, or
> -some shared object mapped into the calling task's address space.
> +Linux supports 5 system calls for controlling memory policy.  The first =
four
> +APIS affect only the calling task, the calling task's address space, or =
some
> +shared object mapped into the calling task's address space. The last one=
 can
> +set the mempolicy of task specified in pidfd.
> =20
>  .. note::
>     the headers that define these APIs and the parameter data types for
> @@ -473,6 +474,20 @@ closest to which page allocation will come from. Spe=
cifying the home node overri
>  the default allocation policy to allocate memory close to the local node=
 for an
>  executing CPU.
> =20
> +Set [pidfd Task] Memory Policy::
> +
> +        long sys_pidfd_set_mempolicy(int pidfd, int mode,
> +                                     const unsigned long __user *nmask,
> +                                     unsigned long maxnode,
> +                                     unsigned int flags);
> +
> +Set's the [pidfd] task's "task/process memory policy". The pidfd argumen=
t is
> +a PID file descriptor (see pidfd_open(2) man page) that specifies the pr=
ocess
> +to which the mempolicy is to be applied. The flags argument is reserved =
for
> +future use; currently, this argument must be specified as 0. Please see =
the
> +set_mempolicy(2) man page for more details about other's arguments.
> +
> +
> =20
>  Memory Policy Command Line Interface
>  =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

The wording can be improved:

---- >8 ----

diff --git a/Documentation/admin-guide/mm/numa_memory_policy.rst b/Document=
ation/admin-guide/mm/numa_memory_policy.rst
index b864dd88b2d236..6df35bf4f960bd 100644
--- a/Documentation/admin-guide/mm/numa_memory_policy.rst
+++ b/Documentation/admin-guide/mm/numa_memory_policy.rst
@@ -410,8 +410,8 @@ Memory Policy APIs
=20
 Linux supports 5 system calls for controlling memory policy.  The first fo=
ur
 APIS affect only the calling task, the calling task's address space, or so=
me
-shared object mapped into the calling task's address space. The last one c=
an
-set the mempolicy of task specified in pidfd.
+shared object mapped into the calling task's address space. The last one
+sets the mempolicy of task specified in the pidfd.
=20
 .. note::
    the headers that define these APIs and the parameter data types for
@@ -481,11 +481,11 @@ Set [pidfd Task] Memory Policy::
                                      unsigned long maxnode,
                                      unsigned int flags);
=20
-Set's the [pidfd] task's "task/process memory policy". The pidfd argument =
is
-a PID file descriptor (see pidfd_open(2) man page) that specifies the proc=
ess
-to which the mempolicy is to be applied. The flags argument is reserved for
-future use; currently, this argument must be specified as 0. Please see the
-set_mempolicy(2) man page for more details about other's arguments.
+Sets the task/process memory policy for the [pidfd] task. The pidfd argume=
nt
+is a PID file descriptor (see pidfd_open(2) man page for details) that
+specifies the process for which the mempolicy is applied to. The flags
+argument is reserved for future use; currently, it must be specified as 0.
+For the description of all other arguments, see set_mempolicy(2) man page.
=20
=20
=20

Thanks.

--=20
An old man doll... just what I always wanted! - Clara

--kr4kyCS+bdnKay0q
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQSSYQ6Cy7oyFNCHrUH2uYlJVVFOowUCY0Y/nAAKCRD2uYlJVVFO
oy70AP4wEUgCFmmVfoPF+Ih5oXK6spLPS9lHk0Dk31CIgVAE7wEAgMChcWW248/J
DbLqTdkHQzGWPrcJVo3ID2vm6Zz2MgQ=
=8XfN
-----END PGP SIGNATURE-----

--kr4kyCS+bdnKay0q--