[PATCH v3 0/2] add epoll_pwait2 syscall

[PATCH v3 0/2] add epoll_pwait2 syscall

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Enable nanosecond timeouts for epoll.

Analogous to pselect and ppoll, introduce an epoll_wait syscall
variant that takes a struct timespec instead of int timeout.

See patch 1 for more details.

patch 1: new syscall
patch 2: selftest

Willem de Bruijn (2):
  epoll: add nsec timeout support with epoll_pwait2
  selftests/filesystems: expand epoll with epoll_pwait2

 arch/alpha/kernel/syscalls/syscall.tbl        |   1 +
 arch/arm/tools/syscall.tbl                    |   1 +
 arch/arm64/include/asm/unistd.h               |   2 +-
 arch/arm64/include/asm/unistd32.h             |   2 +
 arch/ia64/kernel/syscalls/syscall.tbl         |   1 +
 arch/m68k/kernel/syscalls/syscall.tbl         |   1 +
 arch/microblaze/kernel/syscalls/syscall.tbl   |   1 +
 arch/mips/kernel/syscalls/syscall_n32.tbl     |   1 +
 arch/mips/kernel/syscalls/syscall_n64.tbl     |   1 +
 arch/mips/kernel/syscalls/syscall_o32.tbl     |   1 +
 arch/parisc/kernel/syscalls/syscall.tbl       |   1 +
 arch/powerpc/kernel/syscalls/syscall.tbl      |   1 +
 arch/s390/kernel/syscalls/syscall.tbl         |   1 +
 arch/sh/kernel/syscalls/syscall.tbl           |   1 +
 arch/sparc/kernel/syscalls/syscall.tbl        |   1 +
 arch/x86/entry/syscalls/syscall_32.tbl        |   1 +
 arch/x86/entry/syscalls/syscall_64.tbl        |   1 +
 arch/xtensa/kernel/syscalls/syscall.tbl       |   1 +
 fs/eventpoll.c                                | 106 +++++++++++++++---
 include/linux/compat.h                        |   6 +
 include/linux/syscalls.h                      |   5 +
 include/uapi/asm-generic/unistd.h             |   4 +-
 kernel/sys_ni.c                               |   2 +
 .../filesystems/epoll/epoll_wakeup_test.c     |  70 ++++++++++++
 24 files changed, 193 insertions(+), 20 deletions(-)

-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Add syscall epoll_pwait2, an epoll_wait variant with nsec resolution
that replaces int timeout with struct timespec. It is equivalent
otherwise.

    int epoll_pwait2(int fd, struct epoll_event *events,
                     int maxevents,
                     const struct timespec *timeout,
                     const sigset_t *sigset);

The underlying hrtimer is already programmed with nsec resolution.
pselect and ppoll also set nsec resolution timeout with timespec.

The sigset_t in epoll_pwait has a compat variant. epoll_pwait2 needs
the same.

For timespec, only support this new interface on 2038 aware platforms
that define __kernel_timespec_t. So no CONFIG_COMPAT_32BIT_TIME.

Changes
  v3:
  - rewrite: add epoll_pwait2 syscall instead of epoll_create1 flag
  v2:
  - cast to s64: avoid overflow on 32-bit platforms (Shuo Chen)
  - minor commit message rewording

Signed-off-by: Willem de Bruijn <willemb@google.com>

---

This version applies cleanly to linux-next-20201117.
---
 arch/alpha/kernel/syscalls/syscall.tbl      |   1 +
 arch/arm/tools/syscall.tbl                  |   1 +
 arch/arm64/include/asm/unistd.h             |   2 +-
 arch/arm64/include/asm/unistd32.h           |   2 +
 arch/ia64/kernel/syscalls/syscall.tbl       |   1 +
 arch/m68k/kernel/syscalls/syscall.tbl       |   1 +
 arch/microblaze/kernel/syscalls/syscall.tbl |   1 +
 arch/mips/kernel/syscalls/syscall_n32.tbl   |   1 +
 arch/mips/kernel/syscalls/syscall_n64.tbl   |   1 +
 arch/mips/kernel/syscalls/syscall_o32.tbl   |   1 +
 arch/parisc/kernel/syscalls/syscall.tbl     |   1 +
 arch/powerpc/kernel/syscalls/syscall.tbl    |   1 +
 arch/s390/kernel/syscalls/syscall.tbl       |   1 +
 arch/sh/kernel/syscalls/syscall.tbl         |   1 +
 arch/sparc/kernel/syscalls/syscall.tbl      |   1 +
 arch/x86/entry/syscalls/syscall_32.tbl      |   1 +
 arch/x86/entry/syscalls/syscall_64.tbl      |   1 +
 arch/xtensa/kernel/syscalls/syscall.tbl     |   1 +
 fs/eventpoll.c                              | 106 ++++++++++++++++----
 include/linux/compat.h                      |   6 ++
 include/linux/syscalls.h                    |   5 +
 include/uapi/asm-generic/unistd.h           |   4 +-
 kernel/sys_ni.c                             |   2 +
 23 files changed, 123 insertions(+), 20 deletions(-)

diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index c5cc5bfa2062..e0a599bfb0e1 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -481,3 +481,4 @@
 549	common	faccessat2			sys_faccessat2
 550	common	process_madvise			sys_process_madvise
 551	common	watch_mount			sys_watch_mount
+552	common	epoll_pwait2			sys_epoll_pwait2
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index 47325b3b661a..dbde88a855b6 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -455,3 +455,4 @@
 439	common	faccessat2			sys_faccessat2
 440	common	process_madvise			sys_process_madvise
 441	common	watch_mount			sys_watch_mount
+443	common	epoll_pwait2			sys_epoll_pwait2
diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index 949788f5ba40..d1f7d35f986e 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -38,7 +38,7 @@
 #define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE + 5)
 #define __ARM_NR_COMPAT_END		(__ARM_NR_COMPAT_BASE + 0x800)
 
-#define __NR_compat_syscalls		443
+#define __NR_compat_syscalls		444
 #endif
 
 #define __ARCH_WANT_SYS_CLONE
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
index c71c3fe0b6cd..b84e24a7e2c0 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -893,6 +893,8 @@ __SYSCALL(__NR_process_madvise, sys_process_madvise)
 __SYSCALL(__NR_watch_mount, sys_watch_mount)
 #define __NR_memfd_secret 442
 __SYSCALL(__NR_memfd_secret, sys_memfd_secret)
