Re: [PATCH 4/4] irqtime: drop local_irq_save/restore from irqtime_account_irq

[Frederic Weisbecker]

On Thu, Jun 30, 2016 at 03:35:50PM -0400, riel@xxxxxxxxxx wrote:
> From: Rik van Riel <riel@xxxxxxxxxx>
> 
> Drop local_irq_save/restore from irqtime_account_irq.
> Instead, have softirq and hardirq track their time spent
> independently, with the softirq code subtracting hardirq
> time that happened during the duration of the softirq run.
> 
> The softirq code can be interrupted by hardirq code at
> any point in time, but it can check whether it got a
> consistent snapshot of the timekeeping variables it wants,
> and loop around in the unlikely case that it did not.
> 
> Signed-off-by: Rik van Riel <riel@xxxxxxxxxx>

So the purpose is to get rid of local_irq_save/restore()?
Is it really worth such complication?

> ---
>  kernel/sched/cputime.c | 72 +++++++++++++++++++++++++++++++++++++++++---------
>  kernel/sched/sched.h   | 38 +++++++++++++++++++++-----
>  2 files changed, 90 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index a0aefd4c7ea6..b78991fac228 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -26,7 +26,9 @@
>  DEFINE_PER_CPU(u64, cpu_hardirq_time);
>  DEFINE_PER_CPU(u64, cpu_softirq_time);
>  
> -static DEFINE_PER_CPU(u64, irq_start_time);
> +static DEFINE_PER_CPU(u64, hardirq_start_time);
> +static DEFINE_PER_CPU(u64, softirq_start_time);
> +static DEFINE_PER_CPU(u64, prev_hardirq_time);
>  static int sched_clock_irqtime;
>  
>  void enable_sched_clock_irqtime(void)
> @@ -41,6 +43,7 @@ void disable_sched_clock_irqtime(void)
>  
>  #ifndef CONFIG_64BIT
>  DEFINE_PER_CPU(seqcount_t, irq_time_seq);
> +DEFINE_PER_CPU(seqcount_t, softirq_time_seq);
>  #endif /* CONFIG_64BIT */
>  
>  /*
> @@ -53,36 +56,79 @@ DEFINE_PER_CPU(seqcount_t, irq_time_seq);
>   * softirq -> hardirq, hardirq -> softirq
>   *
>   * When exiting hardirq or softirq time, account the elapsed time.
> + *
> + * When exiting softirq time, subtract the amount of hardirq time that
> + * interrupted this softirq run, to avoid double accounting of that time.
>   */
>  void irqtime_account_irq(struct task_struct *curr, int irqtype)
>  {
> -	unsigned long flags;
> +	u64 prev_softirq_start;
> +	bool leaving_softirq;
> +	u64 prev_hardirq;
> +	u64 hardirq_time;
>  	s64 delta;
>  	int cpu;
>  
>  	if (!sched_clock_irqtime)
>  		return;
>  
> -	local_irq_save(flags);
> -
>  	cpu = smp_processor_id();
> -	delta = sched_clock_cpu(cpu) - __this_cpu_read(irq_start_time);
> -	__this_cpu_add(irq_start_time, delta);
>  
> -	irq_time_write_begin();
> +	/*
> +	 * Hardirq time accounting is pretty straightforward. If not in
> +	 * hardirq context yet (entering hardirq), set the start time.
> +	 * If already in hardirq context (leaving), account the elapsed time.
> +	 */
> +	if (irqtype == HARDIRQ_OFFSET) {
> +		bool leaving_hardirq = hardirq_count();
> +		delta = sched_clock_cpu(cpu) - __this_cpu_read(hardirq_start_time);
> +		__this_cpu_add(hardirq_start_time, delta);
> +		if (leaving_hardirq) {
> +			hardirq_time_write_begin();
> +			__this_cpu_add(cpu_hardirq_time, delta);
> +			hardirq_time_write_end();
> +		}

This doesn't seem to work with nesting hardirqs.

Thanks.