Messages in this thread Patch in this message | | | From | Dmitry Vyukov <> | Date | Tue, 27 Sep 2016 08:21:32 +0200 | Subject | Re: [PATCH] kcov: properly check if we are in an interrupt |
| |
On Tue, Sep 27, 2016 at 1:32 AM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Fri, 23 Sep 2016 16:51:13 +0200 Andrey Konovalov <andreyknvl@google.com> wrote: > >> in_interrupt() returns a nonzero value when we are either in an >> interrupt or have bh disabled via local_bh_disable(). Since we are >> interested in only ignoring coverage from actual interrupts, do a >> proper check of whether we are really in an interrupt. >> >> Signed-off-by: Andrey Konovalov <andreyknvl@google.com> >> --- >> It would look totally better to reuse in_irq(), in_serving_softirq() and >> in_nmi() instead of checking flags manually, but that leads to slower >> generated code (three separate tests for each of the flags). Would it be >> better to add another macro to preempt.h that would check if we're actually >> in interrupt and use it? > > Yes please. Is there anywhere else where such a macro can be used?
I suspect there is a bunch of places that use in_interrupt(), but mean the same as KCOV wants -- am I in interrupt? and not am I in interrupt context or in normal task context but inside local_bh_disable(). For example, why does fput handles closure asynchronously if the task called local_bh_disable?
264 void fput(struct file *file) 265 { 266 if (atomic_long_dec_and_test(&file->f_count)) { 267 struct task_struct *task = current; 268 269 if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) { 270 init_task_work(&file->f_u.fu_rcuhead, ____fput); 271 if (!task_work_add(task, &file->f_u.fu_rcuhead, true)) 272 return; 273 /* 274 * After this task has run exit_task_work(), 275 * task_work_add() will fail. Fall through to delayed 276 * fput to avoid leaking *file. 277 */ 278 } 279 280 if (llist_add(&file->f_u.fu_llist, &delayed_fput_list)) 281 schedule_delayed_work(&delayed_fput_work, 1); 282 } 283 }
>> --- a/kernel/kcov.c >> +++ b/kernel/kcov.c >> @@ -54,7 +54,8 @@ void notrace __sanitizer_cov_trace_pc(void) >> * We are interested in code coverage as a function of a syscall inputs, >> * so we ignore code executed in interrupts. >> */ >> - if (!t || in_interrupt()) >> + if (!t || (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_OFFSET >> + | NMI_MASK))) > > Or include a prominent and very apologetic comment here explaining why > it is open-coded. But I do agree that not open-coding it is better.
| |