+#define __NR_epoll_pwait2 443
+__SYSCALL(__NR_epoll_pwait2, sys_epoll_pwait2)
 
 /*
  * Please add new compat syscalls above this comment and update
diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
index 033244462350..c8809959636f 100644
--- a/arch/ia64/kernel/syscalls/syscall.tbl
+++ b/arch/ia64/kernel/syscalls/syscall.tbl
@@ -362,3 +362,4 @@
 439	common	faccessat2			sys_faccessat2
 440	common	process_madvise			sys_process_madvise
 441	common	watch_mount			sys_watch_mount
+443	common	epoll_pwait2			sys_epoll_pwait2
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index efd3ecb3cdfc..dde585616bf8 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -441,3 +441,4 @@
 439	common	faccessat2			sys_faccessat2
 440	common	process_madvise			sys_process_madvise
 441	common	watch_mount			sys_watch_mount
+443	common	epoll_pwait2			sys_epoll_pwait2
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 67ae5a5e4d21..4c09f27fedd0 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -447,3 +447,4 @@
 439	common	faccessat2			sys_faccessat2
 440	common	process_madvise			sys_process_madvise
 441	common	watch_mount			sys_watch_mount
+443	common	epoll_pwait2			sys_epoll_pwait2
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index c59bc6acc47a..00921244242d 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -380,3 +380,4 @@
 439	n32	faccessat2			sys_faccessat2
 440	n32	process_madvise			sys_process_madvise
 441	n32	watch_mount			sys_watch_mount
+443	n32	epoll_pwait2			sys_epoll_pwait2
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index 50bbb9517052..b7920f76358d 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -356,3 +356,4 @@
 439	n64	faccessat2			sys_faccessat2
 440	n64	process_madvise			sys_process_madvise
 441	n64	watch_mount			sys_watch_mount
+443	n64	epoll_pwait2			sys_epoll_pwait2
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index 1d5644e221b4..2ab5ed500113 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -429,3 +429,4 @@
 439	o32	faccessat2			sys_faccessat2
 440	o32	process_madvise			sys_process_madvise
 441	o32	watch_mount			sys_watch_mount
+443	o32	epoll_pwait2			sys_epoll_pwait2		compat_sys_epoll_pwait2
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index 4492cc2fce23..9caf7d969846 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -439,3 +439,4 @@
 439	common	faccessat2			sys_faccessat2
 440	common	process_madvise			sys_process_madvise
 441	common	watch_mount			sys_watch_mount
+443	common	epoll_pwait2			sys_epoll_pwait2		compat_sys_epoll_pwait2
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index ee122bfc2ddc..dce635420226 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -531,3 +531,4 @@
 439	common	faccessat2			sys_faccessat2
 440	common	process_madvise			sys_process_madvise
 441	common	watch_mount			sys_watch_mount
+443	common	epoll_pwait2			sys_epoll_pwait2		compat_sys_epoll_pwait2
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index b467b57d66d1..dedfa6db9f8d 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -444,3 +444,4 @@
 439  common	faccessat2		sys_faccessat2			sys_faccessat2
 440  common	process_madvise		sys_process_madvise		sys_process_madvise
 441	common	watch_mount		sys_watch_mount			sys_watch_mount
+443  common	epoll_pwait2		sys_epoll_pwait2		sys_epoll_pwait2
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index f9c7ca5cb969..f7a383ae2c48 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -444,3 +444,4 @@
 439	common	faccessat2			sys_faccessat2
 440	common	process_madvise			sys_process_madvise
 441	common	watch_mount			sys_watch_mount
+443	common	epoll_pwait2			sys_epoll_pwait2
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index 341fd4ba053b..a6cdf450d0f4 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -487,3 +487,4 @@
 439	common	faccessat2			sys_faccessat2
 440	common	process_madvise			sys_process_madvise
 441	common	watch_mount			sys_watch_mount
+443	common	epoll_pwait2			sys_epoll_pwait2
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 109e6681b8fa..9a4e8ec207fc 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -447,3 +447,4 @@
 440	i386	process_madvise		sys_process_madvise
 441	i386	watch_mount		sys_watch_mount
 442	i386	memfd_secret		sys_memfd_secret
+443	i386	epoll_pwait2		sys_epoll_pwait2		compat_sys_epoll_pwait2
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 742cf17d7725..e2cb5f87e760 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -364,6 +364,7 @@
 440	common	process_madvise		sys_process_madvise
 441	common	watch_mount		sys_watch_mount
 442	common	memfd_secret		sys_memfd_secret
+443	common	epoll_pwait2		sys_epoll_pwait2
 
 #
 # Due to a historical design error, certain syscalls are numbered differently
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index 6225d87be81f..1c8ae5fd026e 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -412,3 +412,4 @@
 439	common	faccessat2			sys_faccessat2
 440	common	process_madvise			sys_process_madvise
 441	common	watch_mount			sys_watch_mount
+443	common	epoll_pwait2			sys_epoll_pwait2
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 297aeb0ee9d1..0f0543e1cc6f 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1714,13 +1714,11 @@ static int ep_send_events(struct eventpoll *ep,
 	return res;
 }
 
-static inline struct timespec64 ep_set_mstimeout(long ms)
+static inline struct timespec64 ep_set_nstimeout(s64 timeout)
 {
-	struct timespec64 now, ts = {
-		.tv_sec = ms / MSEC_PER_SEC,
-		.tv_nsec = NSEC_PER_MSEC * (ms % MSEC_PER_SEC),
-	};
+	struct timespec64 now, ts;
 
+	ts = ns_to_timespec64(timeout);
 	ktime_get_ts64(&now);
 	return timespec64_add_safe(now, ts);
 }
@@ -1734,7 +1732,7 @@ static inline struct timespec64 ep_set_mstimeout(long ms)
  *          stored.
  * @maxevents: Size (in terms of number of events) of the caller event buffer.
  * @timeout: Maximum timeout for the ready events fetch operation, in
- *           milliseconds. If the @timeout is zero, the function will not block,
+ *           nanoseconds. If the @timeout is zero, the function will not block,
  *           while if the @timeout is less than zero, the function will block
  *           until at least one event has been retrieved (or an error
  *           occurred).
@@ -1743,7 +1741,7 @@ static inline struct timespec64 ep_set_mstimeout(long ms)
  *          error code, in case of error.
  */
 static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
-		   int maxevents, long timeout)
+		   int maxevents, s64 timeout)
 {
 	int res, eavail, timed_out = 0;
 	u64 slack = 0;
@@ -1753,7 +1751,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 	lockdep_assert_irqs_enabled();
 
 	if (timeout > 0) {
-		struct timespec64 end_time = ep_set_mstimeout(timeout);
+		struct timespec64 end_time = ep_set_nstimeout(timeout);
 
 		slack = select_estimate_accuracy(&end_time);
 		to = &expires;
@@ -2177,7 +2175,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, fd,
  * part of the user space epoll_wait(2).
  */
 static int do_epoll_wait(int epfd, struct epoll_event __user *events,
-			 int maxevents, int timeout)
+			 int maxevents, s64 timeout)
 {
 	int error;
 	struct fd f;
@@ -2218,19 +2216,27 @@ static int do_epoll_wait(int epfd, struct epoll_event __user *events,
 	return error;
 }
 
+static s64 timeout_to_ns(int timeout)
+{
+	if (timeout <= 0)
+		return timeout;
+	else
+		return timeout * (s64)NSEC_PER_MSEC;
+}
+
 SYSCALL_DEFINE4(epoll_wait, int, epfd, struct epoll_event __user *, events,
 		int, maxevents, int, timeout)
 {
-	return do_epoll_wait(epfd, events, maxevents, timeout);
+	return do_epoll_wait(epfd, events, maxevents, timeout_to_ns(timeout));
 }
 
 /*
  * Implement the event wait interface for the eventpoll file. It is the kernel
  * part of the user space epoll_pwait(2).
  */
-SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events,
-		int, maxevents, int, timeout, const sigset_t __user *, sigmask,
-		size_t, sigsetsize)
+static int do_epoll_pwait(int epfd, struct epoll_event __user *events,
+			  int maxevents, s64 timeout,
+			  const sigset_t __user *sigmask, size_t sigsetsize)
 {
 	int error;
 
@@ -2248,12 +2254,40 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events,
 	return error;
 }
 
+SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events,
+		int, maxevents, int, timeout, const sigset_t __user *, sigmask,
+		size_t, sigsetsize)
+{
+	return do_epoll_pwait(epfd, events, maxevents, timeout_to_ns(timeout),
+			      sigmask, sigsetsize);
+}
+
+SYSCALL_DEFINE6(epoll_pwait2, int, epfd, struct epoll_event __user *, events,
+		int, maxevents, const struct __kernel_timespec __user *, timeout,
+		const sigset_t __user *, sigmask, size_t, sigsetsize)
+{
+	struct timespec64 ts;
+	s64 timeout_ns;
+
+	if (timeout) {
+		if (get_timespec64(&ts, timeout))
+			return -EFAULT;
+		if (!timespec64_valid(&ts))
+			return -EINVAL;
+		timeout_ns = timespec64_to_ns(&ts);
+	} else {
+		timeout_ns = -1;
+	}
+
+	return do_epoll_pwait(epfd, events, maxevents, timeout_ns,
+			      sigmask, sigsetsize);
+}
+
 #ifdef CONFIG_COMPAT
-COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd,
-			struct epoll_event __user *, events,
-			int, maxevents, int, timeout,
-			const compat_sigset_t __user *, sigmask,
-			compat_size_t, sigsetsize)
+static int do_compat_epoll_pwait(int epfd, struct epoll_event __user *events,
+				 int maxevents, s64 timeout,
+				 const compat_sigset_t __user *sigmask,
+				 compat_size_t sigsetsize)
 {
 	long err;
 
@@ -2270,6 +2304,42 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd,
 
 	return err;
 }
