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

Eric Dumazet
2008-11-12T07:41:48Z

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
> }
>