On Tue, Sep 27, 2016 at 1:32 AM, Andrew Morton
<[email protected]> wrote:
> On Fri, 23 Sep 2016 16:51:13 +0200 Andrey Konovalov <[email protected]>
> 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 <[email protected]>
>> ---
>> 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.
