Message-ID: <78384abd-7fae-492d-947e-c3311f952d87@kzalloc.com>
Date: Wed, 6 Aug 2025 00:41:36 +0900
From: Yunseong Kim <ysk@...lloc.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Dmitry Vyukov <dvyukov@...gle.com>,
 Andrey Konovalov <andreyknvl@...il.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Thomas Gleixner <tglx@...utronix.de>,
 Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
 Byungchul Park <byungchul@...com>, max.byungchul.park@...il.com,
 Yeoreum Yun <yeoreum.yun@....com>, ppbuk5246@...il.com,
 linux-usb@...r.kernel.org, linux-rt-devel@...ts.linux.dev,
 syzkaller@...glegroups.com, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH 2/4] kcov: Replace per-CPU local_lock with
 local_irq_save/restore

Hi Steve,

Thanks for the detailed feedback and suggestions.

On 8/5/25 1:37 오전, Steven Rostedt wrote:
> On Sun,  3 Aug 2025 07:20:45 +0000
> Yunseong Kim <ysk@...lloc.com> wrote:
> 
>> Commit f85d39dd7ed8 ("kcov, usb: disable interrupts in
>> kcov_remote_start_usb_softirq") introduced a local_irq_save() in the
>> kcov_remote_start_usb_softirq() wrapper, placing kcov_remote_start() in
>> atomic context.
>>
>> The previous patch addressed this by converting the global
> 
> Don't ever use the phrase "The previous patch" in a change log. These get
> added to git and it's very hard to find any order of one patch to another.
> When doing a git blame 5 years from now, "The previous patch" will be
> meaningless.

I agree that using phrases like "The previous patch" in changelogs is not a
good practice, especially considering future maintenance and git blame
scenarios.

>> kcov_remote_lock to a non-sleeping raw_spinlock_t. However, per-CPU
>> data in kcov_remote_start() and kcov_remote_stop() remains protected
>> by kcov_percpu_data.lock, which is a local_lock_t.
> 
> Instead, you should say something like:
> 
>   As kcov_remote_start() is now in atomic context, the kcov_remote lock was
>   converted to a non-sleeping raw_spinlock. However, per-cpu ...

I’ll revise the commit messages in the next iteration to explicitly
describe the context.

>> On PREEMPT_RT kernels, local_lock_t is implemented as a sleeping lock.
>> Acquiring it from atomic context triggers warnings or crashes due to
>> invalid sleeping behavior.
>>
>> The original use of local_lock_t assumed that kcov_remote_start() would
>> never be called in atomic context. Now that this assumption no longer
>> holds, replace it with local_irq_save() and local_irq_restore(), which are
>> safe in all contexts and compatible with the use of raw_spinlock_t.
> 
> Hmm, if the local_lock_t() is called inside of the taking of the
> raw_spinlock_t, then this patch should probably be first. Why introduce a
> different bug when fixing another one?

Regarding the patch ordering and the potential for introducing new bugs if the
local_lock_t conversions come after the raw_spinlock conversion, that’s a very
good point. I’ll review the patch sequence carefully to ensure the fixes apply
cleanly without regressions.

> Then the change log of this and the previous patch can both just mention
> being called from atomic context.
> 
> This change log would probably then say, "in order to convert the kcov locks
> to raw_spinlocks, the local_lock_irqsave()s need to be converted over to
> local_irq_save()".
> 
> -- Steve

Also, I will update the changelog to clearly state.

Thanks again for your thorough review and guidance!

Best regards,
Yunseong Kim

Powered by blists - more mailing lists