netdev - Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

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]

Message-ID: <a1e6619e-a2f5-ced7-c053-ab196fb0cf09@intel.com>
Date:   Wed, 7 Dec 2022 15:22:38 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Saeed Mahameed <saeed@...nel.org>,
        Tony Nguyen <anthony.l.nguyen@...el.com>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <edumazet@...gle.com>,
        Anatolii Gerasymenko <anatolii.gerasymenko@...el.com>,
        <netdev@...r.kernel.org>, <richardcochran@...il.com>,
        Gurucharan G <gurucharanx.g@...el.com>
Subject: Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp
 extts work

On 12/7/2022 2:19 PM, Saeed Mahameed wrote:
> On 07 Dec 13:10, Tony Nguyen wrote:
>> From: Anatolii Gerasymenko <anatolii.gerasymenko@...el.com>
>>
>> ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
>> the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
>> sends messages to AQ and waits for responses. This causes
>> ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
>> causes problems with the reading of the incoming signal timestamps,
>> which disrupts a 100 Hz signal.
>>
> 
> Sounds like an optimization rather than a bug fix, unless you explain what
> the symptoms are and how critical this patch is.

I'm not the original author, but I think Anatolii is out right now. I'll 
try to explain for him.

The problem is that extts must execute to read the timestamp value from 
the incoming signal. It has to complete before the next level change. 
Otherwise I believe if we don't read it in time the next level change 
will be ignored.

For a 100Hz signal, this means it has to process within 10 milliseconds. 
Kworkers can only execute one task at a time. If the periodic work is 
already blocking and currently processing an AdminQ message it might go 
to sleep for a while until it can process the message. Even if the task 
goes to sleep, the other kthread item cannot execute on the same 
kworker. This can potentially result in a delay long enough to prevent 
the external timestamp work function from reading the external timestamp 
within the time limit. I believe this results in missed external 
timestamp events. I am not certain if this loss is permanent or only 
transient.

I would consider that a bug, because it results in loss of 
functionality, not just something like lower CPU usage.

> 
> code LGTM, although i find it wasteful to create a kthread per device event
> type, but i can't think of a better way.
> 

Right. I can't think of another good way either. I think we can't 
process this directly in the interrupt which is why we had processed it 
in a kthread item to begin with.

Thanks,
Jake