+
+COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd,
+		       struct epoll_event __user *, events,
+		       int, maxevents, int, timeout,
+		       const compat_sigset_t __user *, sigmask,
+		       compat_size_t, sigsetsize)
+{
+	return do_compat_epoll_pwait(epfd, events, maxevents,
+				     timeout_to_ns(timeout),
+				     sigmask, sigsetsize);
+}
+
+COMPAT_SYSCALL_DEFINE6(epoll_pwait2, int, epfd,
+		       struct epoll_event __user *, events,
+		       int, maxevents,
+		       const struct __kernel_timespec __user *, timeout,
+		       const compat_sigset_t __user *, sigmask,
+		       compat_size_t, sigsetsize)
+{
+	struct timespec64 ts;
+	s64 timeout_ns;
+
+	if (timeout) {
+		if (get_timespec64(&ts, timeout))
+			return -EFAULT;
+		if (!timespec64_valid(&ts))
+			return -EINVAL;
+		timeout_ns = timespec64_to_ns(&ts);
+	} else {
+		timeout_ns = -1;
+	}
+
+	return do_compat_epoll_pwait(epfd, events, maxevents, timeout_ns,
+				     sigmask, sigsetsize);
+}
+
 #endif
 
 static int __init eventpoll_init(void)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 14d514233e1d..76dc28a2e7f1 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -505,6 +505,12 @@ asmlinkage long compat_sys_epoll_pwait(int epfd,
 			int maxevents, int timeout,
 			const compat_sigset_t __user *sigmask,
 			compat_size_t sigsetsize);
+asmlinkage long compat_sys_epoll_pwait2(int epfd,
+			struct epoll_event __user *events,
+			int maxevents,
+			const struct __kernel_timespec __user *timeout,
+			const compat_sigset_t __user *sigmask,
+			compat_size_t sigsetsize);
 
 /* fs/fcntl.c */
 asmlinkage long compat_sys_fcntl(unsigned int fd, unsigned int cmd,
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e87a96ace85b..2ba45fb7874c 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -362,6 +362,11 @@ asmlinkage long sys_epoll_pwait(int epfd, struct epoll_event __user *events,
 				int maxevents, int timeout,
 				const sigset_t __user *sigmask,
 				size_t sigsetsize);
+asmlinkage long sys_epoll_pwait2(int epfd, struct epoll_event __user *events,
+				 int maxevents,
+				 const struct __kernel_timespec __user *timeout,
+				 const sigset_t __user *sigmask,
+				 size_t sigsetsize);
 
 /* fs/fcntl.c */
 asmlinkage long sys_dup(unsigned int fildes);
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 3bc087d14535..aa883388700e 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -865,9 +865,11 @@ __SYSCALL(__NR_watch_mount, sys_watch_mount)
 #define __NR_memfd_secret 442
 __SYSCALL(__NR_memfd_secret, sys_memfd_secret)
 #endif
+#define __NR_epoll_pwait2 443
+__SC_COMP(__NR_epoll_pwait2, sys_epoll_pwait2, compat_sys_epoll_pwait2)
 
 #undef __NR_syscalls
-#define __NR_syscalls 443
+#define __NR_syscalls 444
 
 /*
  * 32 bit systems traditionally used different
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 805fd7a668be..869aa6b5bf34 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -68,6 +68,8 @@ COND_SYSCALL(epoll_create1);
 COND_SYSCALL(epoll_ctl);
 COND_SYSCALL(epoll_pwait);
 COND_SYSCALL_COMPAT(epoll_pwait);
+COND_SYSCALL(epoll_pwait2);
+COND_SYSCALL_COMPAT(epoll_pwait2);
 
 /* fs/fcntl.c */
 
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH manpages RFC] epoll_wait.2: add epoll_pwait2

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Expand the epoll_wait page with epoll_pwait2, an epoll_wait variant
that takes a struct timespec to enable nanosecond resolution timeout.

    int epoll_pwait2(int fd, struct epoll_event *events,
                     int maxevents,
                     const struct timespec *timeout,
                     const sigset_t *sigset);

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 man2/epoll_wait.2 | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/man2/epoll_wait.2 b/man2/epoll_wait.2
index 55890c82a53a..01047df28cb1 100644
--- a/man2/epoll_wait.2
+++ b/man2/epoll_wait.2
@@ -22,7 +22,7 @@
 .\"
 .TH EPOLL_WAIT 2 2020-04-11 "Linux" "Linux Programmer's Manual"
 .SH NAME
-epoll_wait, epoll_pwait \- wait for an I/O event on an epoll file descriptor
+epoll_wait, epoll_pwait, epoll_pwait2 \- wait for an I/O event on an epoll file descriptor
 .SH SYNOPSIS
 .nf
 .B #include <sys/epoll.h>
@@ -32,6 +32,9 @@ epoll_wait, epoll_pwait \- wait for an I/O event on an epoll file descriptor
 .BI "int epoll_pwait(int " epfd ", struct epoll_event *" events ,
 .BI "               int " maxevents ", int " timeout ,
 .BI "               const sigset_t *" sigmask );
+.BI "int epoll_pwait2(int " epfd ", struct epoll_event *" events ,
+.BI "                int " maxevents ", const struct timespec *" timeout ,
+.BI "                const sigset_t *" sigmask );
 .fi
 .SH DESCRIPTION
 The
@@ -170,6 +173,25 @@ argument may be specified as NULL, in which case
 .BR epoll_pwait ()
 is equivalent to
 .BR epoll_wait ().
+.SS epoll_pwait2 ()
+The
+.BR epoll_pwait2 ()
+system call is equivalent to
+.BR epoll_pwait ()
+except for the
+.I timeout
+argument. It takes an argument of type
+.I timespec
+to be able to specify nanosecond resolution timeouts. This argument functions
+the same as in
+.BR pselect (2)
+and
+.BR ppoll (2).
+If
+.I timeout
+is NULL, then
+.BR epoll_pwait2 ()
+can block indefinitely.
 .SH RETURN VALUE
 When successful,
 .BR epoll_wait ()
@@ -217,6 +239,9 @@ Library support is provided in glibc starting with version 2.3.2.
 .BR epoll_pwait ()
 was added to Linux in kernel 2.6.19.
 Library support is provided in glibc starting with version 2.6.
+.PP
+.BR epoll_pwait2 ()
+was added to Linux in kernel 5.11.
 .SH CONFORMING TO
 .BR epoll_wait ()
 is Linux-specific.
@@ -267,7 +292,9 @@ this means that timeouts greater than 35.79 minutes are treated as infinity.
 .SS C library/kernel differences
 The raw
 .BR epoll_pwait ()
-system call has a sixth argument,
+and
+.BR epoll_pwait2 ()
+system calls have a sixth argument,
 .IR "size_t sigsetsize" ,
 which specifies the size in bytes of the
 .IR sigmask
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v3 2/2] selftests/filesystems: expand epoll with epoll_pwait2

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Code coverage for the epoll_pwait2 syscall.

epoll62: Repeat basic test epoll1, but exercising the new syscall.
epoll63: Pass a timespec and exercise the timeout wakeup path.

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 .../filesystems/epoll/epoll_wakeup_test.c     | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/tools/testing/selftests/filesystems/epoll/epoll_wakeup_test.c b/tools/testing/selftests/filesystems/epoll/epoll_wakeup_test.c
index 8f82f99f7748..4d5656978746 100644
--- a/tools/testing/selftests/filesystems/epoll/epoll_wakeup_test.c
+++ b/tools/testing/selftests/filesystems/epoll/epoll_wakeup_test.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #define _GNU_SOURCE
+#include <asm/unistd.h>
 #include <poll.h>
 #include <unistd.h>
 #include <assert.h>
@@ -21,6 +22,18 @@ struct epoll_mtcontext
 	pthread_t waiter;
 };
 
+#ifndef __NR_epoll_pwait2
+#define __NR_epoll_pwait2 -1
+#endif
+
+static inline int sys_epoll_pwait2(int fd, struct epoll_event *events,
+				   int maxevents,
+				   const struct timespec *timeout,
+				   const sigset_t *sigset)
+{
+	return syscall(__NR_epoll_pwait2, fd, events, maxevents, timeout, sigset);
+}
+
 static void signal_handler(int signum)
 {
 }
@@ -3377,4 +3390,61 @@ TEST(epoll61)
 	close(ctx.evfd);
 }
 
