Re: [PATCH bpf-next v7 05/13] net-timestamp: prepare for isolating two modes of SO_TIMESTAMPING

[Martin KaFai Lau <martin.lau@linux.dev>]

On 1/28/25 12:46 AM, Jason Xing wrote:
> No functional changes here. I add skb_enable_app_tstamp() to test
> if the orig_skb matches the usage of application SO_TIMESTAMPING
> and skb_sw_tstamp_tx() to distinguish the software and hardware

There is no skb_sw_tstamp_tx() in the code. An outdated commit message?

> timestamp when tsflag is SCM_TSTAMP_SND.
> 
> Also, I deliberately distinguish the the software and hardware
> SCM_TSTAMP_SND timestamp by passing 'sw' parameter in order to
> avoid such a case where hardware may go wrong and pass a NULL
> hwstamps, which is even though unlikely to happen. If it really
> happens, bpf prog will finally consider it as a software timestamp.
> It will be hardly recognized. Let's make the timestamping part
> more robust.
> 
> After this patch, I will soon add checks about bpf SO_TIMESTAMPING.

This needs to be updated also. BPF does not use the SO_TIMESTAMPING socket option.

> In this way, we can support two modes parallelly.

s/parallely/in parallel/

> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>   include/linux/skbuff.h | 13 +++++++------
>   net/core/dev.c         |  2 +-
>   net/core/skbuff.c      | 32 ++++++++++++++++++++++++++++++--
>   net/ipv4/tcp_input.c   |  3 ++-
>   4 files changed, 40 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index bb2b751d274a..dfc419281cc9 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -39,6 +39,7 @@
>   #include <net/net_debug.h>
>   #include <net/dropreason-core.h>
>   #include <net/netmem.h>
> +#include <uapi/linux/errqueue.h>
>   
>   /**
>    * DOC: skb checksums
> @@ -4533,18 +4534,18 @@ void skb_complete_tx_timestamp(struct sk_buff *skb,
>   
>   void __skb_tstamp_tx(struct sk_buff *orig_skb, const struct sk_buff *ack_skb,
>   		     struct skb_shared_hwtstamps *hwtstamps,
> -		     struct sock *sk, int tstype);
> +		     struct sock *sk, bool sw, int tstype);
>   
>   /**
> - * skb_tstamp_tx - queue clone of skb with send time stamps
> + * skb_tstamp_tx - queue clone of skb with send HARDWARE timestamps
>    * @orig_skb:	the original outgoing packet
>    * @hwtstamps:	hardware time stamps, may be NULL if not available
>    *
>    * If the skb has a socket associated, then this function clones the
>    * skb (thus sharing the actual data and optional structures), stores
> - * the optional hardware time stamping information (if non NULL) or
> - * generates a software time stamp (otherwise), then queues the clone

This line is removed. Does it mean no software timestamp now after this change?

> - * to the error queue of the socket.  Errors are silently ignored.
> + * the optional hardware time stamping information (if non NULL) then
> + * queues the clone to the error queue of the socket.  Errors are
> + * silently ignored.
>    */
>   void skb_tstamp_tx(struct sk_buff *orig_skb,
>   		   struct skb_shared_hwtstamps *hwtstamps);
> @@ -4565,7 +4566,7 @@ static inline void skb_tx_timestamp(struct sk_buff *skb)
>   {
>   	skb_clone_tx_timestamp(skb);
>   	if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
> -		skb_tstamp_tx(skb, NULL);
> +		__skb_tstamp_tx(skb, NULL, NULL, skb->sk, true, SCM_TSTAMP_SND);
>   }