Date:   Mon, 08 Mar 2021 11:53:47 -0800
From:   Vinicius Costa Gomes <vinicius.gomes@...el.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Po Liu <po.liu@....com>
Cc:     Alex Marginean <alexandru.marginean@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Michael Walle <michael@...le.cc>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net 2/2] net: enetc: allow hardware timestamping on TX
 queues with tc-etf enabled

Vladimir Oltean <olteanv@...il.com> writes:

> From: Vladimir Oltean <vladimir.oltean@....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.gomes@...el.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---

Anyway,

Acked-by: Vinicius Costa Gomes <vinicius.gomes@...el.com>


Cheers,
-- 
Vinicius

Powered by blists - more mailing lists