+/* Equivalent to basic test epoll1, but exercising epoll_pwait2. */
+TEST(epoll62)
+{
+	int efd;
+	int sfd[2];
+	struct epoll_event e;
+
+	ASSERT_EQ(socketpair(AF_UNIX, SOCK_STREAM, 0, sfd), 0);
+
+	efd = epoll_create(1);
+	ASSERT_GE(efd, 0);
+
+	e.events = EPOLLIN;
+	ASSERT_EQ(epoll_ctl(efd, EPOLL_CTL_ADD, sfd[0], &e), 0);
+
+	ASSERT_EQ(write(sfd[1], "w", 1), 1);
+
+	EXPECT_EQ(sys_epoll_pwait2(efd, &e, 1, NULL, NULL), 1);
+	EXPECT_EQ(sys_epoll_pwait2(efd, &e, 1, NULL, NULL), 1);
+
+	close(efd);
+	close(sfd[0]);
+	close(sfd[1]);
+}
+
+/* Epoll_pwait2 basic timeout test. */
+TEST(epoll63)
+{
+	const int cfg_delay_ms = 10;
+	unsigned long long tdiff;
+	struct timespec ts;
+	int efd;
+	int sfd[2];
+	struct epoll_event e;
+
+	ASSERT_EQ(socketpair(AF_UNIX, SOCK_STREAM, 0, sfd), 0);
+
+	efd = epoll_create(1);
+	ASSERT_GE(efd, 0);
+
+	e.events = EPOLLIN;
+	ASSERT_EQ(epoll_ctl(efd, EPOLL_CTL_ADD, sfd[0], &e), 0);
+
+	ts.tv_sec = 0;
+	ts.tv_nsec = cfg_delay_ms * 1000 * 1000;
+
+	tdiff = msecs();
+	EXPECT_EQ(sys_epoll_pwait2(efd, &e, 1, &ts, NULL), 0);
+	tdiff = msecs() - tdiff;
+
+	EXPECT_GE(tdiff, cfg_delay_ms);
+
+	close(efd);
+	close(sfd[0]);
+	close(sfd[1]);
+}
+
 TEST_HARNESS_MAIN
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Matthew Wilcox]

On Wed, Nov 18, 2020 at 09:46:15AM -0500, Willem de Bruijn wrote:
> -static inline struct timespec64 ep_set_mstimeout(long ms)
> +static inline struct timespec64 ep_set_nstimeout(s64 timeout)
>  {
> -	struct timespec64 now, ts = {
> -		.tv_sec = ms / MSEC_PER_SEC,
> -		.tv_nsec = NSEC_PER_MSEC * (ms % MSEC_PER_SEC),
> -	};
> +	struct timespec64 now, ts;
>  
> +	ts = ns_to_timespec64(timeout);
>  	ktime_get_ts64(&now);
>  	return timespec64_add_safe(now, ts);
>  }

Why do you pass around an s64 for timeout, converting it to and from
a timespec64 instead of passing around a timespec64?

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

On Wed, Nov 18, 2020 at 10:00 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Nov 18, 2020 at 09:46:15AM -0500, Willem de Bruijn wrote:
> > -static inline struct timespec64 ep_set_mstimeout(long ms)
> > +static inline struct timespec64 ep_set_nstimeout(s64 timeout)
> >  {
> > -     struct timespec64 now, ts = {
> > -             .tv_sec = ms / MSEC_PER_SEC,
> > -             .tv_nsec = NSEC_PER_MSEC * (ms % MSEC_PER_SEC),
> > -     };
> > +     struct timespec64 now, ts;
> >
> > +     ts = ns_to_timespec64(timeout);
> >       ktime_get_ts64(&now);
> >       return timespec64_add_safe(now, ts);
> >  }
>
> Why do you pass around an s64 for timeout, converting it to and from
> a timespec64 instead of passing around a timespec64?

I implemented both approaches. The alternative was no simpler.
Conversion in existing epoll_wait, epoll_pwait and epoll_pwait
(compat) becomes a bit more complex and adds a stack variable there if
passing the timespec64 by reference. And in ep_poll the ternary
timeout test > 0, 0, < 0 now requires checking both tv_secs and
tv_nsecs. Based on that, I found this simpler. But no strong
preference.

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Arnd Bergmann]

On Wed, Nov 18, 2020 at 4:10 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Wed, Nov 18, 2020 at 10:00 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Wed, Nov 18, 2020 at 09:46:15AM -0500, Willem de Bruijn wrote:
> > > -static inline struct timespec64 ep_set_mstimeout(long ms)
> > > +static inline struct timespec64 ep_set_nstimeout(s64 timeout)
> > >  {
> > > -     struct timespec64 now, ts = {
> > > -             .tv_sec = ms / MSEC_PER_SEC,
> > > -             .tv_nsec = NSEC_PER_MSEC * (ms % MSEC_PER_SEC),
> > > -     };
> > > +     struct timespec64 now, ts;
> > >
> > > +     ts = ns_to_timespec64(timeout);
> > >       ktime_get_ts64(&now);
> > >       return timespec64_add_safe(now, ts);
> > >  }
> >
> > Why do you pass around an s64 for timeout, converting it to and from
> > a timespec64 instead of passing around a timespec64?
>
> I implemented both approaches. The alternative was no simpler.
> Conversion in existing epoll_wait, epoll_pwait and epoll_pwait
> (compat) becomes a bit more complex and adds a stack variable there if
> passing the timespec64 by reference. And in ep_poll the ternary
> timeout test > 0, 0, < 0 now requires checking both tv_secs and
> tv_nsecs. Based on that, I found this simpler. But no strong
> preference.

The 64-bit division can be fairly expensive on 32-bit architectures,
at least when it doesn't get optimized into a multiply+shift.

     Arnd

RE: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[David Laight]

From: Arnd Bergmann
> Sent: 18 November 2020 15:38
> 
> On Wed, Nov 18, 2020 at 4:10 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> > On Wed, Nov 18, 2020 at 10:00 AM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Wed, Nov 18, 2020 at 09:46:15AM -0500, Willem de Bruijn wrote:
> > > > -static inline struct timespec64 ep_set_mstimeout(long ms)
> > > > +static inline struct timespec64 ep_set_nstimeout(s64 timeout)
> > > >  {
> > > > -     struct timespec64 now, ts = {
> > > > -             .tv_sec = ms / MSEC_PER_SEC,
> > > > -             .tv_nsec = NSEC_PER_MSEC * (ms % MSEC_PER_SEC),
> > > > -     };
> > > > +     struct timespec64 now, ts;
> > > >
> > > > +     ts = ns_to_timespec64(timeout);
> > > >       ktime_get_ts64(&now);
> > > >       return timespec64_add_safe(now, ts);
> > > >  }
> > >
> > > Why do you pass around an s64 for timeout, converting it to and from
> > > a timespec64 instead of passing around a timespec64?
> >
> > I implemented both approaches. The alternative was no simpler.
> > Conversion in existing epoll_wait, epoll_pwait and epoll_pwait
> > (compat) becomes a bit more complex and adds a stack variable there if
> > passing the timespec64 by reference. And in ep_poll the ternary
> > timeout test > 0, 0, < 0 now requires checking both tv_secs and
> > tv_nsecs. Based on that, I found this simpler. But no strong
> > preference.
> 
> The 64-bit division can be fairly expensive on 32-bit architectures,
> at least when it doesn't get optimized into a multiply+shift.

I'd have thought you'd want to do everything in 64bit nanosecs.
Conversions to/from any of the 'timespec' structure are expensive.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

On Wed, Nov 18, 2020 at 9:46 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.com>
>
> Add syscall epoll_pwait2, an epoll_wait variant with nsec resolution
> that replaces int timeout with struct timespec. It is equivalent
> otherwise.
>
>     int epoll_pwait2(int fd, struct epoll_event *events,
>                      int maxevents,
>                      const struct timespec *timeout,
>                      const sigset_t *sigset);
>
> The underlying hrtimer is already programmed with nsec resolution.
> pselect and ppoll also set nsec resolution timeout with timespec.
>
> The sigset_t in epoll_pwait has a compat variant. epoll_pwait2 needs
> the same.
>
> For timespec, only support this new interface on 2038 aware platforms
> that define __kernel_timespec_t. So no CONFIG_COMPAT_32BIT_TIME.
>
> Changes
>   v3:
>   - rewrite: add epoll_pwait2 syscall instead of epoll_create1 flag
>   v2:
>   - cast to s64: avoid overflow on 32-bit platforms (Shuo Chen)
>   - minor commit message rewording
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
>
> ---

> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 109e6681b8fa..9a4e8ec207fc 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -447,3 +447,4 @@
>  440    i386    process_madvise         sys_process_madvise
>  441    i386    watch_mount             sys_watch_mount
>  442    i386    memfd_secret            sys_memfd_secret
> +443    i386    epoll_pwait2            sys_epoll_pwait2                compat_sys_epoll_pwait2

I should have caught this sooner, but this does not work as intended.

x86 will still call epoll_pwait2 with old_timespec32.

