* [PATCH net-next] net: skb: free up one bit in tx_flags
@ 2025-02-21 3:58 Willem de Bruijn
2025-02-21 6:12 ` Jason Xing
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Willem de Bruijn @ 2025-02-21 3:58 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, edumazet, pabeni, kerneljasonxing, pav, gerhard,
vinicius.gomes, anthony.l.nguyen, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
The linked series wants to add skb tx completion timestamps.
That needs a bit in skb_shared_info.tx_flags, but all are in use.
A per-skb bit is only needed for features that are configured on a
per packet basis. Per socket features can be read from sk->sk_tsflags.
Per packet tsflags can be set in sendmsg using cmsg, but only those in
SOF_TIMESTAMPING_TX_RECORD_MASK.
Per packet tsflags can also be set without cmsg by sandwiching a
send inbetween two setsockopts:
val |= SOF_TIMESTAMPING_$FEATURE;
setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING, &val, sizeof(val));
write(fd, buf, sz);
val &= ~SOF_TIMESTAMPING_$FEATURE;
setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING, &val, sizeof(val));
Changing a datapath test from skb_shinfo(skb)->tx_flags to
skb->sk->sk_tsflags can change behavior in that case, as the tx_flags
is written before the second setsockopt updates sk_tsflags.
Therefore, only bits can be reclaimed that cannot be set by cmsg and
are also highly unlikely to be used to target individual packets
otherwise.
Free up the bit currently used for SKBTX_HW_TSTAMP_USE_CYCLES. This
selects between clock and free running counter source for HW TX
timestamps. It is probable that all packets of the same socket will
always use the same source.
Link: https://lore.kernel.org/netdev/cover.1739988644.git.pav@iki.fi/
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
drivers/net/ethernet/engleder/tsnep_main.c | 4 ++--
drivers/net/ethernet/intel/igc/igc_main.c | 3 ++-
include/linux/skbuff.h | 5 ++---
net/socket.c | 11 +----------
4 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 0d030cb0b21c..3de4cb06e266 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -852,8 +852,8 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
struct skb_shared_hwtstamps hwtstamps;
u64 timestamp;
- if (skb_shinfo(entry->skb)->tx_flags &
- SKBTX_HW_TSTAMP_USE_CYCLES)
+ if (entry->skb->sk &&
+ READ_ONCE(entry->skb->sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC)
timestamp =
__le64_to_cpu(entry->desc_wb->counter);
else
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 84307bb7313e..0c4216a4552b 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1650,7 +1650,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
if (igc_request_tx_tstamp(adapter, skb, &tstamp_flags)) {
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
tx_flags |= IGC_TX_FLAGS_TSTAMP | tstamp_flags;
- if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_USE_CYCLES)
+ if (skb->sk &&
+ READ_ONCE(skb->sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC)
tx_flags |= IGC_TX_FLAGS_TSTAMP_TIMER_1;
} else {
adapter->tx_hwtstamp_skipped++;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index bb2b751d274a..a65b2b08f994 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -478,8 +478,8 @@ enum {
/* device driver is going to provide hardware time stamp */
SKBTX_IN_PROGRESS = 1 << 2,
- /* generate hardware time stamp based on cycles if supported */
- SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,
+ /* reserved */
+ SKBTX_RESERVED = 1 << 3,
/* generate wifi status information (where possible) */
SKBTX_WIFI_STATUS = 1 << 4,
@@ -494,7 +494,6 @@ enum {
#define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \
SKBTX_SCHED_TSTAMP)
#define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \
- SKBTX_HW_TSTAMP_USE_CYCLES | \
SKBTX_ANY_SW_TSTAMP)
/* Definitions for flags in struct skb_shared_info */
diff --git a/net/socket.c b/net/socket.c
index 28bae5a94234..2e3e69710ea4 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -680,18 +680,9 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags)
{
u8 flags = *tx_flags;
- if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE) {
+ if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE)
flags |= SKBTX_HW_TSTAMP;
- /* PTP hardware clocks can provide a free running cycle counter
- * as a time base for virtual clocks. Tell driver to use the
- * free running cycle counter for timestamp if socket is bound
- * to virtual clock.
- */
- if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
- flags |= SKBTX_HW_TSTAMP_USE_CYCLES;
- }
-
if (tsflags & SOF_TIMESTAMPING_TX_SOFTWARE)
flags |= SKBTX_SW_TSTAMP;
--
2.48.1.658.g4767266eb4-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net-next] net: skb: free up one bit in tx_flags
2025-02-21 3:58 [PATCH net-next] net: skb: free up one bit in tx_flags Willem de Bruijn
@ 2025-02-21 6:12 ` Jason Xing
2025-02-21 21:11 ` Gerhard Engleder
2025-02-24 22:21 ` Jakub Kicinski
2 siblings, 0 replies; 4+ messages in thread
From: Jason Xing @ 2025-02-21 6:12 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev, davem, kuba, edumazet, pabeni, pav, gerhard,
vinicius.gomes, anthony.l.nguyen, Willem de Bruijn
On Fri, Feb 21, 2025 at 11:59 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.com>
>
> The linked series wants to add skb tx completion timestamps.
> That needs a bit in skb_shared_info.tx_flags, but all are in use.
>
> A per-skb bit is only needed for features that are configured on a
> per packet basis. Per socket features can be read from sk->sk_tsflags.
>
> Per packet tsflags can be set in sendmsg using cmsg, but only those in
> SOF_TIMESTAMPING_TX_RECORD_MASK.
>
> Per packet tsflags can also be set without cmsg by sandwiching a
> send inbetween two setsockopts:
>
> val |= SOF_TIMESTAMPING_$FEATURE;
> setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING, &val, sizeof(val));
> write(fd, buf, sz);
> val &= ~SOF_TIMESTAMPING_$FEATURE;
> setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING, &val, sizeof(val));
>
> Changing a datapath test from skb_shinfo(skb)->tx_flags to
> skb->sk->sk_tsflags can change behavior in that case, as the tx_flags
> is written before the second setsockopt updates sk_tsflags.
>
> Therefore, only bits can be reclaimed that cannot be set by cmsg and
> are also highly unlikely to be used to target individual packets
> otherwise.
Agreed.
>
> Free up the bit currently used for SKBTX_HW_TSTAMP_USE_CYCLES. This
> selects between clock and free running counter source for HW TX
> timestamps. It is probable that all packets of the same socket will
> always use the same source.
>
> Link: https://lore.kernel.org/netdev/cover.1739988644.git.pav@iki.fi/
> Signed-off-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
Thanks, Willem!
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] net: skb: free up one bit in tx_flags
2025-02-21 3:58 [PATCH net-next] net: skb: free up one bit in tx_flags Willem de Bruijn
2025-02-21 6:12 ` Jason Xing
@ 2025-02-21 21:11 ` Gerhard Engleder
2025-02-24 22:21 ` Jakub Kicinski
2 siblings, 0 replies; 4+ messages in thread
From: Gerhard Engleder @ 2025-02-21 21:11 UTC (permalink / raw)
To: Willem de Bruijn
Cc: davem, kuba, edumazet, pabeni, kerneljasonxing, pav,
vinicius.gomes, anthony.l.nguyen, Willem de Bruijn, netdev
On 21.02.25 04:58, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> The linked series wants to add skb tx completion timestamps.
> That needs a bit in skb_shared_info.tx_flags, but all are in use.
>
> A per-skb bit is only needed for features that are configured on a
> per packet basis. Per socket features can be read from sk->sk_tsflags.
>
> Per packet tsflags can be set in sendmsg using cmsg, but only those in
> SOF_TIMESTAMPING_TX_RECORD_MASK.
>
> Per packet tsflags can also be set without cmsg by sandwiching a
> send inbetween two setsockopts:
>
> val |= SOF_TIMESTAMPING_$FEATURE;
> setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING, &val, sizeof(val));
> write(fd, buf, sz);
> val &= ~SOF_TIMESTAMPING_$FEATURE;
> setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING, &val, sizeof(val));
>
> Changing a datapath test from skb_shinfo(skb)->tx_flags to
> skb->sk->sk_tsflags can change behavior in that case, as the tx_flags
> is written before the second setsockopt updates sk_tsflags.
>
> Therefore, only bits can be reclaimed that cannot be set by cmsg and
> are also highly unlikely to be used to target individual packets
> otherwise.
>
> Free up the bit currently used for SKBTX_HW_TSTAMP_USE_CYCLES. This
> selects between clock and free running counter source for HW TX
> timestamps. It is probable that all packets of the same socket will
> always use the same source.
For separate ptp4l instances with separate clocks this should be true.
> Link: https://lore.kernel.org/netdev/cover.1739988644.git.pav@iki.fi/
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> drivers/net/ethernet/engleder/tsnep_main.c | 4 ++--
> drivers/net/ethernet/intel/igc/igc_main.c | 3 ++-
> include/linux/skbuff.h | 5 ++---
> net/socket.c | 11 +----------
> 4 files changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
> index 0d030cb0b21c..3de4cb06e266 100644
> --- a/drivers/net/ethernet/engleder/tsnep_main.c
> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> @@ -852,8 +852,8 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
> struct skb_shared_hwtstamps hwtstamps;
> u64 timestamp;
>
> - if (skb_shinfo(entry->skb)->tx_flags &
> - SKBTX_HW_TSTAMP_USE_CYCLES)
> + if (entry->skb->sk &&
> + READ_ONCE(entry->skb->sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC)
> timestamp =
> __le64_to_cpu(entry->desc_wb->counter);
> else
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 84307bb7313e..0c4216a4552b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1650,7 +1650,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
> if (igc_request_tx_tstamp(adapter, skb, &tstamp_flags)) {
> skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> tx_flags |= IGC_TX_FLAGS_TSTAMP | tstamp_flags;
> - if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_USE_CYCLES)
> + if (skb->sk &&
> + READ_ONCE(skb->sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC)
> tx_flags |= IGC_TX_FLAGS_TSTAMP_TIMER_1;
> } else {
> adapter->tx_hwtstamp_skipped++;
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index bb2b751d274a..a65b2b08f994 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -478,8 +478,8 @@ enum {
> /* device driver is going to provide hardware time stamp */
> SKBTX_IN_PROGRESS = 1 << 2,
>
> - /* generate hardware time stamp based on cycles if supported */
> - SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,
> + /* reserved */
> + SKBTX_RESERVED = 1 << 3,
>
> /* generate wifi status information (where possible) */
> SKBTX_WIFI_STATUS = 1 << 4,
> @@ -494,7 +494,6 @@ enum {
> #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \
> SKBTX_SCHED_TSTAMP)
> #define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \
> - SKBTX_HW_TSTAMP_USE_CYCLES | \
> SKBTX_ANY_SW_TSTAMP)
>
> /* Definitions for flags in struct skb_shared_info */
> diff --git a/net/socket.c b/net/socket.c
> index 28bae5a94234..2e3e69710ea4 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -680,18 +680,9 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags)
> {
> u8 flags = *tx_flags;
>
> - if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE) {
> + if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE)
> flags |= SKBTX_HW_TSTAMP;
>
> - /* PTP hardware clocks can provide a free running cycle counter
> - * as a time base for virtual clocks. Tell driver to use the
> - * free running cycle counter for timestamp if socket is bound
> - * to virtual clock.
> - */
> - if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
> - flags |= SKBTX_HW_TSTAMP_USE_CYCLES;
> - }
> -
> if (tsflags & SOF_TIMESTAMPING_TX_SOFTWARE)
> flags |= SKBTX_SW_TSTAMP;
>
Reviewed-by: Gerhard Engleder <gerhard@engleder-embedded.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next] net: skb: free up one bit in tx_flags
2025-02-21 3:58 [PATCH net-next] net: skb: free up one bit in tx_flags Willem de Bruijn
2025-02-21 6:12 ` Jason Xing
2025-02-21 21:11 ` Gerhard Engleder
@ 2025-02-24 22:21 ` Jakub Kicinski
2 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2025-02-24 22:21 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev, davem, edumazet, pabeni, kerneljasonxing, pav, gerhard,
vinicius.gomes, anthony.l.nguyen, Willem de Bruijn
On Thu, 20 Feb 2025 22:58:20 -0500 Willem de Bruijn wrote:
> The linked series wants to add skb tx completion timestamps.
> That needs a bit in skb_shared_info.tx_flags, but all are in use.
>
> A per-skb bit is only needed for features that are configured on a
> per packet basis. Per socket features can be read from sk->sk_tsflags.
>
> Per packet tsflags can be set in sendmsg using cmsg, but only those in
> SOF_TIMESTAMPING_TX_RECORD_MASK.
>
> Per packet tsflags can also be set without cmsg by sandwiching a
> send inbetween two setsockopts:
>
> val |= SOF_TIMESTAMPING_$FEATURE;
> setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING, &val, sizeof(val));
> write(fd, buf, sz);
> val &= ~SOF_TIMESTAMPING_$FEATURE;
> setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING, &val, sizeof(val));
>
> Changing a datapath test from skb_shinfo(skb)->tx_flags to
> skb->sk->sk_tsflags can change behavior in that case, as the tx_flags
> is written before the second setsockopt updates sk_tsflags.
>
> Therefore, only bits can be reclaimed that cannot be set by cmsg and
> are also highly unlikely to be used to target individual packets
> otherwise.
>
> Free up the bit currently used for SKBTX_HW_TSTAMP_USE_CYCLES. This
> selects between clock and free running counter source for HW TX
> timestamps. It is probable that all packets of the same socket will
> always use the same source.
This needs a respin, presumably because Jason's series reached net-next
already.
--
pw-bot: cr
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-02-24 22:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-21 3:58 [PATCH net-next] net: skb: free up one bit in tx_flags Willem de Bruijn
2025-02-21 6:12 ` Jason Xing
2025-02-21 21:11 ` Gerhard Engleder
2025-02-24 22:21 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).