Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sergey Organov <sorganov@gmail.com>
To:     John Stultz <john.stultz@linaro.org>
Cc:     Eugene Syromiatnikov <esyr@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Stephen Boyd <sboyd@kernel.org>,
        Frederic Weisbecker <frederic@kernel.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] uapi, posix-timers: provide clockid-related macros
 and functions to UAPI
References: <20190923130531.GA27774@asgard.redhat.com>
        <87y2pxvsbr.fsf@osv.gnss.ru> <20200512223103.GC1016@asgard.redhat.com>
        <CALAqxLUGr=+UjYhQSN34fUMCqj1Ya85tbFKu685cN+XTWYfgXg@mail.gmail.com>
        <8736845ha6.fsf@osv.gnss.ru>
        <CALAqxLWkQmiFOCY=YhQLCJHwGHCO9kfZzysCEDLzJFTvgapUwA@mail.gmail.com>
Date:   Wed, 13 May 2020 23:28:48 +0300
In-Reply-To: <CALAqxLWkQmiFOCY=YhQLCJHwGHCO9kfZzysCEDLzJFTvgapUwA@mail.gmail.com>
        (John Stultz's message of "Wed, 13 May 2020 10:11:32 -0700")
Message-ID: <87y2pvy3y7.fsf@osv.gnss.ru>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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