Re: [PATCH] kcov: properly check if we are in an interrupt
On Tue, Sep 27, 2016 at 12:59 PM, Peter Zijlstra wrote: > On Tue, Sep 27, 2016 at 09:50:41AM +0200, Dmitry Vyukov wrote: >> On Tue, Sep 27, 2016 at 9:34 AM, Peter Zijlstra wrote: >> > On Tue, Sep 27, 2016 at 08:21:32AM +0200, Dmitry Vyukov wrote: >> >> >> >> 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? >> > >> > Agreed, but it would mean auditing all in_interrupt()/irq_count() users. >> >> >> I don't think this means auditing all users. We are not making things >> worse by introduction of a new predicate. >> It would be nice to look at some uses in core code, but the only place >> with observed harm is KCOV. >> >> Any naming suggestions? Other than really_in_interrupt or >> in_interrupt_and_not_in_bh_disabled? > > Hence the suggestion to audit and fix instead of making a bigger mess :/ Ah, I see what you mean. I am not sure Andrey and me are well equipped to tackle it. We have very narrow kernel experience. Lots of uses are in drivers, which we don't even know how to invoke. Guessing re current state of the things, calling potentially expensive functions with bh disabled is probably bad, but we have no idea as to the threshold. Any ideas regarding a shorter term fix? We have a very real problem in KCOV. We could open code and comment the new check in KCOV. Or maybe do a global s/in_interrupt/in_interrupt_or_bh_disabled/ and then introduce new in_interrupt. I don't know what's the practice with such global changes.
Re: [PATCH] kcov: properly check if we are in an interrupt
On 09/27/2016 01:20 PM, Vegard Nossum wrote: Your patch was: -if (!t || in_interrupt()) +if (!t || (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_OFFSET +| NMI_MASK))) But look at the definitions: #define irq_count() (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_MASK \ | NMI_MASK)) #define in_interrupt() (irq_count()) So isn't the patch a no-op to start with? Eh, SOFTIRQ_OFFSET vs SOFTIRQ_MASK. I'll just go stand in the corner. Vegard
Re: [PATCH] kcov: properly check if we are in an interrupt
On 09/27/2016 09:50 AM, Dmitry Vyukov wrote: On Tue, Sep 27, 2016 at 9:34 AM, Peter Zijlstra wrote: On Tue, Sep 27, 2016 at 08:21:32AM +0200, Dmitry Vyukov wrote: 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? Agreed, but it would mean auditing all in_interrupt()/irq_count() users. I don't think this means auditing all users. We are not making things worse by introduction of a new predicate. It would be nice to look at some uses in core code, but the only place with observed harm is KCOV. Any naming suggestions? Other than really_in_interrupt or in_interrupt_and_not_in_bh_disabled? Your patch was: - if (!t || in_interrupt()) + if (!t || (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_OFFSET + | NMI_MASK))) But look at the definitions: #define irq_count() (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_MASK \ | NMI_MASK)) #define in_interrupt() (irq_count()) So isn't the patch a no-op to start with? Vegard
Re: [PATCH] kcov: properly check if we are in an interrupt
On Tue, Sep 27, 2016 at 09:50:41AM +0200, Dmitry Vyukov wrote: > On Tue, Sep 27, 2016 at 9:34 AM, Peter Zijlstra wrote: > > On Tue, Sep 27, 2016 at 08:21:32AM +0200, Dmitry Vyukov wrote: > >> > >> 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? > > > > Agreed, but it would mean auditing all in_interrupt()/irq_count() users. > > > I don't think this means auditing all users. We are not making things > worse by introduction of a new predicate. > It would be nice to look at some uses in core code, but the only place > with observed harm is KCOV. > > Any naming suggestions? Other than really_in_interrupt or > in_interrupt_and_not_in_bh_disabled? Hence the suggestion to audit and fix instead of making a bigger mess :/
Re: [PATCH] kcov: properly check if we are in an interrupt
On Tue, Sep 27, 2016 at 9:34 AM, Peter Zijlstra wrote: > On Tue, Sep 27, 2016 at 08:21:32AM +0200, Dmitry Vyukov wrote: >> >> 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? > > Agreed, but it would mean auditing all in_interrupt()/irq_count() users. I don't think this means auditing all users. We are not making things worse by introduction of a new predicate. It would be nice to look at some uses in core code, but the only place with observed harm is KCOV. Any naming suggestions? Other than really_in_interrupt or in_interrupt_and_not_in_bh_disabled?
Re: [PATCH] kcov: properly check if we are in an interrupt
On Tue, Sep 27, 2016 at 08:21:32AM +0200, Dmitry Vyukov wrote: > > 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? Agreed, but it would mean auditing all in_interrupt()/irq_count() users.
Re: [PATCH] kcov: properly check if we are in an interrupt
On Tue, Sep 27, 2016 at 1:32 AM, Andrew Morton
wrote:
> On Fri, 23 Sep 2016 16:51:13 +0200 Andrey Konovalov
> 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
>> ---
>> 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.
Re: [PATCH] kcov: properly check if we are in an interrupt
On Fri, 23 Sep 2016 16:51:13 +0200 Andrey Konovalov 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 > --- > 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? > --- 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.
[PATCH] kcov: properly check if we are in an interrupt
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
---
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?
kernel/kcov.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/kcov.c b/kernel/kcov.c
index 8d44b3f..47c35a8 100644
--- 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)))
return;
mode = READ_ONCE(t->kcov_mode);
if (mode == KCOV_MODE_TRACE) {
--
2.8.0.rc3.226.g39d4020
