Re: [PATCH v2] uapi, posix-timers: provide clockid-related macros and functions to UAPI

[Sergey Organov <sorganov@gmail.com>]

John Stultz <john.stultz@linaro.org> writes:

> On Wed, May 13, 2020 at 2:13 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> John Stultz <john.stultz@linaro.org> writes:
>>
>> > On Tue, May 12, 2020 at 3:31 PM Eugene Syromiatnikov
>> > <esyr@redhat.com> wrote:
>> >> On Tue, May 12, 2020 at 10:58:16PM +0300, Sergey Organov wrote:
>> >> > Eugene Syromiatnikov <esyr@redhat.com> writes:
>> >> >
>> >> > > As of now, there is no interface exposed for converting pid/fd into
>> >> > > clockid and vice versa; linuxptp, for example, has been carrying these
>> >> > > definitions in missing.h header for quite some time[1].
>> >> > >
>> >> > > [1] https://sourceforge.net/p/linuxptp/code/ci/af380e86/tree/missing.h
>> >> > >
>> >> > > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
>> >> > > ---
>> >> > > Changes since v1[1]:
>> >> > >  * Actually tried to build with the patch and fixed the build error
>> >> > >    reported by kbuild test robot[2].
>> >> > >
>> >> > > [1] https://lkml.org/lkml/2019/9/20/698
>> >> > > [2] https://lkml.org/lkml/2019/9/22/13
>> >> > > ---
>> >> > >  include/linux/posix-timers.h | 47 +------------------------------------------
>> >> > >  include/uapi/linux/time.h    | 48 ++++++++++++++++++++++++++++++++++++++++++++
>> >> > >  2 files changed, 49 insertions(+), 46 deletions(-)
>> >> >
>> >> > Was this patch applied, rejected, lost?
>> >> >
>> >> > I can't find it in the current master.
>> >>
>> >> IIRC, it was ignored.
>> >
>> > Overlooked. :)  Not intentionally ignored.
>> >
>> > I don't have any major objection with adding helpers, though I feel
>> > like you're exporting a lot more to the uapi then applications likely
>> > need.
>> >
>> > Would it be better to add just the bits from the missing.h header you
>> > pointed to:
>> > #define CLOCKFD 3
>> > #define FD_TO_CLOCKID(fd) ((~(clockid_t) (fd) << 3) | CLOCKFD)
>> > #define CLOCKID_TO_FD(clk) ((unsigned int) ~((clk) >> 3))
>> >
>> >  to the uapi header?
>>
>> Please, no:
>>
>> 1. These macros were copied almost verbatim from the kernel code long
>> ago, and since then kernel has changed them to inline functions, so
>> getting back to these obsolete macros is pointless.
>>
>> 2. If we do need to export macroses, then kernel inline functions are
>> better to be re-implemented in terms of these macros, not to have 2
>> different points of implementation.
>>
>> Overall, I'd vote for the current approach of the patch, provided
>> exporting inline functions to user-space is allowed.
>
> Sure, I just want to make sure we're only exporting the minimal
> necessary amount of details to userland. The current patch is
> exporting a bit more than that.

From userland POV, I've only seen 2 above conversions to be used,
and I have absolutely no idea if the other 2 functions:

static inline __kernel_clockid_t make_process_cpuclock(const unsigned int pid,
static inline __kernel_clockid_t make_thread_cpuclock(const unsigned int tid,

are directly useful from userspace.

Then, I now realize that exporting defines could be a better idea as
their existence could be easily checked from userspace.

However, exporting exactly these defines would likely break existing
userspace due to redefinition, so we probably need to come-up with
different names then.

And then it's probably C library that ideally should provide
corresponding interface to user programs, so there is yet another layer
to be considered?

Personally, I don't feel being experienced enough in kernel-to-userspace
interface subtleties to suggest proper patch that'd expose minimum
details yet doesn't create too much of maintenance burden both in the
kernel and userspace.

Thanks,
-- Sergey