Re: [PATCH net v1 3/3] tcp: use skb_get_hwtstamp() for hardware timestamps

[Kohei Enju <kohei@enjuk.jp>]

On 04/29 17:09, Willem de Bruijn wrote:
> Kohei Enju wrote:
> > Since commit 97dc7cd92ac6 ("ptp: Support late timestamp determination"),
> > skb_shared_hwtstamps may contain netdev_data instead of hwtstamp. TCP
> > receive timestamping can then interpret the stored value as a ktime_t
> > and report bogus hardware timestamps to userspace.
> > 
> > Use skb_get_hwtstamp() instead of reading hwtstamp directly, so TCP
> > sockets follow the same hardware timestamp resolution path as the socket
> > layer. When coalescing SKBs, resolve late timestamps before copying them
> > to the merged skb.
> 
> Why? Does this preclude supporting SOF_TIMESTAMPING_BIND_PHC for such
> sockets at a later time?

You're right. If we want to keep the door open for
SOF_TIMESTAMPING_BIND_PHC on coalesced skbs, resolving the timestamp at
coalescing time is the wrong approach.

> 
> It is just as easy to coalesce the cookie as the htwtstamp.
> 
> That also avoids the need to mask out SKBTX_HW_TSTAMP_NETDEV.

Indeed. We should also carry the matching @napi_id, since
skb_get_hwtstamp() uses it for late resolution.

I'll update this in v2.

> 
> > Additionally, recognize SKBTX_HW_TSTAMP_NETDEV as
> > indicating a receive timestamp is present.
> 
> What is the reason for this extra condition?

With late timestamp determination, skb_shared_hwtstamps may hold
netdev_data instead of a resolved hwtstamp, and 0 can be a valid cookie
value. So checking only skb_hwtstamps(skb)->hwtstamp is not enough to
detect the presence of an RX hardware timestamp.

> 
> > Note that skb_get_hwtstamp() is called with cycles == false, since TCP
> > hasn't honored SOF_TIMESTAMPING_BIND_PHC so far, and this patch doesn't
> > change that behavior.
> > 
> > Fixes: 97dc7cd92ac6 ("ptp: Support late timestamp determination")
> > Signed-off-by: Kohei Enju <kohei@enjuk.jp>
> > ---
[...]