Re: [Y2038] [PATCH v2 6/8] socket: Add SO_TIMESTAMP[NS]_NEW
> > 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 {
> }
>
> I think we can use the same helper for both the sock and tcp variant.
> The only intended difference between the two functions, as described
> in the tcp_recv_timestamp function comment, is the absence of an skb
> in the tcp case. That is immaterial at this level.
I will just not refactor things into a function: __sock_rescv_timestamp_new().
I will just add new conditionals for the new timestamps.
When you guys refactor the other timestamp stuff like you mentioned
below maybe you can move the new timestamps to a new funtcion as you
see fit.
The helper functions in skbuff.h might first need to be refactored
first. But I again leave this to you guys.
> 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);
> }
>
You could just ifdef CONFIG_64BIT if you are worried about branching.
Note that SO_TIMESTAMP is by default SO_TIMESTAMP_OLD on 64 bit machines.
But, I will again leave the optimization to you. I will implement in a
straight forward way and you guys can deicde how you want to optimize
the fast path or what should it even be.
-Deepa
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 6/8] socket: Add SO_TIMESTAMP[NS]_NEW
> 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.
> 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 {
}
I think we can use the same helper for both the sock and tcp variant.
The only intended difference between the two functions, as described
in the tcp_recv_timestamp function comment, is the absence of an skb
in the tcp case. That is immaterial at this level.
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'm trying to follow your request to keep code churn to minimal.
> It's just that I moved to a different function as that seemed logical
> to me. Do you prefer me to remove that refactoring?
Yes, please avoid rearranging existing code as much as possible.
If there is any refactoring to be done, I think it would be to
deduplicate the shared logic between __sock_recv_timestamp and
tcp_recv_timestamp. I think the first can be rewritten to reuse the
second, if the only difference really is that the first takes an skb with
embedded timestamps, while the second directly takes a pointer to
struct scm_timestamping.
Either way, that's out of scope for this patchset.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 6/8] socket: Add SO_TIMESTAMP[NS]_NEW
On Sat, Dec 15, 2018 at 7:12 AM Willem de Bruijn wrote: > > On Fri, Dec 14, 2018 at 8:07 PM Deepa Dinamani wrote: > > > > > > diff --git a/arch/alpha/include/uapi/asm/socket.h > > > > b/arch/alpha/include/uapi/asm/socket.h > > > > index 00e45c80e574..352e3dc0b3d9 100644 > > > > --- a/arch/alpha/include/uapi/asm/socket.h > > > > +++ b/arch/alpha/include/uapi/asm/socket.h > > > > @@ -3,6 +3,7 @@ > > > > #define _UAPI_ASM_SOCKET_H > > > > > > > > #include > > > > +#include > > > > > > > > /* For setsockopt(2) */ > > > > /* > > > > @@ -110,12 +111,22 @@ > > > > > > > > #define SO_TIMESTAMP_OLD 29 > > > > #define SO_TIMESTAMPNS_OLD 35 > > > > + > > > > #define SO_TIMESTAMPING_OLD 37 > > > > > > > > +#define SO_TIMESTAMP_NEW 62 > > > > +#define SO_TIMESTAMPNS_NEW 63 > > > > + > > > > #if !defined(__KERNEL__) > > > > > > > > -#define SO_TIMESTAMP SO_TIMESTAMP_OLD > > > > -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > > > > +#if __BITS_PER_LONG == 64 > > > > +#define SO_TIMESTAMP SO_TIMESTAMP_OLD > > > > +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > > > > +#else > > > > +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? > > > > SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) > > > > +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? > > > > SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) > > > > +#endif > > > > + > > > > > > This is not platform specific. Perhaps it can be deduplicated. The > > > interface expects callers to include , not > > > directly. So perhaps it can go there? > > > > I'm not following what you are saying here. > > > > Are you talking about in kernel users or userspace interface? > > > > Userspace should always include sys/socket.h according to the man page. > > I'm not sure if userspace can even include linux/socket.h directly. > > On my distribution this includes bits/socket.h which in turn includes > > asm/socket.h. > > I meant include/uapi/linux/socket.h. > > But you're right that that is not referenced from sys/socket.h. > > I do see a reference to it in my bits/socket.h > > /* Socket level message types. This must match the definitions in >. */ > > so perhaps the logic could be both there and in libc bits/socket.h. bits/socket.h cannot be included directly, and it's just how one of the libc implementations decided to do it. It doesn't even have to exist. > > Which file gets installed as asm/socket.h is defined per architecture > > in the kbuild file such as > > arch/ia64/include/uapi/asm/Kbuild (without series applied): > > > > generic-y += poll.h > > generic-y += sembuf.h > > generic-y += shmbuf.h > > generic-y += socket.h > > > > Also the new timestamp numbers being added are not the same for all > > architectures. > > > > So I'm not sure how this can be moved to linux/socket.h. > > Does that matter, as long as they are defined? This basic block is the > same between all archs: 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. Also for the other comment. The reason the conditionals were not consistent is because they were not consistent to begin with. I'm trying to follow your request to keep code churn to minimal. It's just that I moved to a different function as that seemed logical to me. Do you prefer me to remove that refactoring? -Deepa ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 6/8] socket: Add SO_TIMESTAMP[NS]_NEW
On Fri, Dec 14, 2018 at 8:07 PM Deepa Dinamani wrote: > > > > diff --git a/arch/alpha/include/uapi/asm/socket.h > > > b/arch/alpha/include/uapi/asm/socket.h > > > index 00e45c80e574..352e3dc0b3d9 100644 > > > --- a/arch/alpha/include/uapi/asm/socket.h > > > +++ b/arch/alpha/include/uapi/asm/socket.h > > > @@ -3,6 +3,7 @@ > > > #define _UAPI_ASM_SOCKET_H > > > > > > #include > > > +#include > > > > > > /* For setsockopt(2) */ > > > /* > > > @@ -110,12 +111,22 @@ > > > > > > #define SO_TIMESTAMP_OLD 29 > > > #define SO_TIMESTAMPNS_OLD 35 > > > + > > > #define SO_TIMESTAMPING_OLD 37 > > > > > > +#define SO_TIMESTAMP_NEW 62 > > > +#define SO_TIMESTAMPNS_NEW 63 > > > + > > > #if !defined(__KERNEL__) > > > > > > -#define SO_TIMESTAMP SO_TIMESTAMP_OLD > > > -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > > > +#if __BITS_PER_LONG == 64 > > > +#define SO_TIMESTAMP SO_TIMESTAMP_OLD > > > +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > > > +#else > > > +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? > > > SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) > > > +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? > > > SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) > > > +#endif > > > + > > > > This is not platform specific. Perhaps it can be deduplicated. The > > interface expects callers to include , not > > directly. So perhaps it can go there? > > I'm not following what you are saying here. > > Are you talking about in kernel users or userspace interface? > > Userspace should always include sys/socket.h according to the man page. > I'm not sure if userspace can even include linux/socket.h directly. > On my distribution this includes bits/socket.h which in turn includes > asm/socket.h. I meant include/uapi/linux/socket.h. But you're right that that is not referenced from sys/socket.h. I do see a reference to it in my bits/socket.h /* Socket level message types. This must match the definitions in . */ so perhaps the logic could be both there and in libc bits/socket.h. > Which file gets installed as asm/socket.h is defined per architecture > in the kbuild file such as > arch/ia64/include/uapi/asm/Kbuild (without series applied): > > generic-y += poll.h > generic-y += sembuf.h > generic-y += shmbuf.h > generic-y += socket.h > > Also the new timestamp numbers being added are not the same for all > architectures. > > So I'm not sure how this can be moved to linux/socket.h. Does that matter, as long as they are defined? This basic block is the same between all archs: -#define SO_TIMESTAMP SO_TIMESTAMP_OLD -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#if __BITS_PER_LONG == 64 +#define SO_TIMESTAMP SO_TIMESTAMP_OLD +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#else +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) +#endif It might be too complex to coordinate changes between kernel and libc headers, in which case you're right that this just has to live in (each arch's) asm/socket.h directly. ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
