Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180712082609.GB8802@infradead.org>
References: <20180707054247.19802-1-deepa.kernel@gmail.com>
 <20180707054247.19802-6-deepa.kernel@gmail.com> <20180712082609.GB8802@infradead.org>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Thu, 12 Jul 2018 16:53:23 +0200
Message-ID: <CAK8P3a02MJt7xDR3-u4r7GGaUc-ndnkq8Hb_ZC7YB6mkNqmeLw@mail.gmail.com>
Subject: Re: [PATCH v3 5/7] time: Add struct __kernel_timex
To:     Christoph Hellwig <hch@infradead.org>
Cc:     Deepa Dinamani <deepa.kernel@gmail.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        y2038 Mailman List <y2038@lists.linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 12, 2018 at 10:26 AM, Christoph Hellwig <hch@infradead.org> wrote:

>> +#ifndef CONFIG_64BIT_TIME
>> +#define __kernel_timex timex
>> +#endif
>
> using #defines for structs has all kinds of ill effects.  Why can't
> we aways use __kernel_timex for the in-kernel usage?

It's the same thing that Deepa did in the already merged
series for the timespec interfaces, this is done in order to
stage the conversion of 50 system calls and 20 architectures
without having to do everything at once.

The system call entry points are changed to take a __kernel_timespec
or __kernel_timex argument as the first step, and the conditional
macro override makes that a NOP. At the same time, the existing
compat entry points are modified so they have the exact same
behavior but use the compat_timespec, compat_timex etc
argument types (which we can rename, too, as discussed).

When a 32-bit architecture gets converted, it sets
CONFIG_64BIT_TIME, which flips the normal syscall entry to
use 64-bit time_t, and enables the compat syscalls to be built.
The same arch specific patch must then change the syscall
table to redirect the old syscall numbers to the compat handlers
(which implement the 32-bit time_t arguments), and add new
numbers pointing to the default handlers that now take the
64-bit time_t, see https://pastebin.com/F3QZdyin

This was all explained in a lot of detail when the first set
of patches got merged for the clock_ syscalls, but I suppose
it can all be explained again.

>> +#ifndef __kernel_timex
>> +struct __kernel_timex {
>> +     unsigned int modes;     /* mode selector */
>> +     int :32;            /* pad */
>
> Why do we need padding for a purely in-kernel structure?
>
> Also the anonymous member syntax is rather odd and I don't remeber
> us using it anywhere else.  Why here?

This is in the uapi header, and the structure just matches the
existing timex definition, which has the same oddity.

     Arnd