On Tue, Dec 18, 2018 at 8:33 AM Arnd Bergmann [off-list ref] wrote:

On Sat, Dec 15, 2018 at 7:52 PM Willem de Bruijn
[off-list ref] wrote:

quoted

quoted

3 reasons for not doing this:

1. We do not want to break userspace. If we move this to
linux/socket.h all the userspace programs now have to include
linux/socket.h or get this definition through a new libc.
2. All the socket options are together in the file asm/socket.h. It
doesn't seem good for maintainability to move just a few bits
elsewhere.
3. There are only 4 arches (after the series is applied) that have
their own asm/socket.h. And, this is because there seems to be
significant differences to asm-generic/socket.h that don't seem
logically obvious to group and eliminate some of the defines.

Agreed. All good reasons to leave as is.

quoted

Also for the other comment. The reason the conditionals were not
consistent is because they were not consistent to begin with.

The only difference I see is an inversion of the test. Nesting order
is the same:

        int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP);
        ...
        if (need_software_tstamp) {
                if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) {
                } else {
                }
        }

vs

                if (sock_flag(sk, SOCK_RCVTSTAMP)) {
                        if (sock_flag(sk, SOCK_RCVTSTAMPNS)) {
                        } else {
                        }
                }

I suggest just adding something like

        if (need_software_tstamp) {
+              if (sock_uses_new_tstamp(sk) {
+                   __sock_recv_timestamp_new(msg, sk,
ktime_to_timespec64(skb->tstamp));
+              } else if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) {
-               if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) {
                } else {
                }

and

                if (sock_flag(sk, SOCK_RCVTSTAMP)) {
+                      if (sock_uses_new_tstamp(sk) {
+                           __sock_recv_timestamp_new(msg, sk, ts);
+                      else if (sock_flag(sk, SOCK_RCVTSTAMPNS)) {
-                       if (sock_flag(sk, SOCK_RCVTSTAMPNS)) {
                        } else {
                        }

Generally speaking, I think we want the new time handling
to be written as the default case rather than have it hidden away
in a separate function. If we didn't have the sparc64 quirk with its
unusual timeval definition, we'd only need a special flag for the
old 32-bit format, but that doesn't work as long we have to support
two different 64-bit formats for 64-bit timeval on sparc64
(32 or 64 bit microseconds).

quoted

Note also (2) tentative helper function sock_uses_new_tstamp(const
struct sock *sk) instead of testing sock_flag(sk, SOCK_TSTAMP_NEW)
directly. Since the .._NEW variants are equivalent to .._OLD on 64-bit,
I wonder if we can just compile out the branch. Something like

    static inline bool sock_uses_new_tstamp(const struct sock *sk) {
            return (sizeof(time_t) != sizeof(__kernel_long_t)) &&
                       sock_flag(sk, SOCK_TSTAMP_NEW);
    }

I think that would break compat handling: when we have a 32-bit
user space process, the difference between old and new timestamps
is meaningful even on 64-bit kernels, but the distinction is only made all
the way down in put_cmsg_compat().

As I mentioned previously, I have refrained from adding these
optimizations for now.
The old timestamps are as is and the new timestamps are not yet being
used anywhere as we have not switched any of the architectures to use
y2038 syscalls and data structures yet.
So even if these optimizations are needed these can be added as
separate patches.
Let me know if this is acceptable for everyone and I can post the update.

-Deepa