diff mbox series

[v2,17/31] ntsync: Allow waits to use the REALTIME clock.

Message ID 20240219223833.95710-18-zfigura@codeweavers.com
State New
Headers
Series NT synchronization primitive driver |

Commit Message

Elizabeth Figura Feb. 19, 2024, 10:38 p.m. UTC 
  NtWaitForMultipleObjects() can receive a timeout in two forms, relative or
absolute. Relative timeouts are unaffected by changes to the system time and do
not count down while the system suspends; for absolute timeouts the opposite is
true.

In order to make the interface and implementation simpler, the ntsync driver
only deals in absolute timeouts. However, we need to be able to emulate both
behaviours apropos suspension and time adjustment, which is achieved by allowing
either the MONOTONIC or REALTIME clock to be used.

Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
 drivers/misc/ntsync.c       | 9 ++++++++-
 include/uapi/linux/ntsync.h | 4 ++++
 2 files changed, 12 insertions(+), 1 deletion(-)

Comments

Arnd Bergmann Feb. 20, 2024, 7:01 a.m. UTC | #1 
On Mon, Feb 19, 2024, at 23:38, Elizabeth Figura wrote:
> NtWaitForMultipleObjects() can receive a timeout in two forms, relative or
> absolute. Relative timeouts are unaffected by changes to the system time and do
> not count down while the system suspends; for absolute timeouts the opposite is
> true.
>
> In order to make the interface and implementation simpler, the ntsync driver
> only deals in absolute timeouts. However, we need to be able to emulate both
> behaviours apropos suspension and time adjustment, which is achieved by allowing
> either the MONOTONIC or REALTIME clock to be used.
>
> Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>

I understand that there is no practical problem in building
up the API one patch at a time in the initial merge, but
it still feels wrong to have an incompatible ABI change in
the middle of the series:

> @@ -35,6 +37,8 @@ struct ntsync_wait_args {
>  	__u32 owner;
>  	__u32 index;
>  	__u32 alert;
> +	__u32 flags;
> +	__u32 pad;
>  };

If this was patch to get merged at any later point, you'd have
to support both the shorter and the longer structure layout
with their distinct ioctl command codes.

If you do a v3 series, maybe just merge this patch into the
one that introduces the struct ntsync_wait_args. Overall,
you could probably have fewer but larger patches anyway
without harming the review process, but other than this
one that is not a problem.

       Arnd
Elizabeth Figura Feb. 22, 2024, 12:48 a.m. UTC | #2 
On Tuesday, 20 February 2024 01:01:59 CST Arnd Bergmann wrote:
> On Mon, Feb 19, 2024, at 23:38, Elizabeth Figura wrote:
> > NtWaitForMultipleObjects() can receive a timeout in two forms, relative or
> > absolute. Relative timeouts are unaffected by changes to the system time and do
> > not count down while the system suspends; for absolute timeouts the opposite is
> > true.
> >
> > In order to make the interface and implementation simpler, the ntsync driver
> > only deals in absolute timeouts. However, we need to be able to emulate both
> > behaviours apropos suspension and time adjustment, which is achieved by allowing
> > either the MONOTONIC or REALTIME clock to be used.
> >
> > Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
> 
> I understand that there is no practical problem in building
> up the API one patch at a time in the initial merge, but
> it still feels wrong to have an incompatible ABI change in
> the middle of the series:
> 
> > @@ -35,6 +37,8 @@ struct ntsync_wait_args {
> >  	__u32 owner;
> >  	__u32 index;
> >  	__u32 alert;
> > +	__u32 flags;
> > +	__u32 pad;
> >  };
> 
> If this was patch to get merged at any later point, you'd have
> to support both the shorter and the longer structure layout
> with their distinct ioctl command codes.
> 
> If you do a v3 series, maybe just merge this patch into the
> one that introduces the struct ntsync_wait_args. Overall,
> you could probably have fewer but larger patches anyway
> without harming the review process, but other than this
> one that is not a problem.

Oops, yes, that does feel wrong now that you point it out.

I'll squash this in v3, assuming there's a need for one.

--Zeb
diff mbox series

Patch

diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index 0055b4671808..f54c81dada3d 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -778,11 +778,15 @@  static void put_obj(struct ntsync_obj *obj)
 static int ntsync_schedule(const struct ntsync_q *q, const struct ntsync_wait_args *args)
 {
 	ktime_t timeout = ns_to_ktime(args->timeout);
+	clockid_t clock = CLOCK_MONOTONIC;
 	ktime_t *timeout_ptr;
 	int ret = 0;
 
 	timeout_ptr = (args->timeout == U64_MAX ? NULL : &timeout);
 
+	if (args->flags & NTSYNC_WAIT_REALTIME)
+		clock = CLOCK_REALTIME;
+
 	do {
 		if (signal_pending(current)) {
 			ret = -ERESTARTSYS;
@@ -794,7 +798,7 @@  static int ntsync_schedule(const struct ntsync_q *q, const struct ntsync_wait_ar
 			ret = 0;
 			break;
 		}
-		ret = schedule_hrtimeout(timeout_ptr, HRTIMER_MODE_ABS);
+		ret = schedule_hrtimeout_range_clock(timeout_ptr, 0, HRTIMER_MODE_ABS, clock);
 	} while (ret < 0);
 	__set_current_state(TASK_RUNNING);
 
@@ -817,6 +821,9 @@  static int setup_wait(struct ntsync_device *dev,
 	if (!args->owner)
 		return -EINVAL;
 
+	if (args->pad || (args->flags & ~NTSYNC_WAIT_REALTIME))
+		return -EINVAL;
+
 	if (args->count > NTSYNC_MAX_WAIT_COUNT)
 		return -EINVAL;
 
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
index 555ae81b479a..b5e835d8dba8 100644
--- a/include/uapi/linux/ntsync.h
+++ b/include/uapi/linux/ntsync.h
@@ -28,6 +28,8 @@  struct ntsync_event_args {
 	__u32 signaled;
 };
 
+#define NTSYNC_WAIT_REALTIME	0x1
+
 struct ntsync_wait_args {
 	__u64 timeout;
 	__u64 objs;
@@ -35,6 +37,8 @@  struct ntsync_wait_args {
 	__u32 owner;
 	__u32 index;
 	__u32 alert;
+	__u32 flags;
+	__u32 pad;
 };
 
 #define NTSYNC_MAX_WAIT_COUNT 64