One approach is a separate epoll_pwait2_time64 syscall, similar to
ppoll_time64. But that was added to work around legacy 32-bit ppoll.
Not needed for a new API.

In libc, ppoll_time64 is declared with type struct __timespec64. That
type is not defined in Linux uapi. Will need to look at this some
more.

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Arnd Bergmann]

On Wed, Nov 18, 2020 at 5:21 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> > diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> > index 109e6681b8fa..9a4e8ec207fc 100644
> > --- a/arch/x86/entry/syscalls/syscall_32.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> > @@ -447,3 +447,4 @@
> >  440    i386    process_madvise         sys_process_madvise
> >  441    i386    watch_mount             sys_watch_mount
> >  442    i386    memfd_secret            sys_memfd_secret
> > +443    i386    epoll_pwait2            sys_epoll_pwait2                compat_sys_epoll_pwait2
>
> I should have caught this sooner, but this does not work as intended.
>
> x86 will still call epoll_pwait2 with old_timespec32.
>
> One approach is a separate epoll_pwait2_time64 syscall, similar to
> ppoll_time64. But that was added to work around legacy 32-bit ppoll.
> Not needed for a new API.
>
> In libc, ppoll_time64 is declared with type struct __timespec64. That
> type is not defined in Linux uapi. Will need to look at this some
> more.

The libc __timespec64 corresponds to the __kernel_timespec64
structure in uapi. It is defined to only have 'long' nanoseconds
member because that's what c99 and posix require, but the bits
are in the position that matches the lower 32 bits of the 64-bit
tv_nsec in the kernel, and get_timespec64() performs the
necessary conversion to either check or zero the upper bits.

I think all you need in user space is to pass the timeout as a
__timespec64 structure and add a conversion in the exported
library interface.

       Arnd

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

On Wed, Nov 18, 2020 at 11:50 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Wed, Nov 18, 2020 at 5:21 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> > > diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> > > index 109e6681b8fa..9a4e8ec207fc 100644
> > > --- a/arch/x86/entry/syscalls/syscall_32.tbl
> > > +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> > > @@ -447,3 +447,4 @@
> > >  440    i386    process_madvise         sys_process_madvise
> > >  441    i386    watch_mount             sys_watch_mount
> > >  442    i386    memfd_secret            sys_memfd_secret
> > > +443    i386    epoll_pwait2            sys_epoll_pwait2                compat_sys_epoll_pwait2
> >
> > I should have caught this sooner, but this does not work as intended.
> >
> > x86 will still call epoll_pwait2 with old_timespec32.
> >
> > One approach is a separate epoll_pwait2_time64 syscall, similar to
> > ppoll_time64. But that was added to work around legacy 32-bit ppoll.
> > Not needed for a new API.
> >
> > In libc, ppoll_time64 is declared with type struct __timespec64. That
> > type is not defined in Linux uapi. Will need to look at this some
> > more.
>
> The libc __timespec64 corresponds to the __kernel_timespec64
> structure in uapi. It is defined to only have 'long' nanoseconds
> member because that's what c99 and posix require, but the bits
> are in the position that matches the lower 32 bits of the 64-bit
> tv_nsec in the kernel, and get_timespec64() performs the
> necessary conversion to either check or zero the upper bits.
>
> I think all you need in user space is to pass the timeout as a
> __timespec64 structure and add a conversion in the exported
> library interface.

Indeed, that resolves it, thanks.

I'll define that struct in the selftest and update the definition of
sys_epoll_pwait2 there.

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

On Wed, Nov 18, 2020 at 10:59 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Arnd Bergmann
> > Sent: 18 November 2020 15:38
> >
> > On Wed, Nov 18, 2020 at 4:10 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > > On Wed, Nov 18, 2020 at 10:00 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Wed, Nov 18, 2020 at 09:46:15AM -0500, Willem de Bruijn wrote:
> > > > > -static inline struct timespec64 ep_set_mstimeout(long ms)
> > > > > +static inline struct timespec64 ep_set_nstimeout(s64 timeout)
> > > > >  {
> > > > > -     struct timespec64 now, ts = {
> > > > > -             .tv_sec = ms / MSEC_PER_SEC,
> > > > > -             .tv_nsec = NSEC_PER_MSEC * (ms % MSEC_PER_SEC),
> > > > > -     };
> > > > > +     struct timespec64 now, ts;
> > > > >
> > > > > +     ts = ns_to_timespec64(timeout);
> > > > >       ktime_get_ts64(&now);
> > > > >       return timespec64_add_safe(now, ts);
> > > > >  }
> > > >
> > > > Why do you pass around an s64 for timeout, converting it to and from
> > > > a timespec64 instead of passing around a timespec64?
> > >
> > > I implemented both approaches. The alternative was no simpler.
> > > Conversion in existing epoll_wait, epoll_pwait and epoll_pwait
> > > (compat) becomes a bit more complex and adds a stack variable there if
> > > passing the timespec64 by reference. And in ep_poll the ternary
> > > timeout test > 0, 0, < 0 now requires checking both tv_secs and
> > > tv_nsecs. Based on that, I found this simpler. But no strong
> > > preference.
> >
> > The 64-bit division can be fairly expensive on 32-bit architectures,
> > at least when it doesn't get optimized into a multiply+shift.
>
> I'd have thought you'd want to do everything in 64bit nanosecs.
> Conversions to/from any of the 'timespec' structure are expensive.

I took another look at this.

The only real reason for the timespec64 is that
select_estimate_accuracy takes that type. Which makes sense, because
do_select does.

But for epoll, this is inefficient: in ep_set_mstimeout it calls
ktime_get_ts64 to convert timeout to an offset from current time, only
to pass it to select_estimate_accuracy to then perform another
ktime_get_ts64 and subtract this to get back to (approx.) the original
timeout.

How about a separate patch that adds epoll_estimate_accuracy with
the same rules (wrt rt_task, current->timer_slack, nice and upper bound)
but taking an s64 timeout.

One variation, since it is approximate, I suppose we could even replace
division by a right shift?

After that, using s64 everywhere is indeed much simpler. And with that
I will revise the new epoll_pwait2 interface to take a long long
instead of struct timespec.

Apologies for the delay. I forgot that I'm only subscribed to netdev@
in my main email account.

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Matthew Wilcox]

On Thu, Nov 19, 2020 at 09:19:35AM -0500, Willem de Bruijn wrote:
> But for epoll, this is inefficient: in ep_set_mstimeout it calls
> ktime_get_ts64 to convert timeout to an offset from current time, only
> to pass it to select_estimate_accuracy to then perform another
> ktime_get_ts64 and subtract this to get back to (approx.) the original
> timeout.
> 
> How about a separate patch that adds epoll_estimate_accuracy with
> the same rules (wrt rt_task, current->timer_slack, nice and upper bound)
> but taking an s64 timeout.
> 
> One variation, since it is approximate, I suppose we could even replace
> division by a right shift?
> 
> After that, using s64 everywhere is indeed much simpler. And with that
> I will revise the new epoll_pwait2 interface to take a long long
> instead of struct timespec.

I think the userspace interface should take a struct timespec
for consistency with ppoll and pselect.  And epoll should use
poll_select_set_timeout() to convert the relative timeout to an absolute
endtime.  Make epoll more consistent with select/poll, not less ...

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

On Thu, Nov 19, 2020 at 9:31 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Nov 19, 2020 at 09:19:35AM -0500, Willem de Bruijn wrote:
> > But for epoll, this is inefficient: in ep_set_mstimeout it calls
> > ktime_get_ts64 to convert timeout to an offset from current time, only
> > to pass it to select_estimate_accuracy to then perform another
> > ktime_get_ts64 and subtract this to get back to (approx.) the original
> > timeout.
> >
> > How about a separate patch that adds epoll_estimate_accuracy with
> > the same rules (wrt rt_task, current->timer_slack, nice and upper bound)
> > but taking an s64 timeout.
> >
> > One variation, since it is approximate, I suppose we could even replace
> > division by a right shift?
> >
> > After that, using s64 everywhere is indeed much simpler. And with that
> > I will revise the new epoll_pwait2 interface to take a long long
> > instead of struct timespec.
>
> I think the userspace interface should take a struct timespec
> for consistency with ppoll and pselect. And epoll should use
> poll_select_set_timeout() to convert the relative timeout to an absolute
> endtime.  Make epoll more consistent with select/poll, not less ...

Okay. The absolute time is also needed for schedule_hrtimeout_range,
so it could not be entirely avoided, anyway.

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Arnd Bergmann]

