Re: [PATCH net-next v4 1/3] net_tstamp: add SCM_TS_OPT_ID to provide OPT_ID in control message

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Vadim Fedorenko wrote:
> SOF_TIMESTAMPING_OPT_ID socket option flag gives a way to correlate TX
> timestamps and packets sent via socket. Unfortunately, there is no way
> to reliably predict socket timestamp ID value in case of error returned
> by sendmsg. For UDP sockets it's impossible because of lockless
> nature of UDP transmit, several threads may send packets in parallel. In
> case of RAW sockets MSG_MORE option makes things complicated. More
> details are in the conversation [1].
> This patch adds new control message type to give user-space
> software an opportunity to control the mapping between packets and
> values by providing ID with each sendmsg for UDP sockets.
> The documentation is also added in this patch.
> 
> [1] https://lore.kernel.org/netdev/CALCETrU0jB+kg0mhV6A8mrHfTE1D1pr1SD_B9Eaa9aDPfgHdtA@mail.gmail.com/
> 
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Two small points if have to respin for other reasons.

> +++ b/include/net/sock.h
> @@ -952,6 +952,12 @@ enum sock_flags {
>  };
>  
>  #define SK_FLAGS_TIMESTAMP ((1UL << SOCK_TIMESTAMP) | (1UL << SOCK_TIMESTAMPING_RX_SOFTWARE))
> +#define SOCKCM_FLAG_TS_OPT_ID	BIT(31)
> +/*
> + * The highest bit of sk_tsflags is reserved for kernel-internal
> + * SOCKCM_FLAG_TS_OPT_ID. This check is to control that SOF_TIMESTAMPING*
> + * values do not reach this reserved area
> + */

nit: odd to add comment below statement. Also, there is no check here.

>  
>  static inline void sock_copy_flags(struct sock *nsk, const struct sock *osk)
>  {
> @@ -1794,6 +1800,7 @@ struct sockcm_cookie {
>  	u64 transmit_time;
>  	u32 mark;
>  	u32 tsflags;
> +	u32 ts_opt_id;
>  };
>  
>  static inline void sockcm_init(struct sockcm_cookie *sockc,
> diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
> index 8ce8a39a1e5f..db3df3e74b01 100644
> --- a/include/uapi/asm-generic/socket.h
> +++ b/include/uapi/asm-generic/socket.h
> @@ -135,6 +135,8 @@
>  #define SO_PASSPIDFD		76
>  #define SO_PEERPIDFD		77
>  
> +#define SCM_TS_OPT_ID		78
> +
>  #if !defined(__KERNEL__)
>  
>  #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 468b1239606c..d2a01eaf6731 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2831,6 +2831,9 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
>  {
>  	u32 tsflags;
>  
> +	BUILD_BUG_ON_MSG(SOF_TIMESTAMPING_LAST == (1 << 31),
> +			 "SOF_TIMESTAMPING* cannot use BIT(31) because it's reserved for SOCKCM_FLAG_TS_OPT_ID");
> +

nit: No need for song a long line. Regular BUILD_BUG_ON will do.