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@kzalloc.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