On Thu, Nov 19, 2020 at 3:31 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Nov 19, 2020 at 09:19:35AM -0500, Willem de Bruijn wrote:
> > But for epoll, this is inefficient: in ep_set_mstimeout it calls
> > ktime_get_ts64 to convert timeout to an offset from current time, only
> > to pass it to select_estimate_accuracy to then perform another
> > ktime_get_ts64 and subtract this to get back to (approx.) the original
> > timeout.

Right, it would be good to avoid the second ktime_get_ts64(), as reading
the clocksource itself can be expensive.

> > How about a separate patch that adds epoll_estimate_accuracy with
> > the same rules (wrt rt_task, current->timer_slack, nice and upper bound)
> > but taking an s64 timeout.
> >
> > One variation, since it is approximate, I suppose we could even replace
> > division by a right shift?

The right shift would work indeed, but it's also a bit ugly unless
__estimate_accuracy() is changed to always use the same shift.

I see that on 32-bit ARM, select_estimate_accuracy() calls
the external __aeabi_idiv() function to do the 32-bit division, so
changing it to a shift would speed up select as well.

Changing select_estimate_accuracy() to take the relative timeout
as an argument to avoid the extra ktime_get_ts64() should
have a larger impact.

> > After that, using s64 everywhere is indeed much simpler. And with that
> > I will revise the new epoll_pwait2 interface to take a long long
> > instead of struct timespec.
>
> I think the userspace interface should take a struct timespec
> for consistency with ppoll and pselect.  And epoll should use
> poll_select_set_timeout() to convert the relative timeout to an absolute
> endtime.  Make epoll more consistent with select/poll, not less ...

I don't see a problem with an s64 timeout if that makes the interface
simpler by avoiding differences between the 32-bit and 64-bit ABIs.

More importantly, I think it should differ from poll/select by calculating
and writing back the remaining timeout.

I don't know what the latest view on absolute timeouts at the syscall
ABI is, it would probably simplify the implementation, but make it
less consistent with the others. Futex uses absolute timeouts, but
is itself inconsistent about that.

     Arnd

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

On Thu, Nov 19, 2020 at 10:45 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Thu, Nov 19, 2020 at 3:31 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Thu, Nov 19, 2020 at 09:19:35AM -0500, Willem de Bruijn wrote:
> > > But for epoll, this is inefficient: in ep_set_mstimeout it calls
> > > ktime_get_ts64 to convert timeout to an offset from current time, only
> > > to pass it to select_estimate_accuracy to then perform another
> > > ktime_get_ts64 and subtract this to get back to (approx.) the original
> > > timeout.
>
> Right, it would be good to avoid the second ktime_get_ts64(), as reading
> the clocksource itself can be expensive.
>
> > > How about a separate patch that adds epoll_estimate_accuracy with
> > > the same rules (wrt rt_task, current->timer_slack, nice and upper bound)
> > > but taking an s64 timeout.
> > >
> > > One variation, since it is approximate, I suppose we could even replace
> > > division by a right shift?
>
> The right shift would work indeed, but it's also a bit ugly unless
> __estimate_accuracy() is changed to always use the same shift.
>
> I see that on 32-bit ARM, select_estimate_accuracy() calls
> the external __aeabi_idiv() function to do the 32-bit division, so
> changing it to a shift would speed up select as well.
>
> Changing select_estimate_accuracy() to take the relative timeout
> as an argument to avoid the extra ktime_get_ts64() should
> have a larger impact.

It could be done by having poll_select_set_timeout take an extra u64*
slack, call select_estimate_accuracy before adding in the current time
and then pass the slack down to do_select and do_sys_poll, also
through core_sys_select and compat_core_sys_select.

It could be a patch independent from this new syscall. Since it changes
poll_select_set_timeout it clearly has a conflict with the planned next
revision of this. I can include it in the next patchset to decide whether
it's worth it.

> > > After that, using s64 everywhere is indeed much simpler. And with that
> > > I will revise the new epoll_pwait2 interface to take a long long
> > > instead of struct timespec.
> >
> > I think the userspace interface should take a struct timespec
> > for consistency with ppoll and pselect.  And epoll should use
> > poll_select_set_timeout() to convert the relative timeout to an absolute
> > endtime.  Make epoll more consistent with select/poll, not less ...
>
> I don't see a problem with an s64 timeout if that makes the interface
> simpler by avoiding differences between the 32-bit and 64-bit ABIs.
>
> More importantly, I think it should differ from poll/select by calculating
> and writing back the remaining timeout.
>
> I don't know what the latest view on absolute timeouts at the syscall
> ABI is, it would probably simplify the implementation, but make it
> less consistent with the others. Futex uses absolute timeouts, but
> is itself inconsistent about that.

If the implementation internally uses poll_select_set_timeout and
passes around timespec64 *, it won't matter much in terms of
performance or implementation. Then there seems to be no downside to
following the consistency argument.

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Arnd Bergmann]

On Thu, Nov 19, 2020 at 9:13 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Thu, Nov 19, 2020 at 10:45 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > On Thu, Nov 19, 2020 at 3:31 PM Matthew Wilcox <willy@infradead.org> wrote:
> > The right shift would work indeed, but it's also a bit ugly unless
> > __estimate_accuracy() is changed to always use the same shift.
> >
> > I see that on 32-bit ARM, select_estimate_accuracy() calls
> > the external __aeabi_idiv() function to do the 32-bit division, so
> > changing it to a shift would speed up select as well.
> >
> > Changing select_estimate_accuracy() to take the relative timeout
> > as an argument to avoid the extra ktime_get_ts64() should
> > have a larger impact.
>
> It could be done by having poll_select_set_timeout take an extra u64*
> slack, call select_estimate_accuracy before adding in the current time
> and then pass the slack down to do_select and do_sys_poll, also
> through core_sys_select and compat_core_sys_select.
>
> It could be a patch independent from this new syscall. Since it changes
> poll_select_set_timeout it clearly has a conflict with the planned next
> revision of this. I can include it in the next patchset to decide whether
> it's worth it.

Yes, that sounds good, not sure how much rework this would require.

It would be easier to do if we first merged the native and compat
native versions of select/pselect/ppoll by moving the
in_compat_syscall() check into combined get_sigset()
and get_fd_set() helpers. I would assume you have enough
on your plate already and don't want to add that to it.

> > I don't see a problem with an s64 timeout if that makes the interface
> > simpler by avoiding differences between the 32-bit and 64-bit ABIs.
> >
> > More importantly, I think it should differ from poll/select by calculating
> > and writing back the remaining timeout.
> >
> > I don't know what the latest view on absolute timeouts at the syscall
> > ABI is, it would probably simplify the implementation, but make it
> > less consistent with the others. Futex uses absolute timeouts, but
> > is itself inconsistent about that.
>
> If the implementation internally uses poll_select_set_timeout and
> passes around timespec64 *, it won't matter much in terms of
> performance or implementation. Then there seems to be no downside to
> following the consistency argument.

Ok. So to clarify, you would stay with relative __kernel_timespec
pointers and not copy back the remaining time, correct?

       ARnd

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

On Fri, Nov 20, 2020 at 3:13 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Thu, Nov 19, 2020 at 9:13 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> > On Thu, Nov 19, 2020 at 10:45 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > > On Thu, Nov 19, 2020 at 3:31 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > The right shift would work indeed, but it's also a bit ugly unless
> > > __estimate_accuracy() is changed to always use the same shift.
> > >
> > > I see that on 32-bit ARM, select_estimate_accuracy() calls
> > > the external __aeabi_idiv() function to do the 32-bit division, so
> > > changing it to a shift would speed up select as well.
> > >
> > > Changing select_estimate_accuracy() to take the relative timeout
> > > as an argument to avoid the extra ktime_get_ts64() should
> > > have a larger impact.
> >
> > It could be done by having poll_select_set_timeout take an extra u64*
> > slack, call select_estimate_accuracy before adding in the current time
> > and then pass the slack down to do_select and do_sys_poll, also
> > through core_sys_select and compat_core_sys_select.
> >
> > It could be a patch independent from this new syscall. Since it changes
> > poll_select_set_timeout it clearly has a conflict with the planned next
> > revision of this. I can include it in the next patchset to decide whether
> > it's worth it.
>
> Yes, that sounds good, not sure how much rework this would require.
>
> It would be easier to do if we first merged the native and compat
> native versions of select/pselect/ppoll by moving the
> in_compat_syscall() check into combined get_sigset()
> and get_fd_set() helpers. I would assume you have enough
> on your plate already and don't want to add that to it.

