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@xxxxxxxxxx> wrote:
>
> On Thu, Nov 19, 2020 at 9:13 PM Willem de Bruijn
> <willemdebruijn.kernel@xxxxxxxxx> wrote:
> > On Thu, Nov 19, 2020 at 10:45 AM Arnd Bergmann <arnd@xxxxxxxxxx> wrote:
> > > On Thu, Nov 19, 2020 at 3:31 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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.