Re: [PATCH net-next v3 1/4] net_tstamp: add SCM_TS_OPT_ID to provide OPT_ID in control message
From: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Date: 2024-09-05 08:34:50
On 05/09/2024 09:24, Jason Xing wrote:
quoted hunk ↗ jump to hunk
Hello Vadim, On Wed, Sep 4, 2024 at 7:32 PM Vadim Fedorenko [off-list ref] wrote: [...]quoted
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h index a2c66b3d7f0f..1c38536350e7 100644 --- a/include/uapi/linux/net_tstamp.h +++ b/include/uapi/linux/net_tstamp.h@@ -38,6 +38,13 @@ enum { SOF_TIMESTAMPING_LAST }; +/* + * 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 areaI wonder if we can add the above description which is quite useful in enum{} like this:diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h index a2c66b3d7f0f..2314fccaf51d 100644 --- a/include/uapi/linux/net_tstamp.h +++ b/include/uapi/linux/net_tstamp.h@@ -13,7 +13,12 @@ #include <linux/types.h> #include <linux/socket.h> /* for SO_TIMESTAMPING */ -/* SO_TIMESTAMPING flags */ +/* SO_TIMESTAMPING flags + * + * The highest bit of sk_tsflags is reserved for kernel-internal + * SOCKCM_FLAG_TS_OPT_ID. + * SOCKCM_FLAG_TS_OPT_ID = (1 << 31), + */ enum { SOF_TIMESTAMPING_TX_HARDWARE = (1<<0), SOF_TIMESTAMPING_TX_SOFTWARE = (1<<1),to explicitly remind the developers not to touch 1<<31 field. Or else, it can be very hard to trace who occupied the highest field in the future at the first glance, I think. [...]
That's a bit contradictory to Willem's comment about not exposing implementation details to UAPI headers, which I think makes sense. I will move the comment to the definition area of SOCKCM_FLAG_TS_OPT_ID and will try to add meaningful message to BUILD_BUG_ON() to make it easier for developers to understand the problem.
quoted
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index f26841f1490f..9b87d23314e8 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c@@ -1401,7 +1401,10 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork, cork->base.gso_size = ipc6->gso_size; cork->base.tx_flags = 0; cork->base.mark = ipc6->sockc.mark; + cork->base.ts_opt_id = ipc6->sockc.ts_opt_id; sock_tx_timestamp(sk, ipc6->sockc.tsflags, &cork->base.tx_flags); + if (ipc6->sockc.tsflags & SOCKCM_FLAG_TS_OPT_ID) + cork->base.flags |= IPCORK_TS_OPT_ID; cork->base.length = 0; cork->base.transmit_time = ipc6->sockc.transmit_time;@@ -1433,7 +1436,7 @@ static int __ip6_append_data(struct sock *sk, bool zc = false; u32 tskey = 0; struct rt6_info *rt = dst_rt6_info(cork->dst); - bool paged, hold_tskey, extra_uref = false; + bool paged, hold_tskey = false, extra_uref = false; struct ipv6_txoptions *opt = v6_cork->opt; int csummode = CHECKSUM_NONE; unsigned int maxnonfragsize, headersize;@@ -1543,10 +1546,15 @@ static int __ip6_append_data(struct sock *sk, flags &= ~MSG_SPLICE_PAGES; } - hold_tskey = cork->tx_flags & SKBTX_ANY_TSTAMP && - READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_OPT_ID; - if (hold_tskey) - tskey = atomic_inc_return(&sk->sk_tskey) - 1; + if (cork->tx_flags & SKBTX_ANY_TSTAMP && + READ_ONCE(sk->sk_tsflags) & SOCKCM_FLAG_TS_OPT_ID) {s/SOCKCM_FLAG_TS_OPT_ID/SOF_TIMESTAMPING_OPT_ID/ In case you forget to change here :)
Yeah, I've fixed this one already, but thanks!
quoted
+ if (cork->flags & IPCORK_TS_OPT_ID) { + tskey = cork->ts_opt_id; + } else { + tskey = atomic_inc_return(&sk->sk_tskey) - 1; + hold_tskey = true; + } + } /* * Let's try using as much space as possible. -- 2.43.5