On Wednesday, 24 January 2024 13:52:52 CST Arnd Bergmann wrote:
On Wed, Jan 24, 2024, at 19:02, Elizabeth Figura wrote:
On Wednesday, 24 January 2024 01:56:52 CST Arnd Bergmann wrote:
On Wed, Jan 24, 2024, at 01:40, Elizabeth Figura wrote:
- if (args->timeout) {
struct timespec64 to;
if (get_timespec64(&to, u64_to_user_ptr(args->timeout)))
return -EFAULT;
if (!timespec64_valid(&to))
return -EINVAL;
timeout = timespec64_to_ns(&to);
- }
Have you considered just passing the nanosecond value here? Since you do not appear to write it back, that would avoid the complexities of dealing with timespec layout differences and indirection.
That'd be nicer in general. I think there was some documentation that advised using timespec64 for new ioctl interfaces but it may have been outdated or misread.
It's probably something I wrote. It depends a bit on whether you have an absolute or relative timeout. If the timeout is relative to the current time as I understand it is here, a 64-bit number seems more logical to me.
For absolute times, I would usually use a __kernel_timespec, especially if it's CLOCK_REALTIME. In this case you would also need to specify the time domain.
Currently the interface does pass it as an absolute time, with the domain implicitly being MONOTONIC. This particular choice comes from process/botching-up-ioctls.rst, which is admittedly focused around GPU ioctls, but the rationale of having easily restartable ioctls applies here too.
(E.g. Wine does play games with signals, so we do want to be able to interrupt arbitrary waits with EINTR. The "usual" fast path for ntsync waits won't hit that, but we want to have it work.)
On the other hand, if we can pass the timeout as relative, and write it back on exit like ppoll() does [assuming that's not proscribed], that would presumably be slightly better for performance.
When writing the patch I just picked the recommended option, and didn't bother doing any micro-optimizations afterward.
What's the rationale for using timespec for absolute or written-back timeouts, instead of dealing in ns directly? I'm afraid it's not obvious to me.
--Zeb