Thanks for the suggestion.

I do have an initial patchset. As expected, it does involve quite a
bit of code churn to pass slack through the callers. I'll take a look
at your suggestion to simplify it.

As is, the patchset is not ready to send to the list for possible
merge. In the meantime, I did push the patchset to github at
https://github.com/wdebruij/linux/commits/epoll-nstimeo-1 . I can send
a version marked RFC to the list if that's easier.

I made the slack specific changes in two separate patches, one to
fs/select.c and one to fs/eventpoll.c, and placed these at the end of
the patchset. So we could first finish the syscall and then send this
as a separate patchset if it proves complex enough.

Btw, the other change, to convert epoll implementation to timespec64
before adding the syscall, equally adds some code churn compared to
patch v3. But perhaps the end state is cleaner and more consistent.

> > > I don't see a problem with an s64 timeout if that makes the interface
> > > simpler by avoiding differences between the 32-bit and 64-bit ABIs.
> > >
> > > More importantly, I think it should differ from poll/select by calculating
> > > and writing back the remaining timeout.
> > >
> > > I don't know what the latest view on absolute timeouts at the syscall
> > > ABI is, it would probably simplify the implementation, but make it
> > > less consistent with the others. Futex uses absolute timeouts, but
> > > is itself inconsistent about that.
> >
> > If the implementation internally uses poll_select_set_timeout and
> > passes around timespec64 *, it won't matter much in terms of
> > performance or implementation. Then there seems to be no downside to
> > following the consistency argument.
>
> Ok. So to clarify, you would stay with relative __kernel_timespec
> pointers and not copy back the remaining time, correct?

That's my understanding, and the current implementation.

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Arnd Bergmann]

On Fri, Nov 20, 2020 at 5:01 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Fri, Nov 20, 2020 at 3:13 AM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > On Thu, Nov 19, 2020 at 9:13 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > > On Thu, Nov 19, 2020 at 10:45 AM Arnd Bergmann <arnd@kernel.org> wrote:

> Thanks for the suggestion.
>
> I do have an initial patchset. As expected, it does involve quite a
> bit of code churn to pass slack through the callers. I'll take a look
> at your suggestion to simplify it.
>
> As is, the patchset is not ready to send to the list for possible
> merge. In the meantime, I did push the patchset to github at
> https://github.com/wdebruij/linux/commits/epoll-nstimeo-1 . I can send
> a version marked RFC to the list if that's easier.

Looks all good to me, just two small things I noticed that you can
address before sending the new series:

* The div_u64_rem() in ep_timeout_to_timespec() looks wrong, as
  you are actually dividing a 'long' that does not need it.

* In "epoll: wire up syscall epoll_pwait2", the alpha syscall has the
wrong number, it
   should be 110 higher than the others, not 109.

> Btw, the other change, to convert epoll implementation to timespec64
> before adding the syscall, equally adds some code churn compared to
> patch v3. But perhaps the end state is cleaner and more consistent.

Right, that's what I meant. If it causes too much churn, don't worry
about it it.

       Arndd

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

On Fri, Nov 20, 2020 at 2:23 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Fri, Nov 20, 2020 at 5:01 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Fri, Nov 20, 2020 at 3:13 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > >
> > > On Thu, Nov 19, 2020 at 9:13 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > On Thu, Nov 19, 2020 at 10:45 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> > Thanks for the suggestion.
> >
> > I do have an initial patchset. As expected, it does involve quite a
> > bit of code churn to pass slack through the callers. I'll take a look
> > at your suggestion to simplify it.
> >
> > As is, the patchset is not ready to send to the list for possible
> > merge. In the meantime, I did push the patchset to github at
> > https://github.com/wdebruij/linux/commits/epoll-nstimeo-1 . I can send
> > a version marked RFC to the list if that's easier.
>
> Looks all good to me, just two small things I noticed that you can
> address before sending the new series:
>
> * The div_u64_rem() in ep_timeout_to_timespec() looks wrong, as
>   you are actually dividing a 'long' that does not need it.
>
> * In "epoll: wire up syscall epoll_pwait2", the alpha syscall has the
> wrong number, it
>    should be 110 higher than the others, not 109.

Thanks! I'll fix these up.

> > Btw, the other change, to convert epoll implementation to timespec64
> > before adding the syscall, equally adds some code churn compared to
> > patch v3. But perhaps the end state is cleaner and more consistent.
>
> Right, that's what I meant. If it causes too much churn, don't worry
> about it it.

I think it'll be better to split the patchsets:

epoll: convert internal api to timespec64
epoll: add syscall epoll_pwait2
epoll: wire up syscall epoll_pwait2
selftests/filesystems: expand epoll with epoll_pwait2

and

select: compute slack based on relative time
epoll: compute slack based on relative time

and judge the slack conversion on its own merit.

I also would rather not tie this up with the compat deduplication.
Happy to take a stab at that though. On that note, when combining
functions like

  int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
                           fd_set __user *exp, struct timespec64 *end_time,
                           u64 slack)

and

  static int compat_core_sys_select(int n, compat_ulong_t __user *inp,
        compat_ulong_t __user *outp, compat_ulong_t __user *exp,
        struct timespec64 *end_time, u64 slack)

by branching on in_compat_syscall() inside get_fd_set/set_fd_set and
deprecating their compat_.. counterparts, what would the argument
pointers look like? Or is that not the approach you have in mind?

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Arnd Bergmann]

On Fri, Nov 20, 2020 at 11:28 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Fri, Nov 20, 2020 at 2:23 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > On Fri, Nov 20, 2020 at 5:01 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>
> I think it'll be better to split the patchsets:
>
> epoll: convert internal api to timespec64
> epoll: add syscall epoll_pwait2
> epoll: wire up syscall epoll_pwait2
> selftests/filesystems: expand epoll with epoll_pwait2
>
> and
>
> select: compute slack based on relative time
> epoll: compute slack based on relative time
>
> and judge the slack conversion on its own merit.

Yes, makes sense.

> I also would rather not tie this up with the compat deduplication.
> Happy to take a stab at that though. On that note, when combining
> functions like
>
>   int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
>                            fd_set __user *exp, struct timespec64 *end_time,
>                            u64 slack)
>
> and
>
>   static int compat_core_sys_select(int n, compat_ulong_t __user *inp,
>         compat_ulong_t __user *outp, compat_ulong_t __user *exp,
>         struct timespec64 *end_time, u64 slack)
>
> by branching on in_compat_syscall() inside get_fd_set/set_fd_set and
> deprecating their compat_.. counterparts, what would the argument
> pointers look like? Or is that not the approach you have in mind?

In this case, the top-level entry points becomes unified, and you get
the prototype from core_sys_select() with the native arguments.

I would imagine this can be done like the way I proposed
for get_bitmap() in sys_migrate_pages:

https://lore.kernel.org/lkml/20201102123151.2860165-4-arnd@kernel.org/

        Arnd

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

On Sat, Nov 21, 2020 at 4:27 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Fri, Nov 20, 2020 at 11:28 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> > On Fri, Nov 20, 2020 at 2:23 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > > On Fri, Nov 20, 2020 at 5:01 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> >
> > I think it'll be better to split the patchsets:
> >
> > epoll: convert internal api to timespec64
> > epoll: add syscall epoll_pwait2
> > epoll: wire up syscall epoll_pwait2
> > selftests/filesystems: expand epoll with epoll_pwait2
> >
> > and
> >
> > select: compute slack based on relative time
> > epoll: compute slack based on relative time
> >
> > and judge the slack conversion on its own merit.
>
> Yes, makes sense.
>
> > I also would rather not tie this up with the compat deduplication.
> > Happy to take a stab at that though. On that note, when combining
> > functions like
> >
> >   int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
> >                            fd_set __user *exp, struct timespec64 *end_time,
> >                            u64 slack)
> >
> > and
> >
> >   static int compat_core_sys_select(int n, compat_ulong_t __user *inp,
> >         compat_ulong_t __user *outp, compat_ulong_t __user *exp,
> >         struct timespec64 *end_time, u64 slack)
> >
> > by branching on in_compat_syscall() inside get_fd_set/set_fd_set and
> > deprecating their compat_.. counterparts, what would the argument
> > pointers look like? Or is that not the approach you have in mind?
>
> In this case, the top-level entry points becomes unified, and you get
> the prototype from core_sys_select() with the native arguments.
>
> I would imagine this can be done like the way I proposed
> for get_bitmap() in sys_migrate_pages:
>
> https://lore.kernel.org/lkml/20201102123151.2860165-4-arnd@kernel.org/

