* Re: [PATCH v5 09/11] mm/mempolicy: add get_mempolicy2 syscall
From: Geert Uytterhoeven @ 2024-01-02 14:46 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-doc, linux-fsdevel, linux-kernel, linux-api, x86,
akpm, arnd, tglx, luto, mingo, bp, dave.hansen, hpa, mhocko, tj,
ying.huang, gregory.price, corbet, rakie.kim, hyeongtak.ji,
honggyu.kim, vtavarespetr, peterz, jgroves, ravis.opensrc,
sthanneeru, emirakhur, Hasan.Maruf, seungjun.ha, Michal Hocko
In-Reply-To: <20231223181101.1954-10-gregory.price@memverge.com>
On Sat, Dec 23, 2023 at 7:14 PM Gregory Price <gourry.memverge@gmail.com> wrote:
> get_mempolicy2 is an extensible get_mempolicy interface which allows
> a user to retrieve the memory policy for a task or address.
>
> Defined as:
>
> get_mempolicy2(struct mpol_args *args, size_t size,
> unsigned long addr, unsigned long flags)
>
> Top level input values:
>
> mpol_args: The field which collects information about the mempolicy
> returned to userspace.
> addr: if MPOL_F_ADDR is passed in `flags`, this address will be
> used to return the mempolicy details of the vma the
> address belongs to
> flags: if MPOL_F_ADDR, return mempolicy info vma containing addr
> else, returns task mempolicy information
>
> Input values include the following fields of mpol_args:
>
> pol_nodes: if set, the nodemask of the policy returned here
> pol_maxnodes: if pol_nodes is set, must describe max number of nodes
> to be copied to pol_nodes
>
> Output values include the following fields of mpol_args:
>
> mode: mempolicy mode
> mode_flags: mempolicy mode flags
> home_node: policy home node will be returned here, or -1 if not.
> pol_nodes: if set, the nodemask for the mempolicy
> policy_node: if the policy has extended node information, it will
> be placed here. For example MPOL_INTERLEAVE will
> return the next node which will be used for allocation
>
> MPOL_F_NODE has been dropped from get_mempolicy2 (EINVAL).
> MPOL_F_MEMS_ALLOWED has been dropped from get_mempolicy2 (EINVAL).
>
> Suggested-by: Michal Hocko <mhocko@suse.com>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> arch/m68k/kernel/syscalls/syscall.tbl | 1 +
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v5 10/11] mm/mempolicy: add the mbind2 syscall
From: Geert Uytterhoeven @ 2024-01-02 14:47 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-doc, linux-fsdevel, linux-kernel, linux-api, x86,
akpm, arnd, tglx, luto, mingo, bp, dave.hansen, hpa, mhocko, tj,
ying.huang, gregory.price, corbet, rakie.kim, hyeongtak.ji,
honggyu.kim, vtavarespetr, peterz, jgroves, ravis.opensrc,
sthanneeru, emirakhur, Hasan.Maruf, seungjun.ha, Michal Hocko,
Frank van der Linden
In-Reply-To: <20231223181101.1954-11-gregory.price@memverge.com>
On Sat, Dec 23, 2023 at 7:14 PM Gregory Price <gourry.memverge@gmail.com> wrote:
> mbind2 is an extensible mbind interface which allows a user to
> set the mempolicy for one or more address ranges.
>
> Defined as:
>
> mbind2(unsigned long addr, unsigned long len, struct mpol_args *args,
> size_t size, unsigned long flags)
>
> addr: address of the memory range to operate on
> len: length of the memory range
> flags: MPOL_MF_HOME_NODE + original mbind() flags
>
> Input values include the following fields of mpol_args:
>
> mode: The MPOL_* policy (DEFAULT, INTERLEAVE, etc.)
> mode_flags: The MPOL_F_* flags that were previously passed in or'd
> into the mode. This was split to hopefully allow future
> extensions additional mode/flag space.
> home_node: if (flags & MPOL_MF_HOME_NODE), set home node of policy
> to this otherwise it is ignored.
> pol_maxnodes: The max number of nodes described by pol_nodes
> pol_nodes: the nodemask to apply for the memory policy
>
> The semantics are otherwise the same as mbind(), except that
> the home_node can be set.
>
> Suggested-by: Michal Hocko <mhocko@suse.com>
> Suggested-by: Frank van der Linden <fvdl@google.com>
> Suggested-by: Vinicius Tavares Petrucci <vtavarespetr@micron.com>
> Suggested-by: Rakie Kim <rakie.kim@sk.com>
> Suggested-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
> Suggested-by: Honggyu Kim <honggyu.kim@sk.com>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> Co-developed-by: Vinicius Tavares Petrucci <vtavarespetr@micron.com>
> arch/m68k/kernel/syscalls/syscall.tbl | 1 +
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v5 08/11] mm/mempolicy: add set_mempolicy2 syscall
From: Geert Uytterhoeven @ 2024-01-02 14:38 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-doc, linux-fsdevel, linux-kernel, linux-api, x86,
akpm, arnd, tglx, luto, mingo, bp, dave.hansen, hpa, mhocko, tj,
ying.huang, gregory.price, corbet, rakie.kim, hyeongtak.ji,
honggyu.kim, vtavarespetr, peterz, jgroves, ravis.opensrc,
sthanneeru, emirakhur, Hasan.Maruf, seungjun.ha, Michal Hocko
In-Reply-To: <20231223181101.1954-9-gregory.price@memverge.com>
On Sat, Dec 23, 2023 at 7:13 PM Gregory Price <gourry.memverge@gmail.com> wrote:
> set_mempolicy2 is an extensible set_mempolicy interface which allows
> a user to set the per-task memory policy.
>
> Defined as:
>
> set_mempolicy2(struct mpol_args *args, size_t size, unsigned long flags);
>
> relevant mpol_args fields include the following:
>
> mode: The MPOL_* policy (DEFAULT, INTERLEAVE, etc.)
> mode_flags: The MPOL_F_* flags that were previously passed in or'd
> into the mode. This was split to hopefully allow future
> extensions additional mode/flag space.
> home_node: ignored (see note below)
> pol_nodes: the nodemask to apply for the memory policy
> pol_maxnodes: The max number of nodes described by pol_nodes
>
> The usize arg is intended for the user to pass in sizeof(mpol_args)
> to allow forward/backward compatibility whenever possible.
>
> The flags argument is intended to future proof the syscall against
> future extensions which may require interpreting the arguments in
> the structure differently.
>
> Semantics of `set_mempolicy` are otherwise the same as `set_mempolicy`
> as of this patch.
>
> As of this patch, setting the home node of a task-policy is not
> supported, as this functionality was not supported by set_mempolicy.
> Additional research should be done to determine whether adding this
> functionality is safe, but doing so would only require setting
> MPOL_MF_HOME_NODE and providing a valid home node value.
>
> Suggested-by: Michal Hocko <mhocko@suse.com>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> arch/m68k/kernel/syscalls/syscall.tbl | 1 +
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v3] posix-timers: add multi_clock_gettime system call
From: Arnd Bergmann @ 2024-01-02 11:29 UTC (permalink / raw)
To: Sagi Maimon
Cc: Richard Cochran, Andy Lutomirski, datglx, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Geert Uytterhoeven, Peter Zijlstra, Johannes Weiner, Sohil Mehta,
Rick Edgecombe, Nhat Pham, Palmer Dabbelt, Kees Cook,
Alexey Gladkov, Mark Rutland, linux-kernel, linux-api, Linux-Arch,
Netdev
In-Reply-To: <CAMuE1bF0Hho4VwO6w3f+9z3j5TtscYzuAjj10MFt2mZXG2P8dQ@mail.gmail.com>
On Sun, Dec 31, 2023, at 17:00, Sagi Maimon wrote:
> On Fri, Dec 29, 2023 at 5:27 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> > +struct __ptp_multi_clock_get {
>> > + unsigned int n_clocks; /* Desired number of clocks. */
>> > + unsigned int n_samples; /* Desired number of measurements per clock. */
>> > + clockid_t clkid_arr[MULTI_PTP_MAX_CLOCKS]; /* list of clock IDs */
>> > + /*
>> > + * Array of list of n_clocks clocks time samples n_samples times.
>> > + */
>> > + struct __kernel_timespec ts[MULTI_PTP_MAX_SAMPLES][MULTI_PTP_MAX_CLOCKS];
>> > +};
>>
>> The fixed size arrays here seem to be an unnecessary limitation,
>> both MULTI_PTP_MAX_SAMPLES and MULTI_PTP_MAX_CLOCKS are small
>> enough that one can come up with scenarios where you would want
>> a higher number, but at the same time the structure is already
>> 808 bytes long, which is more than you'd normally want to put
>> on the kernel stack, and which may take a significant time to
>> copy to and from userspace.
>>
>> Since n_clocks and n_samples are always inputs to the syscall,
>> you can just pass them as register arguments and use a dynamically
>> sized array instead.
>>
> Both MULTI_PTP_MAX_SAMPLES and MULTI_PTP_MAX_CLOCKS are enough of any
> usage we can think of,
> But I think you are right, it is better to use a dynamically sized
> array for future use, plus to use less stack memory.
> On patch v4 a dynamically sized array will be used .
> I leaving both MULTI_PTP_MAX_SAMPLES and MULTI_PTP_MAX_CLOCKS but
> increasing their values, since there should be some limitation.
I think having an implementation specific limit in the kernel is
fine, but it would be nice to hardcode that limit in the API.
If both clkidarr[] and ts[] are passed as pointer arguments
in registers, they can be arbitrarily long in the API and
still have a documented maximum that we can extend in the
future without changing the interface.
>> It's not clear to me what you gain from having the n_samples
>> argument over just calling the syscall repeatedly. Does
>> this offer a benefit for accuracy or is this just meant to
>> avoid syscall overhead.
> It is mainly to avoid syscall overhead which also slightly
> improve the accuracy.
This is not a big deal as far as I'm concerned, but it
would be nice to back this up with some numbers if you
think it's worthwhile, as my impression is that the effect
is barely measurable: my guess would be that the syscall
overhead is always much less than the cost for the hardware
access.
>> On the other hand, this will still give less accuracy than the
>> getcrosststamp() callback with ioctl(PTP_SYS_OFFSET_PRECISE),
>> so either the last bit of accuracy isn't all that important,
>> or you need to refine the interface to actually be an
>> improvement over the chardev.
>>
> I don't understand this comment, please explain.
> The ioctl(PTP_SYS_OFFSET_PRECISE) is one specific case that can be
> done by multi_clock_gettime syscall (which cover many more cases)
> Plus the ioctl(PTP_SYS_OFFSET_PRECISE) works only on drivers that
> support this feature.
My point here is that on drivers that do support
PTP_SYS_OFFSET_PRECISE, the extra accuracy should be maintained
by the new interface, ideally in a way that does not have any
other downsides.
I think Andy's suggestion of exposing time offsets instead
of absolute times would actually achieve that: If the
interface is changed to return the offset against
CLOCK_MONOTONIC, CLOCK_MONOTONIC_RAW or CLOCK_BOOTTIME
(not sure what is best here), then the new syscall can use
getcrosststamp() where supported for the best results or
fall back to gettimex64() or gettime64() otherwise to
provide a consistent user interface.
Returning an offset would also allow easily calculating an
average over multiple calls in the kernel, instead of
returning a two-dimensional array.
Arnd
^ permalink raw reply
* Re: [PATCH v5] posix-timers: add multi_clock_gettime system call
From: Arnd Bergmann @ 2024-01-02 10:21 UTC (permalink / raw)
To: Sagi Maimon, Richard Cochran, Andy Lutomirski, datglx,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Geert Uytterhoeven, Peter Zijlstra, Johannes Weiner, Sohil Mehta,
Rick Edgecombe, Nhat Pham, Palmer Dabbelt, Kees Cook,
Alexey Gladkov, Mark Rutland
Cc: linux-kernel, linux-api, Linux-Arch, Netdev
In-Reply-To: <20240102091855.70418-1-maimon.sagi@gmail.com>
On Tue, Jan 2, 2024, at 10:18, Sagi Maimon wrote:
> Some user space applications need to read some clocks.
> Each read requires moving from user space to kernel space.
> The syscall overhead causes unpredictable delay between N clocks reads
> Removing this delay causes better synchronization between N clocks.
>
> Introduce a new system call multi_clock_gettime, which can be used to measure
> the offset between multiple clocks, from variety of types: PHC, virtual PHC
> and various system clocks (CLOCK_REALTIME, CLOCK_MONOTONIC, etc).
> The offset includes the total time that the driver needs to read the clock
> timestamp.
>
> New system call allows the reading of a list of clocks - up to PTP_MAX_CLOCKS.
> Supported clocks IDs: PHC, virtual PHC and various system clocks.
> Up to PTP_MAX_SAMPLES times (per clock) in a single system call read.
> The system call returns n_clocks timestamps for each measurement:
> - clock 0 timestamp
> - ...
> - clock n timestamp
>
> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
> ---
> Changes since version 4:
> - fix error : 'struct __ptp_multi_clock_get' declared inside parameter list
> will not be visible outside of this definition or declaration
I usually put all the changes for previous versions in a
list here, it helps reviewers.
The changes you made for previous versions all look good
to me, but I think there is still a few things worth
considering. I'll also follow up on the earlier threads.
> +#define MULTI_PTP_MAX_CLOCKS 32 /* Max number of clocks */
> +#define MULTI_PTP_MAX_SAMPLES 32 /* Max allowed offset measurement samples. */
> +
> +struct __ptp_multi_clock_get {
> + unsigned int n_clocks; /* Desired number of clocks. */
> + unsigned int n_samples; /* Desired number of measurements per clock. */
> + clockid_t clkid_arr[MULTI_PTP_MAX_CLOCKS]; /* list of clock IDs */
> + /*
> + * Array of list of n_clocks clocks time samples n_samples times.
> + */
> + struct __kernel_timespec ts[MULTI_PTP_MAX_SAMPLES][MULTI_PTP_MAX_CLOCKS];
> +};
Since you now access each member individually, I think it
makes more sense here to just pass these as four
register arguments. It helps with argument introspection,
avoids a couple of get_user(), and lets you remove the fixed
array dimensions.
> +SYSCALL_DEFINE1(multi_clock_gettime, struct __ptp_multi_clock_get
> __user *, ptp_multi_clk_get)
> +{
> + const struct k_clock *kc;
> + struct timespec64 *kernel_tp;
> + struct timespec64 *kernel_tp_base;
> + unsigned int n_clocks; /* Desired number of clocks. */
> + unsigned int n_samples; /* Desired number of measurements per clock.
> */
> + unsigned int i, j;
> + clockid_t clkid_arr[MULTI_PTP_MAX_CLOCKS]; /* list of clock IDs */
> + int error = 0;
> +
> + if (copy_from_user(&n_clocks, &ptp_multi_clk_get->n_clocks,
> sizeof(n_clocks)))
> + return -EFAULT;
> + if (copy_from_user(&n_samples, &ptp_multi_clk_get->n_samples,
> sizeof(n_samples)))
If these remain as struct members rather than register arguments,
you should use get_user() instead of copy_from_user().
> + kernel_tp_base = kmalloc_array(n_clocks * n_samples,
> + sizeof(struct timespec64), GFP_KERNEL);
> + if (!kernel_tp_base)
> + return -ENOMEM;
To be on the safe side regarding possible data leak, maybe use
kcalloc() instead of kmalloc_array() here.
> + kernel_tp = kernel_tp_base;
> + for (j = 0; j < n_samples; j++) {
> + for (i = 0; i < n_clocks; i++) {
> + if (put_timespec64(kernel_tp++, (struct __kernel_timespec __user *)
> + &ptp_multi_clk_get->ts[j][i])) {
I think the typecast here can be removed.
Arnd
^ permalink raw reply
* [PATCH v5] posix-timers: add multi_clock_gettime system call
From: Sagi Maimon @ 2024-01-02 9:18 UTC (permalink / raw)
To: richardcochran, luto, datglx, mingo, bp, dave.hansen, x86, hpa,
arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, maimon.sagi, keescook, legion, mark.rutland
Cc: linux-kernel, linux-api, linux-arch, netdev
Some user space applications need to read some clocks.
Each read requires moving from user space to kernel space.
The syscall overhead causes unpredictable delay between N clocks reads
Removing this delay causes better synchronization between N clocks.
Introduce a new system call multi_clock_gettime, which can be used to measure
the offset between multiple clocks, from variety of types: PHC, virtual PHC
and various system clocks (CLOCK_REALTIME, CLOCK_MONOTONIC, etc).
The offset includes the total time that the driver needs to read the clock
timestamp.
New system call allows the reading of a list of clocks - up to PTP_MAX_CLOCKS.
Supported clocks IDs: PHC, virtual PHC and various system clocks.
Up to PTP_MAX_SAMPLES times (per clock) in a single system call read.
The system call returns n_clocks timestamps for each measurement:
- clock 0 timestamp
- ...
- clock n timestamp
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
Changes since version 4:
- fix error : 'struct __ptp_multi_clock_get' declared inside parameter list
will not be visible outside of this definition or declaration
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 3 +-
include/uapi/asm-generic/unistd.h | 4 +-
include/uapi/linux/multi_clock.h | 21 +++++++++
kernel/time/posix-timers.c | 59 ++++++++++++++++++++++++++
5 files changed, 86 insertions(+), 2 deletions(-)
create mode 100644 include/uapi/linux/multi_clock.h
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 8cb8bf68721c..9cdeb0bf49db 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -378,6 +378,7 @@
454 common futex_wake sys_futex_wake
455 common futex_wait sys_futex_wait
456 common futex_requeue sys_futex_requeue
+457 common multi_clock_gettime sys_multi_clock_gettime
#
# Due to a historical design error, certain syscalls are numbered differently
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index fd9d12de7e92..bde7dec493fd 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -74,6 +74,7 @@ struct landlock_ruleset_attr;
enum landlock_rule_type;
struct cachestat_range;
struct cachestat;
+struct __ptp_multi_clock_get;
#include <linux/types.h>
#include <linux/aio_abi.h>
@@ -1161,7 +1162,7 @@ asmlinkage long sys_mmap_pgoff(unsigned long addr, unsigned long len,
unsigned long prot, unsigned long flags,
unsigned long fd, unsigned long pgoff);
asmlinkage long sys_old_mmap(struct mmap_arg_struct __user *arg);
-
+asmlinkage long sys_multi_clock_gettime(struct __ptp_multi_clock_get __user * ptp_multi_clk_get);
/*
* Not a real system call, but a placeholder for syscalls which are
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 756b013fb832..beb3e0052d3c 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -828,9 +828,11 @@ __SYSCALL(__NR_futex_wake, sys_futex_wake)
__SYSCALL(__NR_futex_wait, sys_futex_wait)
#define __NR_futex_requeue 456
__SYSCALL(__NR_futex_requeue, sys_futex_requeue)
+#define __NR_multi_clock_gettime 457
+__SYSCALL(__NR_multi_clock_gettime, sys_multi_clock_gettime)
#undef __NR_syscalls
-#define __NR_syscalls 457
+#define __NR_syscalls 458
/*
* 32 bit systems traditionally used different
diff --git a/include/uapi/linux/multi_clock.h b/include/uapi/linux/multi_clock.h
new file mode 100644
index 000000000000..5e78dac3a533
--- /dev/null
+++ b/include/uapi/linux/multi_clock.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_MULTI_CLOCK_H
+#define _UAPI_MULTI_CLOCK_H
+
+#include <linux/types.h>
+#include <linux/time_types.h>
+
+#define MULTI_PTP_MAX_CLOCKS 32 /* Max number of clocks */
+#define MULTI_PTP_MAX_SAMPLES 32 /* Max allowed offset measurement samples. */
+
+struct __ptp_multi_clock_get {
+ unsigned int n_clocks; /* Desired number of clocks. */
+ unsigned int n_samples; /* Desired number of measurements per clock. */
+ clockid_t clkid_arr[MULTI_PTP_MAX_CLOCKS]; /* list of clock IDs */
+ /*
+ * Array of list of n_clocks clocks time samples n_samples times.
+ */
+ struct __kernel_timespec ts[MULTI_PTP_MAX_SAMPLES][MULTI_PTP_MAX_CLOCKS];
+};
+
+#endif /* _UAPI_MULTI_CLOCK_H */
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index b924f0f096fa..1d321dc56a25 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -31,6 +31,8 @@
#include <linux/compat.h>
#include <linux/nospec.h>
#include <linux/time_namespace.h>
+#include <linux/multi_clock.h>
+#include <linux/slab.h>
#include "timekeeping.h"
#include "posix-timers.h"
@@ -1426,6 +1428,63 @@ SYSCALL_DEFINE4(clock_nanosleep_time32, clockid_t, which_clock, int, flags,
#endif
+SYSCALL_DEFINE1(multi_clock_gettime, struct __ptp_multi_clock_get __user *, ptp_multi_clk_get)
+{
+ const struct k_clock *kc;
+ struct timespec64 *kernel_tp;
+ struct timespec64 *kernel_tp_base;
+ unsigned int n_clocks; /* Desired number of clocks. */
+ unsigned int n_samples; /* Desired number of measurements per clock. */
+ unsigned int i, j;
+ clockid_t clkid_arr[MULTI_PTP_MAX_CLOCKS]; /* list of clock IDs */
+ int error = 0;
+
+ if (copy_from_user(&n_clocks, &ptp_multi_clk_get->n_clocks, sizeof(n_clocks)))
+ return -EFAULT;
+ if (copy_from_user(&n_samples, &ptp_multi_clk_get->n_samples, sizeof(n_samples)))
+ return -EFAULT;
+ if (n_samples > MULTI_PTP_MAX_SAMPLES)
+ return -EINVAL;
+ if (n_clocks > MULTI_PTP_MAX_CLOCKS)
+ return -EINVAL;
+ if (copy_from_user(clkid_arr, &ptp_multi_clk_get->clkid_arr,
+ sizeof(clockid_t) * n_clocks))
+ return -EFAULT;
+
+ kernel_tp_base = kmalloc_array(n_clocks * n_samples,
+ sizeof(struct timespec64), GFP_KERNEL);
+ if (!kernel_tp_base)
+ return -ENOMEM;
+
+ kernel_tp = kernel_tp_base;
+ for (j = 0; j < n_samples; j++) {
+ for (i = 0; i < n_clocks; i++) {
+ kc = clockid_to_kclock(clkid_arr[i]);
+ if (!kc) {
+ error = -EINVAL;
+ goto out;
+ }
+ error = kc->clock_get_timespec(clkid_arr[i], kernel_tp++);
+ if (error)
+ goto out;
+ }
+ }
+
+ kernel_tp = kernel_tp_base;
+ for (j = 0; j < n_samples; j++) {
+ for (i = 0; i < n_clocks; i++) {
+ if (put_timespec64(kernel_tp++, (struct __kernel_timespec __user *)
+ &ptp_multi_clk_get->ts[j][i])) {
+ error = -EFAULT;
+ goto out;
+ }
+ }
+ }
+out:
+ kfree(kernel_tp_base);
+ return error;
+}
+
static const struct k_clock clock_realtime = {
.clock_getres = posix_get_hrtimer_res,
.clock_get_timespec = posix_get_realtime_timespec,
--
2.26.3
^ permalink raw reply related
* Re: [PATCH v5 03/11] mm/mempolicy: refactor sanitize_mpol_flags for reuse
From: Huang, Ying @ 2024-01-02 9:09 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-doc, linux-fsdevel, linux-kernel,
linux-api, x86, akpm, arnd, tglx, luto, mingo, bp, dave.hansen,
hpa, mhocko, tj, corbet, rakie.kim, hyeongtak.ji, honggyu.kim,
vtavarespetr, peterz, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha
In-Reply-To: <ZYq9klTts4yg8RhG@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Tue, Dec 26, 2023 at 02:05:35AM -0500, Gregory Price wrote:
>> On Wed, Dec 27, 2023 at 04:39:29PM +0800, Huang, Ying wrote:
>> > Gregory Price <gourry.memverge@gmail.com> writes:
>> >
>> > > + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS);
>> > > +
>> > > + *flags = *mode_arg & MPOL_MODE_FLAGS;
>> > > + *mode_arg = mode;
>> >
>> > It appears that it's unnecessary to introduce a local variable to split
>> > mode/flags. Just reuse the original code?
>> >
>
> Revisiting during fixes: Note the change from int to short.
>
> I chose to make this explicit because validate_mpol_flags takes a short.
>
> I'm fairly sure changing it back throws a truncation warning.
Why something like below doesn't work?
int sanitize_mpol_flags(int *mode, unsigned short *flags)
{
*flags = *mode & MPOL_MODE_FLAGS;
*mode &= ~MPOL_MODE_FLAGS;
return validate_mpol_flags(*mode, flags);
}
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [PATCH v5 02/11] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Huang, Ying @ 2024-01-02 8:42 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-doc, linux-fsdevel, linux-kernel,
linux-api, x86, akpm, arnd, tglx, luto, mingo, bp, dave.hansen,
hpa, mhocko, tj, corbet, rakie.kim, hyeongtak.ji, honggyu.kim,
vtavarespetr, peterz, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, Srinivasulu Thanneeru
In-Reply-To: <ZYp6ZRLZQVtTHest@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Wed, Dec 27, 2023 at 04:32:37PM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@gmail.com> writes:
>>
>> > +static unsigned int weighted_interleave_nid(struct mempolicy *pol, pgoff_t ilx)
>> > +{
>> > + nodemask_t nodemask = pol->nodes;
>> > + unsigned int target, weight_total = 0;
>> > + int nid;
>> > + unsigned char weights[MAX_NUMNODES];
>>
>> MAX_NUMNODSE could be as large as 1024. 1KB stack space may be too
>> large?
>>
>
> I've been struggling with a good solution to this. We need a local copy
> of weights to prevent weights from changing out from under us during
> allocation (which may take quite some time), but it seemed unwise to
> to allocate 1KB heap in this particular path.
>
> Is my concern unfounded? If so, I can go ahead and add the allocation
> code.
Please take a look at NODEMASK_ALLOC().
>> > + unsigned char weight;
>> > +
>> > + barrier();
>>
>> Memory barrier needs comments.
>>
>
> Barrier is to stabilize nodemask on the stack, but yes i'll carry the
> comment from interleave_nid into this barrier as well.
Please see below.
>> > +
>> > + /* first ensure we have a valid nodemask */
>> > + nid = first_node(nodemask);
>> > + if (nid == MAX_NUMNODES)
>> > + return nid;
>>
>> It appears that this isn't necessary, because we can check whether
>> weight_total == 0 after the next loop.
>>
>
> fair, will snip.
>
>> > +
>> > + /* Then collect weights on stack and calculate totals */
>> > + for_each_node_mask(nid, nodemask) {
>> > + weight = iw_table[nid];
>> > + weight_total += weight;
>> > + weights[nid] = weight;
>> > + }
>> > +
>> > + /* Finally, calculate the node offset based on totals */
>> > + target = (unsigned int)ilx % weight_total;
>>
>> Why use type casting?
>>
>
> Artifact of old prototypes, snipped.
>
>> > +
>> > + /* Stabilize the nodemask on the stack */
>> > + barrier();
>>
>> I don't think barrier() is needed to wait for memory operations for
>> stack. It's usually used for cross-processor memory order.
>>
>
> This is present in the old interleave code. To the best of my
> understanding, the concern is for mempolicy->nodemask rebinding that can
> occur when cgroups.cpusets.mems_allowed changes.
>
> so we can't iterate over (mempolicy->nodemask), we have to take a local
> copy.
>
> My *best* understanding of the barrier here is to prevent the compiler
> from reordering operations such that it attempts to optimize out the
> local copy (or do lazy-fetch).
>
> It is present in the original interleave code, so I pulled it forward to
> this, but I have not tested whether this is a bit paranoid or not.
>
> from `interleave_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();
Got it. This is kind of READ_ONCE() for nodemask. To avoid to add
comments all over the place. Can we implement a wrapper for it? For
example, memcpy_once(). __read_once_size() in
tools/include/linux/compiler.h can be used as reference.
Because node_weights[] may be changed simultaneously too. We may need
to consider similar issue for it too. But RCU seems more appropriate
for node_weights[].
>> > + /* Otherwise we adjust nr_pages down, and continue from there */
>> > + rem_pages -= pol->wil.cur_weight;
>> > + pol->wil.cur_weight = 0;
>> > + prev_node = node;
>>
>> If pol->wil.cur_weight == 0, prev_node will be used without being
>> initialized below.
>>
>
> pol->wil.cur_weight is not used below.
>
>> > + }
>> > +
>> > + /* Now we can continue allocating as if from 0 instead of an offset */
>> > + rounds = rem_pages / weight_total;
>> > + delta = rem_pages % weight_total;
>> > + for (i = 0; i < nnodes; i++) {
>> > + node = next_node_in(prev_node, nodes);
>> > + weight = weights[node];
>> > + node_pages = weight * rounds;
>> > + if (delta) {
>> > + if (delta > weight) {
>> > + node_pages += weight;
>> > + delta -= weight;
>> > + } else {
>> > + node_pages += delta;
>> > + delta = 0;
>> > + }
>> > + }
>> > + /* We may not make it all the way around */
>> > + if (!node_pages)
>> > + break;
>> > + /* If an over-allocation would occur, floor it */
>> > + if (node_pages + total_allocated > nr_pages) {
>>
>> Why is this possible?
>>
>
> this may have been a paranoid artifact from an early prototype, will
> snip and validate.
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [PATCH v5 01/11] mm/mempolicy: implement the sysfs-based weighted_interleave interface
From: Huang, Ying @ 2024-01-02 7:41 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-doc, linux-fsdevel, linux-kernel,
linux-api, x86, akpm, arnd, tglx, luto, mingo, bp, dave.hansen,
hpa, mhocko, tj, corbet, rakie.kim, hyeongtak.ji, honggyu.kim,
vtavarespetr, peterz, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha
In-Reply-To: <ZYp3JbcCPQc4fUrB@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Wed, Dec 27, 2023 at 02:42:15PM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@gmail.com> writes:
>>
>> > + These weights only affect new allocations, and changes at runtime
>> > + will not cause migrations on already allocated pages.
>> > +
>> > + Writing an empty string resets the weight value to 1.
>>
>> I still think that it's a good idea to provide some better default
>> weight value with HMAT or CDAT if available. So, better not to make "1"
>> as part of ABI?
>>
>
> That's the eventual goal,
Good to know this.
> but this is just the initial mechanism.
>
> My current thought is that the CXL driver will apply weights as the
> system iterates through devices and creates numa nodes. In the
> meantime, you have to give the "possible" nodes a default value to
> prevent nodes onlined after boot from showing up with 0-value.
>
> Not allowing 0-value weights is simply easier in many respects.
>
>> > +
>> > + Minimum weight: 1
>>
>> Can weight be "0"? Do we need a way to specify that a node don't want
>> to participate weighted interleave?
>>
>
> In this code, weight cannot be 0. My thoguht is that removing the node
> from the nodemask is the way to denote 0.
>
> The problem with 0 is hotplug, migration, and cpusets.mems_allowed.
>
> Example issue: Use set local weights to [1,0,1,0] for nodes [0-3],
> and has a cpusets.mems_allowed mask of (0, 2).
>
> Lets say the user migrates the task via cgroups from nodes (0,2) to
> (1,3).
>
> The task will instantly crash as basically OOM because weights of
> [1,0,1,0] will prevent memory from being allocations.
>
> Not allowing nodes weights of 0 is defensive. Instead, simply removing
> the node from the nodemask and/or mems_allowed is both equivalent to and
> the preferred way to apply a weight of 0.
It sounds reasonable to set minimum weight to 1. But "1" may be not the
default weight, so, I don't think it's a good idea to make "1" as
default in ABI.
>> > + Maximum weight: 255
>> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> > index 10a590ee1c89..0e77633b07a5 100644
>> > --- a/mm/mempolicy.c
>> > +++ b/mm/mempolicy.c
>> > @@ -131,6 +131,8 @@ static struct mempolicy default_policy = {
>> >
>> > static struct mempolicy preferred_node_policy[MAX_NUMNODES];
>> >
>> > +static char iw_table[MAX_NUMNODES];
>> > +
>>
>> It's kind of obscure whether "char" is "signed" or "unsigned". Given
>> the max weight is 255 above, it's better to use "u8"?
>>
>
> bah, stupid mistake. I will switch this to u8.
>
>> And, we may need a way to specify whether the weight has been overridden
>> by the user.
>> A special value (such as 255) can be used for that. If
>> so, the maximum weight should be 254 instead of 255. As a user space
>> interface, is it better to use 100 as the maximum value?
>>
>
> There's global weights and local weights. These are the global weights.
>
> Local weights are stored in task->mempolicy.wil.il_weights.
>
> (policy->mode_flags & MPOL_F_GWEIGHT) denotes the override.
> This is set if (mempolicy_args->il_weights) was provided.
>
> This simplifies the interface.
>
> (note: local weights are not introduced until the last patch 11/11)
The global weight is writable via sysfs too, right? Then, for global
weights, we have 2 sets of values,
iw_table_default[], and iw_table[].
iw_table_default[] is set to "1" now, and will be set to some other
values after we have enabled HMAT/CDAT based default value.
iw_table[] is initialized with a special value (for example, "0", if "1"
is the minimal weight). And users can change it via sysfs. Then the
actual global weight for a node becomes
iw_table[node] ? : iw_table_default[node]
As for global weight and local weight, we may need a way to copy from
the global weights to the local weights to simplify the memory policy
setup. For example, if users want to use the global weights of node 0,
1, 2 and override the weight of node 3. They can specify some special
value, for example, 0, in mempolicy_args->il_weights[0], [1], [2] to
copy from the global values, and override [3] via some other value.
Think about the default weight value via HMAT/CDAT again. It may be not
a good idea to use "1" as default even for now.
For example,
- The memory bandwidth of DRAM is 100GB, whose default weight is "1".
- We hot-plug CXL.mem A with memory bandwidth 20GB. So, we change the
weight of DRAM to 5, and use "1" as the weight of CXL.mem A.
- We hot-plug CXL.mem B with memory bandwidth 10GB. So, we change the
weight of DRAM to 10, the weight of CXL.mem A to 2, and use "1" as the
weight of CXL.mem B.
That is, if we use "1" as default weight, we need to change weights of
nodes frequently because we haven't a "base" weight. The best candidate
base weight is the weight of DRAM node. For example, if we set the
default weight of DRAM node to be "16" and use that as the base weight,
we don't need to change it in most cases. The weight of other nodes can
be set according to the ratio of its memory bandwidth to that of DRAM.
This makes it easy to set the default weight via HMAT/CDAT too.
What do you think about that?
>> > +
>> > +static void sysfs_mempolicy_release(struct kobject *mempolicy_kobj)
>> > +{
>> > + int i;
>> > +
>> > + for (i = 0; i < MAX_NUMNODES; i++)
>> > + sysfs_wi_node_release(node_attrs[i], mempolicy_kobj);
>>
>> IIUC, if this is called in error path (such as, in
>> add_weighted_interleave_group()), some node_attrs[] element may be
>> "NULL"?
>>
>
> The null check is present in sysfs_wi_node_release
>
> if (!node_attr)
> return;
This works. Sorry for noise.
> Is it preferable to pull this out? Seemed more defensive to put it
> inside the function.
Both are OK for me.
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [PATCH v5 00/11] mempolicy2, mbind2, and weighted interleave
From: Huang, Ying @ 2024-01-02 4:27 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-doc, linux-fsdevel, linux-kernel,
linux-api, x86, akpm, arnd, tglx, luto, mingo, bp, dave.hansen,
hpa, mhocko, tj, corbet, rakie.kim, hyeongtak.ji, honggyu.kim,
vtavarespetr, peterz, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, Johannes Weiner,
Hasan Al Maruf, Hao Wang, Dan Williams, Michal Hocko, Zhongkun He,
Frank van der Linden, John Groves, Jonathan Cameron
In-Reply-To: <ZYqEjsaqseI68EyJ@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Mon, Dec 25, 2023 at 03:54:18PM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@gmail.com> writes:
>>
>> > For example, the stream benchmark demonstrates that default interleave
>> > is actively harmful, where weighted interleave is beneficial.
>> >
>> > Hardware: 1-socket 8 channel DDR5 + 1 CXL expander in PCIe x16
>> > Default interleave : -78% (slower than DRAM)
>> > Global weighting : -6% to +4% (workload dependant)
>> > Targeted weights : +2.5% to +4% (consistently better than DRAM)
>> >
>> > If nothing else, this shows how awful round-robin interleave is.
>>
>> I guess the performance of the default policy, local (fast memory)
>> first, may be even better in some situation? For example, before the
>> bandwidth of DRAM is saturated?
>>
>
> Yes - but it's more complicated than that.
>
> Global weighting here means we did `numactl -w --interleave ...`, which
> means *all* memory regions will be interleaved. Code, stack, heap, etc.
>
> Targeted weights means we used mbind2() with local weights, which only
> targted specific heap regions.
>
> The default policy was better than global weighting likely as a result
> of things like stack/code being distributed to higher latency memory
> produced a measurable overhead.
>
> To provide this, we only applied weights to bandwidth driving regions,
> and as a result we demonstrated a measurable performance increase.
>
> So yes, the defautl policy may be better in some situations - but that
> will be true of any policy.
Yes. Some memory area may be more sensitive to memory latency than
other area.
Per my understanding, memory latency will increase with the actual
memory throughput. And it increases quickly when the memory throughput
nears the maximum memory bandwidth. As in the figures in the following
URL.
https://mahmoudhatem.wordpress.com/2017/11/07/memory-bandwidth-vs-latency-response-curve/
If the memory latency of the DRAM will not increase much, it's better to
place the hot data in DRAM always. But if the memory throughput nears
the max memory bandwidth, so that the memory latency of DRAM increases
greatly, may be even higher than that of CXL memory, it's better to put
some hot data in CXL memory to reduce the overall memory latency.
If it's right, I suggest to add something like above in the patch
description.
>> I understand that you may want to limit the memory usage of the fast
>> memory too. But IMHO, that is another requirements. That should be
>> enforced by something like per-node memory limit.
>>
>
> This interface does not limit memory usage of a particular node, it
> distributes data according to the requested policy.
>
> Nuanced distinction, but important. If nodes become exhausted, tasks
> are still free to allocate memory from any node in the nodemask, even if
> it violates the requested mempolicy.
>
> This is consistent with the existing behavior of mempolicy.
Good.
>> > =====================================================================
>> > (Patches 3-6) Refactoring mempolicy for code-reuse
>> >
>> > To avoid multiple paths of mempolicy creation, we should refactor the
>> > existing code to enable the designed extensibility, and refactor
>> > existing users to utilize the new interface (while retaining the
>> > existing userland interface).
>> >
>> > This set of patches introduces a new mempolicy_args structure, which
>> > is used to more fully describe a requested mempolicy - to include
>> > existing and future extensions.
>> >
>> > /*
>> > * Describes settings of a mempolicy during set/get syscalls and
>> > * kernel internal calls to do_set_mempolicy()
>> > */
>> > 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 */
>> > };
>>
>> According to
>>
>> https://www.geeksforgeeks.org/difference-between-argument-and-parameter-in-c-c-with-examples/
>>
>> it appears that "parameter" are better than "argument" for struct name
>> here. It appears that current kernel source supports this too.
>>
>> $ grep 'struct[\t ]\+[a-zA-Z0-9]\+_param' -r include/linux | wc -l
>> 411
>> $ grep 'struct[\t ]\+[a-zA-Z0-9]\+_arg' -r include/linux | wc -l
>> 25
>>
>
> Will change.
>
>> > This arg structure will eventually be utilized by the following
>> > interfaces:
>> > mpol_new() - new mempolicy creation
>> > do_get_mempolicy() - acquiring information about mempolicy
>> > do_set_mempolicy() - setting the task mempolicy
>> > do_mbind() - setting a vma mempolicy
>> >
>> > do_get_mempolicy() is completely refactored to break it out into
>> > separate functionality based on the flags provided by get_mempolicy(2)
>> > MPOL_F_MEMS_ALLOWED: acquires task->mems_allowed
>> > MPOL_F_ADDR: acquires information on vma policies
>> > MPOL_F_NODE: changes the output for the policy arg to node info
>> >
>> > We refactor the get_mempolicy syscall flatten the logic based on these
>> > flags, and aloow for set_mempolicy2() to re-use the underlying logic.
>> >
>> > The result of this refactor, and the new mempolicy_args structure, is
>> > that extensions like 'sys_set_mempolicy_home_node' can now be directly
>> > integrated into the initial call to 'set_mempolicy2', and that more
>> > complete information about a mempolicy can be returned with a single
>> > call to 'get_mempolicy2', rather than multiple calls to 'get_mempolicy'
>> >
>> >
>> > =====================================================================
>> > (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;
>> > __u64 pol_maxnodes;
>>
>> I understand that we want to avoid hole in struct. But I still feel
>> uncomfortable to use __u64 for a small. But I don't have solution too.
>> Anyone else has some idea?
>>
>
> maxnode has been an `unsigned long` in every other interface for quite
> some time. Seems better to keep this consistent rather than it suddenly
> become `unsigned long` over here and `unsigned short` over there.
I don't think that it matters. The actual maximum node number will be
less than maximum `unsigned short`.
>> > __aligned_u64 pol_nodes;
>> > __aligned_u64 *il_weights; /* of size pol_maxnodes */
>>
>> Typo? Should be,
>>
>
> derp derp
>
>> >
>> > The 'flags' argument for mbind2 is the same as 'mbind', except with
>> > the addition of MPOL_MF_HOME_NODE to denote whether the 'home_node'
>> > field should be utilized.
>> >
>> > The 'flags' argument for get_mempolicy2 allows for MPOL_F_ADDR to
>> > allow operating on VMA policies, but MPOL_F_NODE and MPOL_F_MEMS_ALLOWED
>> > behavior has been omitted, since get_mempolicy() provides this already.
>>
>> I still think that it's a good idea to make it possible to deprecate
>> get_mempolicy(). How about use a union as follows?
>>
>> struct mpol_mems_allowed {
>> __u64 maxnodes;
>> __aligned_u64 nodemask;
>> };
>>
>> union mpol_info {
>> struct mpol_args args;
>> struct mpol_mems_allowed mems_allowed;
>> __s32 node;
>> };
>>
>
> See my other email. I've come around to see mems_allowed as a wart that
> needs to be removed. The same information is already available via
> sysfs cpusets.mems and cpusets.mems_effective.
>
> Additionally, mems_allowed isn't even technically part of the mempolicy,
> so if we did want an interface to acquire the infomation, you'd prefer
> to just implement a stand-alone syscall.
>
> The sysfs interface seems sufficient though.
>
> `policy_node` is a similar "why does this even exist" type feature,
> except that it can still be used from get_mempolicy() and if there is an
> actual reason to extend it to get_mempolicy2() it can be added to
> mpol_params.
OK.
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [PATCH v4 00/11] mempolicy2, mbind2, and weighted interleave
From: Huang, Ying @ 2024-01-02 4:08 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-doc, linux-fsdevel, linux-kernel,
linux-api, x86, akpm, arnd, tglx, luto, mingo, bp, dave.hansen,
hpa, mhocko, tj, corbet, rakie.kim, hyeongtak.ji, honggyu.kim,
vtavarespetr, peterz, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, Johannes Weiner,
Hasan Al Maruf, Hao Wang, Dan Williams, Michal Hocko, Zhongkun He,
Frank van der Linden, John Groves, Jonathan Cameron
In-Reply-To: <ZYqAHesihJ+XCCyy@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Wed, Dec 20, 2023 at 10:27:06AM +0800, Huang, Ying wrote:
>> Gregory Price <gregory.price@memverge.com> writes:
>>
>> > 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()?
>>
>
> We discussed using flags to change the operation of mempolicy earlier
> and it was expressed that multiplexing syscalls via flags is no longer
> a preferred design because it increases complexity in the long term.
In general, I agree with that. "ioctl" isn't the best pattern to define
syscall.
> The mems_allowed extension to get_mempolicy() is basically this kind of
> multiplexing. So ultimately I think it better to simply remove that
> functionality from get_mempolicy2().
>
> Further: it's not even technically *part* of mempolicy, it's part of
> cpusets, and is accessible via sysfs through some combination of
> cpuset.mems and cpuset.mems.effective.
>
> So the mems_allowed part of get_mempolicy() has already been deprecated
> in that way. Doesn't seem worth it to add it to mempolicy2.
>
>
> The `policy_node` is more of a question as to whether it's even useful.
> Right now it only applies to interleave policies... but it's also
> insanely racey. The moment you pluck the next interleave target, it's
> liable to change. I don't know how anyone would even use this.
Both sounds reasonable for me. How about add this into the patch
description? This will help anyone who want to know why the syscall is
defined this way.
> If we drop it, we can alway add it back in with an extension if someone
> actually has a use-case for it and we decide to fully deprecate
> get_mempolicy() (which seems unlikely, btw).
I still think it's possible, after decades.
> In either case, the extension I made allows get_mempolicy() to be used
> to fetch policy_node via the original method, for new policies, so that
> would cover it if anyone is actually using it.
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [PATCH v4] posix-timers: add multi_clock_gettime system call
From: Sagi Maimon @ 2024-01-01 8:44 UTC (permalink / raw)
To: Andy Lutomirski
Cc: richardcochran, datglx, mingo, bp, dave.hansen, x86, hpa, arnd,
geert, peterz, hannes, sohil.mehta, rick.p.edgecombe, nphamcs,
palmer, keescook, legion, mark.rutland, linux-kernel, linux-api,
linux-arch, netdev
In-Reply-To: <CALCETrUd=16gAYvx93EsyMaaSJ-6mLvSru8Gie48Y+_dXq5FGA@mail.gmail.com>
On Sun, Dec 31, 2023 at 11:10 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Sun, Dec 31, 2023 at 9:07 AM Sagi Maimon <maimon.sagi@gmail.com> wrote:
> >
> > Some user space applications need to read some clocks.
> > Each read requires moving from user space to kernel space.
> > The syscall overhead causes unpredictable delay between N clocks reads
> > Removing this delay causes better synchronization between N clocks.
> >
> > Introduce a new system call multi_clock_gettime, which can be used to measure
> > the offset between multiple clocks, from variety of types: PHC, virtual PHC
> > and various system clocks (CLOCK_REALTIME, CLOCK_MONOTONIC, etc).
> > The offset includes the total time that the driver needs to read the clock
> > timestamp.
Andy Thank you for your notes.
>
> Knowing this offset sounds quite nice, but...
>
> >
> > New system call allows the reading of a list of clocks - up to PTP_MAX_CLOCKS.
> > Supported clocks IDs: PHC, virtual PHC and various system clocks.
> > Up to PTP_MAX_SAMPLES times (per clock) in a single system call read.
> > The system call returns n_clocks timestamps for each measurement:
> > - clock 0 timestamp
> > - ...
> > - clock n timestamp
>
> Could this instead be arranged to read the actual, exact offset?
>
It can be done, but I prefer to leave it generic and consistent with
other time system calls.
In most cases the offset calculation is done in user space application
> > + kernel_tp = kernel_tp_base;
> > + for (j = 0; j < n_samples; j++) {
> > + for (i = 0; i < n_clocks; i++) {
> > + if (put_timespec64(kernel_tp++, (struct __kernel_timespec __user *)
> > + &ptp_multi_clk_get->ts[j][i])) {
> > + error = -EFAULT;
> > + goto out;
> > + }
> > + }
> > + }
>
> There are several pairs of clocks that tick at precisely same rate
> (and use the same underlying hardware clock), and the offset could be
> computed exactly instead of doing this noisy loop that is merely
> somewhat less bad than what user code could do all by itself.
You are correct, there are some PHCs on the same NIC (each per port)
that share the same HW counter/clock.
In that case it is slightly better to do the offset calculation in the
NIC driver code,
but that requires changes in each NIC driver's code.
The main thing is that the multi_clock_gettime system call is a
generic solution,
it covers that case among other cases, for example sync between two
PHCs on different NICs.
^ permalink raw reply
* Re: [PATCH v4] posix-timers: add multi_clock_gettime system call
From: kernel test robot @ 2023-12-31 23:11 UTC (permalink / raw)
To: Sagi Maimon, richardcochran, luto, datglx, mingo, bp, dave.hansen,
x86, hpa, arnd, geert, peterz, hannes, sohil.mehta,
rick.p.edgecombe, nphamcs, palmer, keescook, legion, mark.rutland
Cc: oe-kbuild-all, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <20231231170721.3381-1-maimon.sagi@gmail.com>
Hi Sagi,
kernel test robot noticed the following build errors:
[auto build test ERROR on tip/x86/asm]
[also build test ERROR on tip/timers/core linus/master v6.7-rc7]
[cannot apply to arnd-asm-generic/master next-20231222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sagi-Maimon/posix-timers-add-multi_clock_gettime-system-call/20240101-011104
base: tip/x86/asm
patch link: https://lore.kernel.org/r/20231231170721.3381-1-maimon.sagi%40gmail.com
patch subject: [PATCH v4] posix-timers: add multi_clock_gettime system call
config: sparc-allmodconfig (https://download.01.org/0day-ci/archive/20240101/202401010719.QfVc3HOt-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240101/202401010719.QfVc3HOt-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401010719.QfVc3HOt-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from arch/sparc/kernel/setup_64.c:21:
>> include/linux/syscalls.h:1164:48: error: 'struct __ptp_multi_clock_get' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
1164 | asmlinkage long sys_multi_clock_gettime(struct __ptp_multi_clock_get __user * ptp_multi_clk_get);
| ^~~~~~~~~~~~~~~~~~~~~
arch/sparc/kernel/setup_64.c:602:13: error: no previous prototype for 'alloc_irqstack_bootmem' [-Werror=missing-prototypes]
602 | void __init alloc_irqstack_bootmem(void)
| ^~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
--
In file included from arch/sparc/kernel/sys_sparc_64.c:25:
>> include/linux/syscalls.h:1164:48: error: 'struct __ptp_multi_clock_get' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
1164 | asmlinkage long sys_multi_clock_gettime(struct __ptp_multi_clock_get __user * ptp_multi_clk_get);
| ^~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +1164 include/linux/syscalls.h
1154
1155 /* obsolete */
1156 asmlinkage long sys_ipc(unsigned int call, int first, unsigned long second,
1157 unsigned long third, void __user *ptr, long fifth);
1158
1159 /* obsolete */
1160 asmlinkage long sys_mmap_pgoff(unsigned long addr, unsigned long len,
1161 unsigned long prot, unsigned long flags,
1162 unsigned long fd, unsigned long pgoff);
1163 asmlinkage long sys_old_mmap(struct mmap_arg_struct __user *arg);
> 1164 asmlinkage long sys_multi_clock_gettime(struct __ptp_multi_clock_get __user * ptp_multi_clk_get);
1165
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v4] posix-timers: add multi_clock_gettime system call
From: kernel test robot @ 2023-12-31 22:09 UTC (permalink / raw)
To: Sagi Maimon, richardcochran, luto, datglx, mingo, bp, dave.hansen,
x86, hpa, arnd, geert, peterz, hannes, sohil.mehta,
rick.p.edgecombe, nphamcs, palmer, keescook, legion, mark.rutland
Cc: oe-kbuild-all, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <20231231170721.3381-1-maimon.sagi@gmail.com>
Hi Sagi,
kernel test robot noticed the following build errors:
[auto build test ERROR on tip/x86/asm]
[also build test ERROR on arnd-asm-generic/master tip/timers/core linus/master v6.7-rc7]
[cannot apply to next-20231222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sagi-Maimon/posix-timers-add-multi_clock_gettime-system-call/20240101-011104
base: tip/x86/asm
patch link: https://lore.kernel.org/r/20231231170721.3381-1-maimon.sagi%40gmail.com
patch subject: [PATCH v4] posix-timers: add multi_clock_gettime system call
config: nios2-randconfig-002-20240101 (https://download.01.org/0day-ci/archive/20240101/202401010512.duJmX3qR-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240101/202401010512.duJmX3qR-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401010512.duJmX3qR-lkp@intel.com/
All errors (new ones prefixed by >>):
>> nios2-linux-ld: arch/nios2/kernel/syscall_table.o:(.data+0x724): undefined reference to `sys_multi_clock_gettime'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v4] posix-timers: add multi_clock_gettime system call
From: Andy Lutomirski @ 2023-12-31 21:09 UTC (permalink / raw)
To: Sagi Maimon
Cc: richardcochran, luto, datglx, mingo, bp, dave.hansen, x86, hpa,
arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, keescook, legion, mark.rutland, linux-kernel,
linux-api, linux-arch, netdev
In-Reply-To: <20231231170721.3381-1-maimon.sagi@gmail.com>
On Sun, Dec 31, 2023 at 9:07 AM Sagi Maimon <maimon.sagi@gmail.com> wrote:
>
> Some user space applications need to read some clocks.
> Each read requires moving from user space to kernel space.
> The syscall overhead causes unpredictable delay between N clocks reads
> Removing this delay causes better synchronization between N clocks.
>
> Introduce a new system call multi_clock_gettime, which can be used to measure
> the offset between multiple clocks, from variety of types: PHC, virtual PHC
> and various system clocks (CLOCK_REALTIME, CLOCK_MONOTONIC, etc).
> The offset includes the total time that the driver needs to read the clock
> timestamp.
Knowing this offset sounds quite nice, but...
>
> New system call allows the reading of a list of clocks - up to PTP_MAX_CLOCKS.
> Supported clocks IDs: PHC, virtual PHC and various system clocks.
> Up to PTP_MAX_SAMPLES times (per clock) in a single system call read.
> The system call returns n_clocks timestamps for each measurement:
> - clock 0 timestamp
> - ...
> - clock n timestamp
Could this instead be arranged to read the actual, exact offset?
> + kernel_tp = kernel_tp_base;
> + for (j = 0; j < n_samples; j++) {
> + for (i = 0; i < n_clocks; i++) {
> + if (put_timespec64(kernel_tp++, (struct __kernel_timespec __user *)
> + &ptp_multi_clk_get->ts[j][i])) {
> + error = -EFAULT;
> + goto out;
> + }
> + }
> + }
There are several pairs of clocks that tick at precisely same rate
(and use the same underlying hardware clock), and the offset could be
computed exactly instead of doing this noisy loop that is merely
somewhat less bad than what user code could do all by itself.
^ permalink raw reply
* [PATCH v4] posix-timers: add multi_clock_gettime system call
From: Sagi Maimon @ 2023-12-31 17:07 UTC (permalink / raw)
To: richardcochran, luto, datglx, mingo, bp, dave.hansen, x86, hpa,
arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, maimon.sagi, keescook, legion, mark.rutland
Cc: linux-kernel, linux-api, linux-arch, netdev
Some user space applications need to read some clocks.
Each read requires moving from user space to kernel space.
The syscall overhead causes unpredictable delay between N clocks reads
Removing this delay causes better synchronization between N clocks.
Introduce a new system call multi_clock_gettime, which can be used to measure
the offset between multiple clocks, from variety of types: PHC, virtual PHC
and various system clocks (CLOCK_REALTIME, CLOCK_MONOTONIC, etc).
The offset includes the total time that the driver needs to read the clock
timestamp.
New system call allows the reading of a list of clocks - up to PTP_MAX_CLOCKS.
Supported clocks IDs: PHC, virtual PHC and various system clocks.
Up to PTP_MAX_SAMPLES times (per clock) in a single system call read.
The system call returns n_clocks timestamps for each measurement:
- clock 0 timestamp
- ...
- clock n timestamp
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
Addressed comments from:
- Arnd Bergmann : https://www.spinics.net/lists/netdev/msg962019.html
Changes since version 3:
- Replacing the clkid fixed size arrays with a dynamically sized array.
- Coping only necessary data from user space.
- Calling put_timespec64() only after all time samples taken in order to
reduce the put_timespec64() overhead.
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 2 +-
include/uapi/asm-generic/unistd.h | 4 +-
include/uapi/linux/multi_clock.h | 21 +++++++++
kernel/time/posix-timers.c | 59 ++++++++++++++++++++++++++
5 files changed, 85 insertions(+), 2 deletions(-)
create mode 100644 include/uapi/linux/multi_clock.h
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 8cb8bf68721c..9cdeb0bf49db 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -378,6 +378,7 @@
454 common futex_wake sys_futex_wake
455 common futex_wait sys_futex_wait
456 common futex_requeue sys_futex_requeue
+457 common multi_clock_gettime sys_multi_clock_gettime
#
# Due to a historical design error, certain syscalls are numbered differently
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index fd9d12de7e92..b59fa776ab76 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1161,7 +1161,7 @@ asmlinkage long sys_mmap_pgoff(unsigned long addr, unsigned long len,
unsigned long prot, unsigned long flags,
unsigned long fd, unsigned long pgoff);
asmlinkage long sys_old_mmap(struct mmap_arg_struct __user *arg);
-
+asmlinkage long sys_multi_clock_gettime(struct __ptp_multi_clock_get __user * ptp_multi_clk_get);
/*
* Not a real system call, but a placeholder for syscalls which are
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 756b013fb832..beb3e0052d3c 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -828,9 +828,11 @@ __SYSCALL(__NR_futex_wake, sys_futex_wake)
__SYSCALL(__NR_futex_wait, sys_futex_wait)
#define __NR_futex_requeue 456
__SYSCALL(__NR_futex_requeue, sys_futex_requeue)
+#define __NR_multi_clock_gettime 457
+__SYSCALL(__NR_multi_clock_gettime, sys_multi_clock_gettime)
#undef __NR_syscalls
-#define __NR_syscalls 457
+#define __NR_syscalls 458
/*
* 32 bit systems traditionally used different
diff --git a/include/uapi/linux/multi_clock.h b/include/uapi/linux/multi_clock.h
new file mode 100644
index 000000000000..5e78dac3a533
--- /dev/null
+++ b/include/uapi/linux/multi_clock.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_MULTI_CLOCK_H
+#define _UAPI_MULTI_CLOCK_H
+
+#include <linux/types.h>
+#include <linux/time_types.h>
+
+#define MULTI_PTP_MAX_CLOCKS 32 /* Max number of clocks */
+#define MULTI_PTP_MAX_SAMPLES 32 /* Max allowed offset measurement samples. */
+
+struct __ptp_multi_clock_get {
+ unsigned int n_clocks; /* Desired number of clocks. */
+ unsigned int n_samples; /* Desired number of measurements per clock. */
+ clockid_t clkid_arr[MULTI_PTP_MAX_CLOCKS]; /* list of clock IDs */
+ /*
+ * Array of list of n_clocks clocks time samples n_samples times.
+ */
+ struct __kernel_timespec ts[MULTI_PTP_MAX_SAMPLES][MULTI_PTP_MAX_CLOCKS];
+};
+
+#endif /* _UAPI_MULTI_CLOCK_H */
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index b924f0f096fa..1d321dc56a25 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -31,6 +31,8 @@
#include <linux/compat.h>
#include <linux/nospec.h>
#include <linux/time_namespace.h>
+#include <linux/multi_clock.h>
+#include <linux/slab.h>
#include "timekeeping.h"
#include "posix-timers.h"
@@ -1426,6 +1428,63 @@ SYSCALL_DEFINE4(clock_nanosleep_time32, clockid_t, which_clock, int, flags,
#endif
+SYSCALL_DEFINE1(multi_clock_gettime, struct __ptp_multi_clock_get __user *, ptp_multi_clk_get)
+{
+ const struct k_clock *kc;
+ struct timespec64 *kernel_tp;
+ struct timespec64 *kernel_tp_base;
+ unsigned int n_clocks; /* Desired number of clocks. */
+ unsigned int n_samples; /* Desired number of measurements per clock. */
+ unsigned int i, j;
+ clockid_t clkid_arr[MULTI_PTP_MAX_CLOCKS]; /* list of clock IDs */
+ int error = 0;
+
+ if (copy_from_user(&n_clocks, &ptp_multi_clk_get->n_clocks, sizeof(n_clocks)))
+ return -EFAULT;
+ if (copy_from_user(&n_samples, &ptp_multi_clk_get->n_samples, sizeof(n_samples)))
+ return -EFAULT;
+ if (n_samples > MULTI_PTP_MAX_SAMPLES)
+ return -EINVAL;
+ if (n_clocks > MULTI_PTP_MAX_CLOCKS)
+ return -EINVAL;
+ if (copy_from_user(clkid_arr, &ptp_multi_clk_get->clkid_arr,
+ sizeof(clockid_t) * n_clocks))
+ return -EFAULT;
+
+ kernel_tp_base = kmalloc_array(n_clocks * n_samples,
+ sizeof(struct timespec64), GFP_KERNEL);
+ if (!kernel_tp_base)
+ return -ENOMEM;
+
+ kernel_tp = kernel_tp_base;
+ for (j = 0; j < n_samples; j++) {
+ for (i = 0; i < n_clocks; i++) {
+ kc = clockid_to_kclock(clkid_arr[i]);
+ if (!kc) {
+ error = -EINVAL;
+ goto out;
+ }
+ error = kc->clock_get_timespec(clkid_arr[i], kernel_tp++);
+ if (error)
+ goto out;
+ }
+ }
+
+ kernel_tp = kernel_tp_base;
+ for (j = 0; j < n_samples; j++) {
+ for (i = 0; i < n_clocks; i++) {
+ if (put_timespec64(kernel_tp++, (struct __kernel_timespec __user *)
+ &ptp_multi_clk_get->ts[j][i])) {
+ error = -EFAULT;
+ goto out;
+ }
+ }
+ }
+out:
+ kfree(kernel_tp_base);
+ return error;
+}
+
static const struct k_clock clock_realtime = {
.clock_getres = posix_get_hrtimer_res,
.clock_get_timespec = posix_get_realtime_timespec,
--
2.26.3
^ permalink raw reply related
* Re: [PATCH v3] posix-timers: add multi_clock_gettime system call
From: Sagi Maimon @ 2023-12-31 16:00 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Richard Cochran, Andy Lutomirski, datglx, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Geert Uytterhoeven, Peter Zijlstra, Johannes Weiner, Sohil Mehta,
Rick Edgecombe, Nhat Pham, Palmer Dabbelt, Kees Cook,
Alexey Gladkov, Mark Rutland, linux-kernel, linux-api, Linux-Arch,
Netdev
In-Reply-To: <f254c189-463e-43a3-bc09-9a8869ebf819@app.fastmail.com>
On Fri, Dec 29, 2023 at 5:27 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Dec 28, 2023, at 13:24, Sagi Maimon wrote:
> > Some user space applications need to read some clocks.
> > Each read requires moving from user space to kernel space.
> > The syscall overhead causes unpredictable delay between N clocks reads
> > Removing this delay causes better synchronization between N clocks.
> >
> > Introduce a new system call multi_clock_gettime, which can be used to measure
> > the offset between multiple clocks, from variety of types: PHC, virtual PHC
> > and various system clocks (CLOCK_REALTIME, CLOCK_MONOTONIC, etc).
> > The offset includes the total time that the driver needs to read the clock
> > timestamp.
> >
> > New system call allows the reading of a list of clocks - up to PTP_MAX_CLOCKS.
> > Supported clocks IDs: PHC, virtual PHC and various system clocks.
> > Up to PTP_MAX_SAMPLES times (per clock) in a single system call read.
> > The system call returns n_clocks timestamps for each measurement:
> > - clock 0 timestamp
> > - ...
> > - clock n timestamp
> >
> > Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
>
> Hi Sagi,
>
> Exposing an interface to read multiple clocks makes sense to me,
> but I wonder if the interface you use is too inflexible.
>
Arnd - thanks alot for your notes.
> > --- a/include/uapi/asm-generic/unistd.h
> > +++ b/include/uapi/asm-generic/unistd.h
> > @@ -828,9 +828,11 @@ __SYSCALL(__NR_futex_wake, sys_futex_wake)
> > __SYSCALL(__NR_futex_wait, sys_futex_wait)
> > #define __NR_futex_requeue 456
> > __SYSCALL(__NR_futex_requeue, sys_futex_requeue)
> > +#define __NR_multi_clock_gettime 457
> > +__SYSCALL(__NR_multi_clock_gettime, sys_multi_clock_gettime)
> >
> > #undef __NR_syscalls
> > -#define __NR_syscalls 457
> > +#define __NR_syscalls 458
>
> Site note: hooking it up only here is sufficient for the
> code review but not for inclusion: once we have an agreement
> on the API, this should be added to all architectures at once.
>
> > +#define MULTI_PTP_MAX_CLOCKS 5 /* Max number of clocks */
> > +#define MULTI_PTP_MAX_SAMPLES 10 /* Max allowed offset measurement samples. */
> > +
> > +struct __ptp_multi_clock_get {
> > + unsigned int n_clocks; /* Desired number of clocks. */
> > + unsigned int n_samples; /* Desired number of measurements per clock. */
> > + clockid_t clkid_arr[MULTI_PTP_MAX_CLOCKS]; /* list of clock IDs */
> > + /*
> > + * Array of list of n_clocks clocks time samples n_samples times.
> > + */
> > + struct __kernel_timespec ts[MULTI_PTP_MAX_SAMPLES][MULTI_PTP_MAX_CLOCKS];
> > +};
>
> The fixed size arrays here seem to be an unnecessary limitation,
> both MULTI_PTP_MAX_SAMPLES and MULTI_PTP_MAX_CLOCKS are small
> enough that one can come up with scenarios where you would want
> a higher number, but at the same time the structure is already
> 808 bytes long, which is more than you'd normally want to put
> on the kernel stack, and which may take a significant time to
> copy to and from userspace.
>
> Since n_clocks and n_samples are always inputs to the syscall,
> you can just pass them as register arguments and use a dynamically
> sized array instead.
>
Both MULTI_PTP_MAX_SAMPLES and MULTI_PTP_MAX_CLOCKS are enough of any
usage we can think of,
But I think you are right, it is better to use a dynamically sized
array for future use, plus to use less stack memory.
On patch v4 a dynamically sized array will be used .
I leaving both MULTI_PTP_MAX_SAMPLES and MULTI_PTP_MAX_CLOCKS but
increasing their values, since there should be some limitation.
> It's not clear to me what you gain from having the n_samples
> argument over just calling the syscall repeatedly. Does
> this offer a benefit for accuracy or is this just meant to
> avoid syscall overhead.
It is mainly to avoid syscall overhead which also slightly improve the accuracy.
> > +SYSCALL_DEFINE1(multi_clock_gettime, struct __ptp_multi_clock_get
> > __user *, ptp_multi_clk_get)
> > +{
> > + const struct k_clock *kc;
> > + struct timespec64 kernel_tp;
> > + struct __ptp_multi_clock_get multi_clk_get;
> > + unsigned int i, j;
> > + int error;
> > +
> > + if (copy_from_user(&multi_clk_get, ptp_multi_clk_get,
> > sizeof(multi_clk_get)))
> > + return -EFAULT;
>
> Here you copy the entire structure from userspace, but
> I don't actually see the .ts[] array on the stack being
> accessed later as you just copy to the user pointer
> directly.
>
you are right, will be fixed on patch v4.
> > + for (i = 0; i < multi_clk_get.n_clocks; i++) {
> > + kc = clockid_to_kclock(multi_clk_get.clkid_arr[i]);
> > + if (!kc)
> > + return -EINVAL;
> > + error = kc->clock_get_timespec(multi_clk_get.clkid_arr[i],
> > &kernel_tp);
> > + if (!error && put_timespec64(&kernel_tp, (struct __kernel_timespec
> > __user *)
> > + &ptp_multi_clk_get->ts[j][i]))
> > + error = -EFAULT;
> > + }
>
> The put_timespec64() and possibly the clockid_to_kclock() have
> some overhead that may introduce jitter, so it may be better to
> pull that out of the loop and have a fixed-size array
> of timespec64 values on the stack and then copy them
> at the end.
>
This overhead may introduce marginal jitter in my opinion,
still I like your idea since it is better to remove any overhead.
This will be fixed in patch v4.
I don't intend to use "a fixed-size array of timespec64 values on the
stack" since it will cause stack memory overflow,
Instead I will use a dynamically sized array (according to your
previews advice).
> On the other hand, this will still give less accuracy than the
> getcrosststamp() callback with ioctl(PTP_SYS_OFFSET_PRECISE),
> so either the last bit of accuracy isn't all that important,
> or you need to refine the interface to actually be an
> improvement over the chardev.
>
I don't understand this comment, please explain.
The ioctl(PTP_SYS_OFFSET_PRECISE) is one specific case that can be
done by multi_clock_gettime syscall (which cover many more cases)
Plus the ioctl(PTP_SYS_OFFSET_PRECISE) works only on drivers that
support this feature.
> Arnd
^ permalink raw reply
* Re: [PATCH v3] posix-timers: add multi_clock_gettime system call
From: Arnd Bergmann @ 2023-12-29 15:26 UTC (permalink / raw)
To: Sagi Maimon, Richard Cochran, Andy Lutomirski, datglx,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Geert Uytterhoeven, Peter Zijlstra, Johannes Weiner, Sohil Mehta,
Rick Edgecombe, Nhat Pham, Palmer Dabbelt, Kees Cook,
Alexey Gladkov, Mark Rutland
Cc: linux-kernel, linux-api, Linux-Arch, Netdev
In-Reply-To: <20231228122411.3189-1-maimon.sagi@gmail.com>
On Thu, Dec 28, 2023, at 13:24, Sagi Maimon wrote:
> Some user space applications need to read some clocks.
> Each read requires moving from user space to kernel space.
> The syscall overhead causes unpredictable delay between N clocks reads
> Removing this delay causes better synchronization between N clocks.
>
> Introduce a new system call multi_clock_gettime, which can be used to measure
> the offset between multiple clocks, from variety of types: PHC, virtual PHC
> and various system clocks (CLOCK_REALTIME, CLOCK_MONOTONIC, etc).
> The offset includes the total time that the driver needs to read the clock
> timestamp.
>
> New system call allows the reading of a list of clocks - up to PTP_MAX_CLOCKS.
> Supported clocks IDs: PHC, virtual PHC and various system clocks.
> Up to PTP_MAX_SAMPLES times (per clock) in a single system call read.
> The system call returns n_clocks timestamps for each measurement:
> - clock 0 timestamp
> - ...
> - clock n timestamp
>
> Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
Hi Sagi,
Exposing an interface to read multiple clocks makes sense to me,
but I wonder if the interface you use is too inflexible.
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -828,9 +828,11 @@ __SYSCALL(__NR_futex_wake, sys_futex_wake)
> __SYSCALL(__NR_futex_wait, sys_futex_wait)
> #define __NR_futex_requeue 456
> __SYSCALL(__NR_futex_requeue, sys_futex_requeue)
> +#define __NR_multi_clock_gettime 457
> +__SYSCALL(__NR_multi_clock_gettime, sys_multi_clock_gettime)
>
> #undef __NR_syscalls
> -#define __NR_syscalls 457
> +#define __NR_syscalls 458
Site note: hooking it up only here is sufficient for the
code review but not for inclusion: once we have an agreement
on the API, this should be added to all architectures at once.
> +#define MULTI_PTP_MAX_CLOCKS 5 /* Max number of clocks */
> +#define MULTI_PTP_MAX_SAMPLES 10 /* Max allowed offset measurement samples. */
> +
> +struct __ptp_multi_clock_get {
> + unsigned int n_clocks; /* Desired number of clocks. */
> + unsigned int n_samples; /* Desired number of measurements per clock. */
> + clockid_t clkid_arr[MULTI_PTP_MAX_CLOCKS]; /* list of clock IDs */
> + /*
> + * Array of list of n_clocks clocks time samples n_samples times.
> + */
> + struct __kernel_timespec ts[MULTI_PTP_MAX_SAMPLES][MULTI_PTP_MAX_CLOCKS];
> +};
The fixed size arrays here seem to be an unnecessary limitation,
both MULTI_PTP_MAX_SAMPLES and MULTI_PTP_MAX_CLOCKS are small
enough that one can come up with scenarios where you would want
a higher number, but at the same time the structure is already
808 bytes long, which is more than you'd normally want to put
on the kernel stack, and which may take a significant time to
copy to and from userspace.
Since n_clocks and n_samples are always inputs to the syscall,
you can just pass them as register arguments and use a dynamically
sized array instead.
It's not clear to me what you gain from having the n_samples
argument over just calling the syscall repeatedly. Does
this offer a benefit for accuracy or is this just meant to
avoid syscall overhead.
> +SYSCALL_DEFINE1(multi_clock_gettime, struct __ptp_multi_clock_get
> __user *, ptp_multi_clk_get)
> +{
> + const struct k_clock *kc;
> + struct timespec64 kernel_tp;
> + struct __ptp_multi_clock_get multi_clk_get;
> + unsigned int i, j;
> + int error;
> +
> + if (copy_from_user(&multi_clk_get, ptp_multi_clk_get,
> sizeof(multi_clk_get)))
> + return -EFAULT;
Here you copy the entire structure from userspace, but
I don't actually see the .ts[] array on the stack being
accessed later as you just copy to the user pointer
directly.
> + for (i = 0; i < multi_clk_get.n_clocks; i++) {
> + kc = clockid_to_kclock(multi_clk_get.clkid_arr[i]);
> + if (!kc)
> + return -EINVAL;
> + error = kc->clock_get_timespec(multi_clk_get.clkid_arr[i],
> &kernel_tp);
> + if (!error && put_timespec64(&kernel_tp, (struct __kernel_timespec
> __user *)
> + &ptp_multi_clk_get->ts[j][i]))
> + error = -EFAULT;
> + }
The put_timespec64() and possibly the clockid_to_kclock() have
some overhead that may introduce jitter, so it may be better to
pull that out of the loop and have a fixed-size array
of timespec64 values on the stack and then copy them
at the end.
On the other hand, this will still give less accuracy than the
getcrosststamp() callback with ioctl(PTP_SYS_OFFSET_PRECISE),
so either the last bit of accuracy isn't all that important,
or you need to refine the interface to actually be an
improvement over the chardev.
Arnd
^ permalink raw reply
* Re: [PATCH v3] posix-timers: add multi_clock_gettime system call
From: kernel test robot @ 2023-12-29 4:03 UTC (permalink / raw)
To: Sagi Maimon, richardcochran, luto, datglx, mingo, bp, dave.hansen,
x86, hpa, arnd, geert, peterz, hannes, sohil.mehta,
rick.p.edgecombe, nphamcs, palmer, keescook, legion, mark.rutland
Cc: llvm, oe-kbuild-all, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <20231228122411.3189-1-maimon.sagi@gmail.com>
Hi Sagi,
kernel test robot noticed the following build errors:
[auto build test ERROR on tip/x86/asm]
[also build test ERROR on arnd-asm-generic/master tip/timers/core linus/master v6.7-rc7]
[cannot apply to next-20231222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sagi-Maimon/posix-timers-add-multi_clock_gettime-system-call/20231228-202632
base: tip/x86/asm
patch link: https://lore.kernel.org/r/20231228122411.3189-1-maimon.sagi%40gmail.com
patch subject: [PATCH v3] posix-timers: add multi_clock_gettime system call
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20231229/202312291154.hCJdKLKM-lkp@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project 8a4266a626914765c0c69839e8a51be383013c1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231229/202312291154.hCJdKLKM-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312291154.hCJdKLKM-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from kernel/time/time.c:33:
>> include/linux/syscalls.h:1164:48: warning: declaration of 'struct __ptp_multi_clock_get' will not be visible outside of this function [-Wvisibility]
1164 | asmlinkage long sys_multi_clock_gettime(struct __ptp_multi_clock_get __user * ptp_multi_clk_get);
| ^
1 warning generated.
--
In file included from kernel/time/hrtimer.c:30:
>> include/linux/syscalls.h:1164:48: warning: declaration of 'struct __ptp_multi_clock_get' will not be visible outside of this function [-Wvisibility]
1164 | asmlinkage long sys_multi_clock_gettime(struct __ptp_multi_clock_get __user * ptp_multi_clk_get);
| ^
kernel/time/hrtimer.c:147:20: warning: unused function 'is_migration_base' [-Wunused-function]
147 | static inline bool is_migration_base(struct hrtimer_clock_base *base)
| ^~~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:1876:20: warning: unused function '__hrtimer_peek_ahead_timers' [-Wunused-function]
1876 | static inline void __hrtimer_peek_ahead_timers(void)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.
--
In file included from kernel/time/posix-timers.c:26:
>> include/linux/syscalls.h:1164:48: warning: declaration of 'struct __ptp_multi_clock_get' will not be visible outside of this function [-Wvisibility]
1164 | asmlinkage long sys_multi_clock_gettime(struct __ptp_multi_clock_get __user * ptp_multi_clk_get);
| ^
>> kernel/time/posix-timers.c:1430:1: error: conflicting types for 'sys_multi_clock_gettime'
1430 | SYSCALL_DEFINE1(multi_clock_gettime, struct __ptp_multi_clock_get __user *, ptp_multi_clk_get)
| ^
include/linux/syscalls.h:219:36: note: expanded from macro 'SYSCALL_DEFINE1'
219 | #define SYSCALL_DEFINE1(name, ...) SYSCALL_DEFINEx(1, _##name, __VA_ARGS__)
| ^
include/linux/syscalls.h:230:2: note: expanded from macro 'SYSCALL_DEFINEx'
230 | __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
| ^
include/linux/syscalls.h:244:18: note: expanded from macro '__SYSCALL_DEFINEx'
244 | asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
| ^
<scratch space>:135:1: note: expanded from here
135 | sys_multi_clock_gettime
| ^
include/linux/syscalls.h:1164:17: note: previous declaration is here
1164 | asmlinkage long sys_multi_clock_gettime(struct __ptp_multi_clock_get __user * ptp_multi_clk_get);
| ^
1 warning and 1 error generated.
vim +/sys_multi_clock_gettime +1430 kernel/time/posix-timers.c
1429
> 1430 SYSCALL_DEFINE1(multi_clock_gettime, struct __ptp_multi_clock_get __user *, ptp_multi_clk_get)
1431 {
1432 const struct k_clock *kc;
1433 struct timespec64 kernel_tp;
1434 struct __ptp_multi_clock_get multi_clk_get;
1435 unsigned int i, j;
1436 int error;
1437
1438 if (copy_from_user(&multi_clk_get, ptp_multi_clk_get, sizeof(multi_clk_get)))
1439 return -EFAULT;
1440
1441 if (multi_clk_get.n_samples > MULTI_PTP_MAX_SAMPLES)
1442 return -EINVAL;
1443 if (multi_clk_get.n_clocks > MULTI_PTP_MAX_CLOCKS)
1444 return -EINVAL;
1445
1446 for (j = 0; j < multi_clk_get.n_samples; j++) {
1447 for (i = 0; i < multi_clk_get.n_clocks; i++) {
1448 kc = clockid_to_kclock(multi_clk_get.clkid_arr[i]);
1449 if (!kc)
1450 return -EINVAL;
1451 error = kc->clock_get_timespec(multi_clk_get.clkid_arr[i], &kernel_tp);
1452 if (!error && put_timespec64(&kernel_tp, (struct __kernel_timespec __user *)
1453 &ptp_multi_clk_get->ts[j][i]))
1454 error = -EFAULT;
1455 }
1456 }
1457
1458 return error;
1459 }
1460
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v3] posix-timers: add multi_clock_gettime system call
From: kernel test robot @ 2023-12-29 3:21 UTC (permalink / raw)
To: Sagi Maimon, richardcochran, luto, datglx, mingo, bp, dave.hansen,
x86, hpa, arnd, geert, peterz, hannes, sohil.mehta,
rick.p.edgecombe, nphamcs, palmer, keescook, legion, mark.rutland
Cc: llvm, oe-kbuild-all, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <20231228122411.3189-1-maimon.sagi@gmail.com>
Hi Sagi,
kernel test robot noticed the following build errors:
[auto build test ERROR on tip/x86/asm]
[also build test ERROR on arnd-asm-generic/master tip/timers/core linus/master v6.7-rc7]
[cannot apply to next-20231222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sagi-Maimon/posix-timers-add-multi_clock_gettime-system-call/20231228-202632
base: tip/x86/asm
patch link: https://lore.kernel.org/r/20231228122411.3189-1-maimon.sagi%40gmail.com
patch subject: [PATCH v3] posix-timers: add multi_clock_gettime system call
config: x86_64-buildonly-randconfig-002-20231229 (https://download.01.org/0day-ci/archive/20231229/202312291148.zfhZ9vDJ-lkp@intel.com/config)
compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231229/202312291148.zfhZ9vDJ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312291148.zfhZ9vDJ-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from <built-in>:1:
>> ./usr/include/linux/multi_clock.h:14:2: error: unknown type name 'clockid_t'
14 | clockid_t clkid_arr[MULTI_PTP_MAX_CLOCKS]; /* list of clock IDs */
| ^
1 error generated.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* [PATCH v3] posix-timers: add multi_clock_gettime system call
From: Sagi Maimon @ 2023-12-28 12:24 UTC (permalink / raw)
To: richardcochran, luto, datglx, mingo, bp, dave.hansen, x86, hpa,
arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, maimon.sagi, keescook, legion, mark.rutland
Cc: linux-kernel, linux-api, linux-arch, netdev
Some user space applications need to read some clocks.
Each read requires moving from user space to kernel space.
The syscall overhead causes unpredictable delay between N clocks reads
Removing this delay causes better synchronization between N clocks.
Introduce a new system call multi_clock_gettime, which can be used to measure
the offset between multiple clocks, from variety of types: PHC, virtual PHC
and various system clocks (CLOCK_REALTIME, CLOCK_MONOTONIC, etc).
The offset includes the total time that the driver needs to read the clock
timestamp.
New system call allows the reading of a list of clocks - up to PTP_MAX_CLOCKS.
Supported clocks IDs: PHC, virtual PHC and various system clocks.
Up to PTP_MAX_SAMPLES times (per clock) in a single system call read.
The system call returns n_clocks timestamps for each measurement:
- clock 0 timestamp
- ...
- clock n timestamp
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
Addressed comments from:
- Thomas Gleixner : https://www.spinics.net/lists/netdev/msg959514.html
Changes since version 2:
- Change multi PHC syscall to fit the Y2038 safe variant.
- Define the syscall data structure under uapi directory, so it will be known in user space.
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 2 +-
include/uapi/asm-generic/unistd.h | 4 +++-
include/uapi/linux/multi_clock.h | 21 ++++++++++++++++
kernel/time/posix-timers.c | 33 ++++++++++++++++++++++++++
5 files changed, 59 insertions(+), 2 deletions(-)
create mode 100644 include/uapi/linux/multi_clock.h
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 8cb8bf68721c..9cdeb0bf49db 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -378,6 +378,7 @@
454 common futex_wake sys_futex_wake
455 common futex_wait sys_futex_wait
456 common futex_requeue sys_futex_requeue
+457 common multi_clock_gettime sys_multi_clock_gettime
#
# Due to a historical design error, certain syscalls are numbered differently
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index fd9d12de7e92..b59fa776ab76 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1161,7 +1161,7 @@ asmlinkage long sys_mmap_pgoff(unsigned long addr, unsigned long len,
unsigned long prot, unsigned long flags,
unsigned long fd, unsigned long pgoff);
asmlinkage long sys_old_mmap(struct mmap_arg_struct __user *arg);
-
+asmlinkage long sys_multi_clock_gettime(struct __ptp_multi_clock_get __user * ptp_multi_clk_get);
/*
* Not a real system call, but a placeholder for syscalls which are
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 756b013fb832..beb3e0052d3c 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -828,9 +828,11 @@ __SYSCALL(__NR_futex_wake, sys_futex_wake)
__SYSCALL(__NR_futex_wait, sys_futex_wait)
#define __NR_futex_requeue 456
__SYSCALL(__NR_futex_requeue, sys_futex_requeue)
+#define __NR_multi_clock_gettime 457
+__SYSCALL(__NR_multi_clock_gettime, sys_multi_clock_gettime)
#undef __NR_syscalls
-#define __NR_syscalls 457
+#define __NR_syscalls 458
/*
* 32 bit systems traditionally used different
diff --git a/include/uapi/linux/multi_clock.h b/include/uapi/linux/multi_clock.h
new file mode 100644
index 000000000000..07099045ec32
--- /dev/null
+++ b/include/uapi/linux/multi_clock.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_MULTI_CLOCK_H
+#define _UAPI_MULTI_CLOCK_H
+
+#include <linux/types.h>
+#include <linux/time_types.h>
+
+#define MULTI_PTP_MAX_CLOCKS 5 /* Max number of clocks */
+#define MULTI_PTP_MAX_SAMPLES 10 /* Max allowed offset measurement samples. */
+
+struct __ptp_multi_clock_get {
+ unsigned int n_clocks; /* Desired number of clocks. */
+ unsigned int n_samples; /* Desired number of measurements per clock. */
+ clockid_t clkid_arr[MULTI_PTP_MAX_CLOCKS]; /* list of clock IDs */
+ /*
+ * Array of list of n_clocks clocks time samples n_samples times.
+ */
+ struct __kernel_timespec ts[MULTI_PTP_MAX_SAMPLES][MULTI_PTP_MAX_CLOCKS];
+};
+
+#endif /* _UAPI_MULTI_CLOCK_H */
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index b924f0f096fa..80cbce59d4f4 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -31,6 +31,7 @@
#include <linux/compat.h>
#include <linux/nospec.h>
#include <linux/time_namespace.h>
+#include <linux/multi_clock.h>
#include "timekeeping.h"
#include "posix-timers.h"
@@ -1426,6 +1427,38 @@ SYSCALL_DEFINE4(clock_nanosleep_time32, clockid_t, which_clock, int, flags,
#endif
+SYSCALL_DEFINE1(multi_clock_gettime, struct __ptp_multi_clock_get __user *, ptp_multi_clk_get)
+{
+ const struct k_clock *kc;
+ struct timespec64 kernel_tp;
+ struct __ptp_multi_clock_get multi_clk_get;
+ unsigned int i, j;
+ int error;
+
+ if (copy_from_user(&multi_clk_get, ptp_multi_clk_get, sizeof(multi_clk_get)))
+ return -EFAULT;
+
+ if (multi_clk_get.n_samples > MULTI_PTP_MAX_SAMPLES)
+ return -EINVAL;
+ if (multi_clk_get.n_clocks > MULTI_PTP_MAX_CLOCKS)
+ return -EINVAL;
+
+ for (j = 0; j < multi_clk_get.n_samples; j++) {
+ for (i = 0; i < multi_clk_get.n_clocks; i++) {
+ kc = clockid_to_kclock(multi_clk_get.clkid_arr[i]);
+ if (!kc)
+ return -EINVAL;
+ error = kc->clock_get_timespec(multi_clk_get.clkid_arr[i], &kernel_tp);
+ if (!error && put_timespec64(&kernel_tp, (struct __kernel_timespec __user *)
+ &ptp_multi_clk_get->ts[j][i]))
+ error = -EFAULT;
+ }
+ }
+
+ return error;
+}
+
+
static const struct k_clock clock_realtime = {
.clock_getres = posix_get_hrtimer_res,
.clock_get_timespec = posix_get_realtime_timespec,
--
2.26.3
^ permalink raw reply related
* Re: [PATCH v5 03/11] mm/mempolicy: refactor sanitize_mpol_flags for reuse
From: Gregory Price @ 2023-12-26 11:48 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-doc, linux-fsdevel, linux-kernel,
linux-api, x86, akpm, arnd, tglx, luto, mingo, bp, dave.hansen,
hpa, mhocko, tj, corbet, rakie.kim, hyeongtak.ji, honggyu.kim,
vtavarespetr, peterz, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha
In-Reply-To: <ZYp7P1fH8nvkr4o0@memverge.com>
On Tue, Dec 26, 2023 at 02:05:35AM -0500, Gregory Price wrote:
> On Wed, Dec 27, 2023 at 04:39:29PM +0800, Huang, Ying wrote:
> > Gregory Price <gourry.memverge@gmail.com> writes:
> >
> > > + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS);
> > > +
> > > + *flags = *mode_arg & MPOL_MODE_FLAGS;
> > > + *mode_arg = mode;
> >
> > It appears that it's unnecessary to introduce a local variable to split
> > mode/flags. Just reuse the original code?
> >
Revisiting during fixes: Note the change from int to short.
I chose to make this explicit because validate_mpol_flags takes a short.
I'm fairly sure changing it back throws a truncation warning.
~Gregroy
^ permalink raw reply
* Re: [PATCH v5 02/11] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Gregory Price @ 2023-12-26 11:32 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-doc, linux-fsdevel, linux-kernel,
linux-api, x86, akpm, arnd, tglx, luto, mingo, bp, dave.hansen,
hpa, mhocko, tj, corbet, rakie.kim, hyeongtak.ji, honggyu.kim,
vtavarespetr, peterz, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, Srinivasulu Thanneeru
In-Reply-To: <ZYp6ZRLZQVtTHest@memverge.com>
On Tue, Dec 26, 2023 at 02:01:57AM -0500, Gregory Price wrote:
> On Wed, Dec 27, 2023 at 04:32:37PM +0800, Huang, Ying wrote:
> > Gregory Price <gourry.memverge@gmail.com> writes:
>
> Barrier is to stabilize nodemask on the stack, but yes i'll carry the
> comment from interleave_nid into this barrier as well.
>
> > > +
> > > + /* first ensure we have a valid nodemask */
> > > + nid = first_node(nodemask);
> > > + if (nid == MAX_NUMNODES)
> > > + return nid;
> >
> > It appears that this isn't necessary, because we can check whether
> > weight_total == 0 after the next loop.
> >
>
> fair, will snip.
>
Follow up - this is only possible if the nodemask is invalid / has no
nodes, so it's better to check for this explicitly. If nodemask is
valid, then it's not possible to have a weight_total of 0, because
weights cannot be 0.
~Gregory
^ permalink raw reply
* Re: [PATCH v5 02/11] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Gregory Price @ 2023-12-26 8:06 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-doc, linux-fsdevel, linux-kernel,
linux-api, x86, akpm, arnd, tglx, luto, mingo, bp, dave.hansen,
hpa, mhocko, tj, corbet, rakie.kim, hyeongtak.ji, honggyu.kim,
vtavarespetr, peterz, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, Srinivasulu Thanneeru
In-Reply-To: <ZYp6ZRLZQVtTHest@memverge.com>
On Tue, Dec 26, 2023 at 02:01:57AM -0500, Gregory Price wrote:
> >
> > If pol->wil.cur_weight == 0, prev_node will be used without being
> > initialized below.
> >
>
> pol->wil.cur_weight is not used below.
>
disregard, i misread your comment. prev_node should be initialized, to
NO_NUMA_NODE. Will fix.
~Gregory
^ permalink raw reply
* Re: [PATCH v5 00/11] mempolicy2, mbind2, and weighted interleave
From: Gregory Price @ 2023-12-26 7:45 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-doc, linux-fsdevel, linux-kernel,
linux-api, x86, akpm, arnd, tglx, luto, mingo, bp, dave.hansen,
hpa, mhocko, tj, corbet, rakie.kim, hyeongtak.ji, honggyu.kim,
vtavarespetr, peterz, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, Johannes Weiner,
Hasan Al Maruf, Hao Wang, Dan Williams, Michal Hocko, Zhongkun He,
Frank van der Linden, John Groves, Jonathan Cameron
In-Reply-To: <87frzqg1jp.fsf@yhuang6-desk2.ccr.corp.intel.com>
On Mon, Dec 25, 2023 at 03:54:18PM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
>
> > For example, the stream benchmark demonstrates that default interleave
> > is actively harmful, where weighted interleave is beneficial.
> >
> > Hardware: 1-socket 8 channel DDR5 + 1 CXL expander in PCIe x16
> > Default interleave : -78% (slower than DRAM)
> > Global weighting : -6% to +4% (workload dependant)
> > Targeted weights : +2.5% to +4% (consistently better than DRAM)
> >
> > If nothing else, this shows how awful round-robin interleave is.
>
> I guess the performance of the default policy, local (fast memory)
> first, may be even better in some situation? For example, before the
> bandwidth of DRAM is saturated?
>
Yes - but it's more complicated than that.
Global weighting here means we did `numactl -w --interleave ...`, which
means *all* memory regions will be interleaved. Code, stack, heap, etc.
Targeted weights means we used mbind2() with local weights, which only
targted specific heap regions.
The default policy was better than global weighting likely as a result
of things like stack/code being distributed to higher latency memory
produced a measurable overhead.
To provide this, we only applied weights to bandwidth driving regions,
and as a result we demonstrated a measurable performance increase.
So yes, the defautl policy may be better in some situations - but that
will be true of any policy.
> I understand that you may want to limit the memory usage of the fast
> memory too. But IMHO, that is another requirements. That should be
> enforced by something like per-node memory limit.
>
This interface does not limit memory usage of a particular node, it
distributes data according to the requested policy.
Nuanced distinction, but important. If nodes become exhausted, tasks
are still free to allocate memory from any node in the nodemask, even if
it violates the requested mempolicy.
This is consistent with the existing behavior of mempolicy.
> > =====================================================================
> > (Patches 3-6) Refactoring mempolicy for code-reuse
> >
> > To avoid multiple paths of mempolicy creation, we should refactor the
> > existing code to enable the designed extensibility, and refactor
> > existing users to utilize the new interface (while retaining the
> > existing userland interface).
> >
> > This set of patches introduces a new mempolicy_args structure, which
> > is used to more fully describe a requested mempolicy - to include
> > existing and future extensions.
> >
> > /*
> > * Describes settings of a mempolicy during set/get syscalls and
> > * kernel internal calls to do_set_mempolicy()
> > */
> > 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 */
> > };
>
> According to
>
> https://www.geeksforgeeks.org/difference-between-argument-and-parameter-in-c-c-with-examples/
>
> it appears that "parameter" are better than "argument" for struct name
> here. It appears that current kernel source supports this too.
>
> $ grep 'struct[\t ]\+[a-zA-Z0-9]\+_param' -r include/linux | wc -l
> 411
> $ grep 'struct[\t ]\+[a-zA-Z0-9]\+_arg' -r include/linux | wc -l
> 25
>
Will change.
> > This arg structure will eventually be utilized by the following
> > interfaces:
> > mpol_new() - new mempolicy creation
> > do_get_mempolicy() - acquiring information about mempolicy
> > do_set_mempolicy() - setting the task mempolicy
> > do_mbind() - setting a vma mempolicy
> >
> > do_get_mempolicy() is completely refactored to break it out into
> > separate functionality based on the flags provided by get_mempolicy(2)
> > MPOL_F_MEMS_ALLOWED: acquires task->mems_allowed
> > MPOL_F_ADDR: acquires information on vma policies
> > MPOL_F_NODE: changes the output for the policy arg to node info
> >
> > We refactor the get_mempolicy syscall flatten the logic based on these
> > flags, and aloow for set_mempolicy2() to re-use the underlying logic.
> >
> > The result of this refactor, and the new mempolicy_args structure, is
> > that extensions like 'sys_set_mempolicy_home_node' can now be directly
> > integrated into the initial call to 'set_mempolicy2', and that more
> > complete information about a mempolicy can be returned with a single
> > call to 'get_mempolicy2', rather than multiple calls to 'get_mempolicy'
> >
> >
> > =====================================================================
> > (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;
> > __u64 pol_maxnodes;
>
> I understand that we want to avoid hole in struct. But I still feel
> uncomfortable to use __u64 for a small. But I don't have solution too.
> Anyone else has some idea?
>
maxnode has been an `unsigned long` in every other interface for quite
some time. Seems better to keep this consistent rather than it suddenly
become `unsigned long` over here and `unsigned short` over there.
> > __aligned_u64 pol_nodes;
> > __aligned_u64 *il_weights; /* of size pol_maxnodes */
>
> Typo? Should be,
>
derp derp
> >
> > The 'flags' argument for mbind2 is the same as 'mbind', except with
> > the addition of MPOL_MF_HOME_NODE to denote whether the 'home_node'
> > field should be utilized.
> >
> > The 'flags' argument for get_mempolicy2 allows for MPOL_F_ADDR to
> > allow operating on VMA policies, but MPOL_F_NODE and MPOL_F_MEMS_ALLOWED
> > behavior has been omitted, since get_mempolicy() provides this already.
>
> I still think that it's a good idea to make it possible to deprecate
> get_mempolicy(). How about use a union as follows?
>
> struct mpol_mems_allowed {
> __u64 maxnodes;
> __aligned_u64 nodemask;
> };
>
> union mpol_info {
> struct mpol_args args;
> struct mpol_mems_allowed mems_allowed;
> __s32 node;
> };
>
See my other email. I've come around to see mems_allowed as a wart that
needs to be removed. The same information is already available via
sysfs cpusets.mems and cpusets.mems_effective.
Additionally, mems_allowed isn't even technically part of the mempolicy,
so if we did want an interface to acquire the infomation, you'd prefer
to just implement a stand-alone syscall.
The sysfs interface seems sufficient though.
`policy_node` is a similar "why does this even exist" type feature,
except that it can still be used from get_mempolicy() and if there is an
actual reason to extend it to get_mempolicy2() it can be added to
mpol_params.
~Gregory
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox