Re: [PATCH] si time accounting accounts bh_disable'd time to si

[Eric Dumazet <eric.dumazet@gmail.com>]

Le lundi 27 septembre 2010 à 13:35 -0700, Venkatesh Pallipadi a écrit :
> >> >> > You still do have the problem with local_bh_disable() though, since you
> >> >> > cannot distinguish between having bh disabled and processing softirq.
> >> >> >
> >> >> > So a hardirq that hits while you have bh disabled will inflate your
> >> >> > softirq time.
> 
> >> >> Hmm, that bug is valid for CONFIG_VIRT_CPU_ACCOUNTING=y as well.
> >> >
> >> > And nobody ever noticed?
> >> >
> >> Yes. I inherited the API from VIRT_CPU_ACCOUNTING along with this
> >> local_bh_disable bug. Agree that we need one extra bit to handle this
> >> case. I will take a stab at fixing this along with refresh of this
> >> patchset if no one else has beaten me to it until then.
> >
> >Make sure to only fix the softirq processing on the hardirq tail, not
> > the ksoftirqd one :-)
> 
> softirq processing from hardirq tail and ksoftirqd are currently
> handled in the same way and I didn't see any issues changing both of
> them. Am I missing something?
> 
> Here's the patch I have for this. 
> 
> [PATCH] si time accounting accounts bh_disable'd time to si
> 
> Peter Zijlstra found a bug in the way softirq time is accounted in
> VIRT_CPU_ACCOUNTING on this thread.
> http://lkml.indiana.edu/hypermail//linux/kernel/1009.2/01366.html
> 
> The problem is, softirq processing uses local_bh_disable internally and there
> would be no way later in the flow to differentiate between whether softirq is
> being processed or is it just that bh has been disabled. So, a hardirq when bh
> id disabled results in time being wrongly accounted for softirq.
> 
> Looking at the code a bit more, the problem exists in !VIRT_CPU_ACCOUNTING
> as well. As account_system_time() in normal tick based accouting also uses
> softirq_count, which will be set when not in softirq with bh disabled.
> 
> Peter also suggested solution of using 2 * SOFTIRQ_OFFSET as irq count
> for local_bh_{disable,enable} and using just SOFTIRQ_OFFSET while softirq
> processing. The patch below does that and adds API in_serving_softirq() which
> returns whether we are currently processing softirq or not.
> 
> Also changes one of the usages of softirq_count in net/sched/cls_cgroup.c
> to in_serving_softirq.
> 
> Looks like many usages of in_softirq really want in_serving_softirq. Those
> changes can be made individually on a case by case basis.
> 
> Signed-off-by: Venkatesh Pallipadi <venki@google.com>
> ---
>  include/linux/hardirq.h |    3 ++
>  include/linux/sched.h   |    2 +-
>  kernel/sched.c          |    2 +-
>  kernel/softirq.c        |   51 +++++++++++++++++++++++++++++++---------------
>  net/sched/cls_cgroup.c  |    2 +-
>  5 files changed, 40 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
> index d5b3876..1c736ae 100644
> --- a/include/linux/hardirq.h
> +++ b/include/linux/hardirq.h
> @@ -82,10 +82,13 @@
>  /*
>   * Are we doing bottom half or hardware interrupt processing?
>   * Are we in a softirq context? Interrupt context?
> + * in_softirq answers - are we currently processing softirq or have bh disabled?
> + * in_serving_softirq answers - are we currently processing softirq?
>   */
>  #define in_irq()		(hardirq_count())
>  #define in_softirq()		(softirq_count())
>  #define in_interrupt()		(irq_count())
> +#define in_serving_softirq()	(softirq_count() == SOFTIRQ_OFFSET)
>  

But softirq handlers sometime call functions that might disable bh
again. It would be good to not switch softirq time to system time ;)

Shouldnt we reserve a bit (high order bit out of 8) instead ?

 * PREEMPT_MASK:    0x000000ff
 * SOFTIRQ_MASK:    0x0000ff00
 * SERVING_SOFTIRQ: 0x00008000
 * HARDIRQ_MASK:    0x03ff0000
 *     NMI_MASK:    0x04000000