netdev - Re: igc: missing HW timestamps at TX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 16 Aug 2022 11:11:47 -0700
From:   Vinicius Costa Gomes <vinicius.gomes@...el.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     Ferenc Fejes <ferenc.fejes@...csson.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "marton12050@...il.com" <marton12050@...il.com>,
        "peti.antal99@...il.com" <peti.antal99@...il.com>
Subject: Re: igc: missing HW timestamps at TX

Vladimir Oltean <vladimir.oltean@....com> writes:

> Hi Vinicius,
>
> On Mon, Aug 15, 2022 at 04:07:13PM -0700, Vinicius Costa Gomes wrote:
>> The interrupt that is generated is a general/misc interrupt, we have to
>> check on the interrupt cause bit that it's something TimeSync related,
>> and only then, we have to check that it's indeed a TX Timestamp that is
>> ready. And then, there's another register with some bits saying which
>> one of the 4 registers for timestamps that is ready. There are a few
>> levels of indirection, but no polling.
>
> I used the word "poll" after being inspired by the code comments:
>
> /**
>  * igc_ptp_tx_work
>  * @work: pointer to work struct
>  *
>  * This work function polls the TSYNCTXCTL valid bit to determine when a
>  * timestamp has been taken for the current stored skb.
>  */
>

Ah, I see. That comment is misleading/wrong, that was "inherited" from
igb, which has a model that didn't have the TimeSync interrupt cause,
IIRC. Going to fix that.

>> I think your question is more "why there's that workqueue on igc?"/"why
>> don't you retrieve the TX timestamp 'inline' with the interrupt?", if I
>> got that right, then, I don't have a good reason, apart from the feeling
>> that reading all those (5-6?) registers may take too long for a
>> interrupt handler. And it's something that's being done the same for
>> most (all?) Intel drivers.
>
> Ok, so basically it is an attempt of making the interrupt handler threaded,
> which doesn't run in hardirq context?
>

Something like that, I guess. Mixed with "if it ain't broken, don't fix
it", but things are breaking with higher link speeds, different use
cases, more users/interest.

> Note that this decision makes the igc limitation of "single timestampable
> skb in flight" even much worse than it needs to be, because it prolongs
> the "in flight" period until the system_wq actually gets to run the work
> item we create.
>

Yes, no disagreement from my side.

>> I have a TODO to experiment with removing the workqueue, and retrieving
>> the TX timestamp in the same context as the interrupt handler, but other
>> things always come up.
>> 
>> 
>> Cheers,
>> -- 
>> Vinicius


Cheers,
-- 
Vinicius