Vladimir Oltean <olte...@gmail.com> writes:
> From: Vladimir Oltean <vladimir.olt...@nxp.com>
>
> The txtime is passed to the driver in skb->skb_mstamp_ns, which is
> actually in a union with skb->tstamp (the place where software
> timestamps are kept).
>
> Since commit b50a5c70ffa4 ("net: allow simultaneous SW and HW transmit
> timestamping"), __sock_recv_timestamp has some logic for making sure
> that the two calls to skb_tstamp_tx:
>
> skb_tx_timestamp(skb) # Software timestamp in the driver
> -> skb_tstamp_tx(skb, NULL)
>
> and
>
> skb_tstamp_tx(skb, &shhwtstamps) # Hardware timestamp in the driver
>
> will both do the right thing and in a race-free manner, meaning that
> skb_tx_timestamp will deliver a cmsg with the software timestamp only,
> and skb_tstamp_tx with a non-NULL hwtstamps argument will deliver a cmsg
> with the hardware timestamp only.
>
> Why are races even possible? Well, because although the software timestamp
> skb->tstamp is private per skb, the hardware timestamp skb_hwtstamps(skb)
> lives in skb_shinfo(skb), an area which is shared between skbs and their
> clones. And skb_tstamp_tx works by cloning the packets when timestamping
> them, therefore attempting to perform hardware timestamping on an skb's
> clone will also change the hardware timestamp of the original skb. And
> the original skb might have been yet again cloned for software
> timestamping, at an earlier stage.
>
> So the logic in __sock_recv_timestamp can't be as simple as saying
> "does this skb have a hardware timestamp? if yes I'll send the hardware
> timestamp to the socket, otherwise I'll send the software timestamp",
> precisely because the hardware timestamp is shared.
> Instead, it's quite the other way around: __sock_recv_timestamp says
> "does this skb have a software timestamp? if yes, I'll send the software
> timestamp, otherwise the hardware one". This works because the software
> timestamp is not shared with clones.
>
> But that means we have a problem when we attempt hardware timestamping
> with skbs that don't have the skb->tstamp == 0. __sock_recv_timestamp
> will say "oh, yeah, this must be some sort of odd clone" and will not
> deliver the hardware timestamp to the socket. And this is exactly what
> is happening when we have txtime enabled on the socket: as mentioned,
> that is put in a union with skb->tstamp, so it is quite easy to mistake
> it.
Great write up :-)
I know first person the time/effort that went on debugging this.
What do you think of introducing a helper, something like:
skb_txtime_consumed()
to make it even clearer what's going on.
>
> Do what other drivers do (intel igb/igc) and write zero to skb->tstamp
> before taking the hardware timestamp. It's of no use to us now (we're
> already on the TX confirmation path).
>
> Fixes: 0d08c9ec7d6e ("enetc: add support time specific departure base on the
> qos etf")
> Cc: Vinicius Costa Gomes <vinicius.go...@intel.com>
> Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.com>
> ---
Anyway,
Acked-by: Vinicius Costa Gomes <vinicius.go...@intel.com>
Cheers,
--
Vinicius
