Re: [RFC PATCH 02/13] extended semantic of sk_buff::tstamp: lowest bit marks hardware time stamps

[Eric Dumazet <dada1@cosmosbay.com>]

Patrick Ohly a écrit :
> If generated in hardware, then the driver must convert to system
> time before storing the transformed value in sk_buff with skb_hwtstamp_set().
> If conversion back to the original hardware time stamp is desired,
> then the driver needs to implement the hwtstamp_raw() callback, which
> is called by skb_hwtstamp_raw().
> 
> The purpose of the new skb_* methods is the hiding of how hardware
> time stamps are really stored. Later they might be stored in an extra
> field instead of mangling the existing tstamp.
> 
> Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
> ---
>  include/linux/netdevice.h |   12 +++++++
>  include/linux/skbuff.h    |   76 +++++++++++++++++++++++++++++++++++++++++++-
>  net/core/skbuff.c         |   32 +++++++++++++++++++
>  3 files changed, 118 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 488c56e..4da51cb 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -749,6 +749,18 @@ struct net_device
>  	/* for setting kernel sock attribute on TCP connection setup */
>  #define GSO_MAX_SIZE		65536
>  	unsigned int		gso_max_size;
> +
> +	/* hardware time stamping support */
> +#define HAVE_HW_TIME_STAMP
> +	/* Transforms skb->tstamp back to the original, raw hardware
> +	 * time stamp. The value must have been generated by the
> +	 * device. Implementing this is optional, but necessary for
> +	 * SO_TIMESTAMP_HARDWARE.
> +	 *
> +	 * Returns 1 if value could be retrieved, 0 otherwise.
> +	 */
> +	int                     (*hwtstamp_raw)(const struct sk_buff *skb,
> +						struct timespec *stamp);
>  };
>  #define to_net_dev(d) container_of(d, struct net_device, dev)
>  
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 9099237..0b3b36a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -199,7 +199,10 @@ typedef unsigned char *sk_buff_data_t;
>   *	@next: Next buffer in list
>   *	@prev: Previous buffer in list
>   *	@sk: Socket we are owned by
> - *	@tstamp: Time we arrived
> + *	@tstamp: Time we arrived: usually generated by ktime_get_real() and
> + *               thus is recorded in system time. If the lowest bit is set,
> + *               then the value was originally generated by a different clock
> + *               in the receiving hardware and then transformed to system time.
>   *	@dev: Device we arrived on/are leaving by
>   *	@transport_header: Transport layer header
>   *	@network_header: Network layer header
> @@ -1524,23 +1527,52 @@ static inline void skb_copy_to_linear_data_offset(struct sk_buff *skb,
>  
>  extern void skb_init(void);

Please use ktime_t instead of "union ktime"

>  
> +/** returns skb->tstamp without the bit which marks hardware time stamps */
> +static inline union ktime skb_get_ktime(const struct sk_buff *skb)
> +{
> +#if BITS_PER_LONG != 64 && !defined(CONFIG_KTIME_SCALAR)
> +	return ktime_set(skb->tstamp.tv.sec,
> +			skb->tstamp.tv.nsec & ~1);
> +#else
> +	return (ktime_t) { .tv64 = skb->tstamp.tv64 & ~1UL };
> +#endif
> +}
> +
>  /**
>   *	skb_get_timestamp - get timestamp from a skb
>   *	@skb: skb to get stamp from
>   *	@stamp: pointer to struct timeval to store stamp in
>   *
>   *	Timestamps are stored in the skb as offsets to a base timestamp.
> + *      The lowest bit is set if and only if the time stamp was originally
> + *      created by hardware when processing the packet.
> + *
>   *	This function converts the offset back to a struct timeval and stores
>   *	it in stamp.
>   */
>  static inline void skb_get_timestamp(const struct sk_buff *skb, struct timeval *stamp)
>  {
> -	*stamp = ktime_to_timeval(skb->tstamp);
> +	*stamp = ktime_to_timeval(skb_get_ktime(skb));
> +}
> +
> +static inline void skb_get_timestampns(const struct sk_buff *skb, struct timespec *stamp)
> +{
> +	*stamp = ktime_to_timespec(skb_get_ktime(skb));
>  }
>  
>  static inline void __net_timestamp(struct sk_buff *skb)
>  {
>  	skb->tstamp = ktime_get_real();
> +
> +	/*
> +	 * make sure that lowest bit is never set: it marks hardware
> +	 * time stamps
> +	 */
> +#if BITS_PER_LONG != 64 && !defined(CONFIG_KTIME_SCALAR)
> +	skb->tstamp.tv.sec = skb->tstamp.tv.sec / 2 * 2;

.tv.sec ? are you sure you dont want .tv.nsec ?

> +#else
> +	skb->tstamp.tv64 = skb->tstamp.tv64 / 2 * 2;
> +#endif
>  }
>