Coming back to this. Current patchset includes new select and poll
selftests to verify the changes. I need to send a small kselftest
patch for that first.

Assuming there's no time pressure, I will finish up and send the main
changes after the merge window, for the next release then.

Current state against linux-next at
https://github.com/wdebruij/linux-next-mirror/tree/select-compat-1

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Arnd Bergmann]

On Thu, Dec 10, 2020 at 6:33 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Sat, Nov 21, 2020 at 4:27 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > On Fri, Nov 20, 2020 at 11:28 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> > I would imagine this can be done like the way I proposed
> > for get_bitmap() in sys_migrate_pages:
> >
> > https://lore.kernel.org/lkml/20201102123151.2860165-4-arnd@kernel.org/
>
> Coming back to this. Current patchset includes new select and poll
> selftests to verify the changes. I need to send a small kselftest
> patch for that first.
>
> Assuming there's no time pressure, I will finish up and send the main
> changes after the merge window, for the next release then.
>
> Current state against linux-next at
> https://github.com/wdebruij/linux-next-mirror/tree/select-compat-1

Ok, sounds good to me. I've had a (very brief) look and have one
suggestion: instead of open-coding the compat vs native mode
in multiple places like

if (!in_compat_syscall())
     return copy_from_user(fdset, ufdset, FDS_BYTES(nr)) ? -EFAULT : 0;
else
     return compat_get_bitmap(fdset, ufdset, nr);

maybe move this into a separate function and call that where needed.

I've done this for the get_bitmap() function in my series at

https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/commit/?h=compat-alloc-user-space-7&id=b1b23ebb12b635654a2060df49455167a142c5d2

The definition is slightly differrent for cpumask, nodemask and fd_set,
so we'd need to try out the best way to structure the code to end
up with the most readable version, but it should be possible when
there are only three callers (and duplicating the function would
be the end of the world either)

        Arnd

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

On Thu, Dec 10, 2020 at 3:34 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Thu, Dec 10, 2020 at 6:33 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> > On Sat, Nov 21, 2020 at 4:27 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > > On Fri, Nov 20, 2020 at 11:28 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> > > I would imagine this can be done like the way I proposed
> > > for get_bitmap() in sys_migrate_pages:
> > >
> > > https://lore.kernel.org/lkml/20201102123151.2860165-4-arnd@kernel.org/
> >
> > Coming back to this. Current patchset includes new select and poll
> > selftests to verify the changes. I need to send a small kselftest
> > patch for that first.
> >
> > Assuming there's no time pressure, I will finish up and send the main
> > changes after the merge window, for the next release then.
> >
> > Current state against linux-next at
> > https://github.com/wdebruij/linux-next-mirror/tree/select-compat-1
>
> Ok, sounds good to me. I've had a (very brief) look and have one
> suggestion: instead of open-coding the compat vs native mode
> in multiple places like
>
> if (!in_compat_syscall())
>      return copy_from_user(fdset, ufdset, FDS_BYTES(nr)) ? -EFAULT : 0;
> else
>      return compat_get_bitmap(fdset, ufdset, nr);
>
> maybe move this into a separate function and call that where needed.
>
> I've done this for the get_bitmap() function in my series at
>
> https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/commit/?h=compat-alloc-user-space-7&id=b1b23ebb12b635654a2060df49455167a142c5d2
>
> The definition is slightly differrent for cpumask, nodemask and fd_set,
> so we'd need to try out the best way to structure the code to end
> up with the most readable version, but it should be possible when
> there are only three callers (and duplicating the function would
> be the end of the world either)

For fd_set there is only a single caller for each direction. Do you
prefer helpers even so?

For sigmask, with three callers, something along the lines of this?

  @@ -1138,10 +1135,7 @@ static int do_ppoll(struct pollfd __user
*ufds, unsigned int nfds,
                          return -EINVAL;
          }

  -       if (!in_compat_syscall())
  -               ret = set_user_sigmask(sigmask, sigsetsize);
  -       else
  -               ret = set_compat_user_sigmask(sigmask, sigsetsize);
  +       ret = set_maybe_compat_user_sigmask(sigmask, sigsetsize);
          if (ret)
                  return ret;

  --- a/include/linux/compat.h
  +++ b/include/linux/compat.h
  @@ -942,6 +942,17 @@ static inline bool in_compat_syscall(void) {
return false; }

  +static inline int set_maybe_compat_user_sigmask(const void __user *sigmask,
  +                                               size_t sigsetsize)
  +{
  +#if defined CONFIG_COMPAT
  +       if (unlikely(in_compat_syscall()))
  +               return set_compat_user_sigmask(sigmask, sigsetsize);
  +#endif
  +
  +       return set_user_sigmask(sigmask, sigsetsize);
  +}

Re: [PATCH v3 1/2] epoll: add nsec timeout support with epoll_pwait2

[Willem de Bruijn]

On Thu, Dec 10, 2020 at 5:59 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Thu, Dec 10, 2020 at 3:34 PM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > On Thu, Dec 10, 2020 at 6:33 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > > On Sat, Nov 21, 2020 at 4:27 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > > > On Fri, Nov 20, 2020 at 11:28 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> > > > I would imagine this can be done like the way I proposed
> > > > for get_bitmap() in sys_migrate_pages:
> > > >
> > > > https://lore.kernel.org/lkml/20201102123151.2860165-4-arnd@kernel.org/
> > >
> > > Coming back to this. Current patchset includes new select and poll
> > > selftests to verify the changes. I need to send a small kselftest
> > > patch for that first.
> > >
> > > Assuming there's no time pressure, I will finish up and send the main
> > > changes after the merge window, for the next release then.
> > >
> > > Current state against linux-next at
> > > https://github.com/wdebruij/linux-next-mirror/tree/select-compat-1
> >
> > Ok, sounds good to me. I've had a (very brief) look and have one
> > suggestion: instead of open-coding the compat vs native mode
> > in multiple places like
> >
> > if (!in_compat_syscall())
> >      return copy_from_user(fdset, ufdset, FDS_BYTES(nr)) ? -EFAULT : 0;
> > else
> >      return compat_get_bitmap(fdset, ufdset, nr);
> >
> > maybe move this into a separate function and call that where needed.
> >
> > I've done this for the get_bitmap() function in my series at
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/commit/?h=compat-alloc-user-space-7&id=b1b23ebb12b635654a2060df49455167a142c5d2
> >
> > The definition is slightly differrent for cpumask, nodemask and fd_set,
> > so we'd need to try out the best way to structure the code to end
> > up with the most readable version, but it should be possible when
> > there are only three callers (and duplicating the function would
> > be the end of the world either)
>
> For fd_set there is only a single caller for each direction. Do you
> prefer helpers even so?
>
> For sigmask, with three callers, something along the lines of this?
>
>   @@ -1138,10 +1135,7 @@ static int do_ppoll(struct pollfd __user
> *ufds, unsigned int nfds,
>                           return -EINVAL;
>           }
>
>   -       if (!in_compat_syscall())
>   -               ret = set_user_sigmask(sigmask, sigsetsize);
>   -       else
>   -               ret = set_compat_user_sigmask(sigmask, sigsetsize);
>   +       ret = set_maybe_compat_user_sigmask(sigmask, sigsetsize);
>           if (ret)
>                   return ret;
>
>   --- a/include/linux/compat.h
>   +++ b/include/linux/compat.h
>   @@ -942,6 +942,17 @@ static inline bool in_compat_syscall(void) {
> return false; }
>
>   +static inline int set_maybe_compat_user_sigmask(const void __user *sigmask,
>   +                                               size_t sigsetsize)
>   +{
>   +#if defined CONFIG_COMPAT
>   +       if (unlikely(in_compat_syscall()))
>   +               return set_compat_user_sigmask(sigmask, sigsetsize);
>   +#endif
>   +
>   +       return set_user_sigmask(sigmask, sigsetsize);
>   +}

set_user_sigmask is the only open-coded variant that is used more than once.

Because it is used in both select.c and eventpoll.c, a helper would
have to live in compat.h. This then needs a new dependency on
sched_signal.h.

So given that this is a simple branch, it might just make logic more
complex, instead of less. I can add this change in a separate patch on
top of the original three, to judge whether it is worthwhile.