Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1434809 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/4] irqtime: drop local_irq_save/restore from irqtime_account_irq riel@redhat.com - 2016-06-30 21:40 +0200
    Re: [PATCH 4/4] irqtime: drop local_irq_save/restore from  irqtime_account_irq Frederic Weisbecker <fweisbec@gmail.com> - 2016-07-08 14:40 +0200
      Re: [PATCH 4/4] irqtime: drop local_irq_save/restore from  irqtime_account_irq Rik van Riel <riel@redhat.com> - 2016-07-08 15:30 +0200
        Re: [PATCH 4/4] irqtime: drop local_irq_save/restore from  irqtime_account_irq Frederic Weisbecker <fweisbec@gmail.com> - 2016-07-08 16:10 +0200
        Re: [PATCH 4/4] irqtime: drop local_irq_save/restore from  irqtime_account_irq Paolo Bonzini <pbonzini@redhat.com> - 2016-07-08 16:40 +0200
          Re: [PATCH 4/4] irqtime: drop local_irq_save/restore from  irqtime_account_irq Rik van Riel <riel@redhat.com> - 2016-07-08 18:00 +0200
            Re: [PATCH 4/4] irqtime: drop local_irq_save/restore from  irqtime_account_irq Frederic Weisbecker <fweisbec@gmail.com> - 2016-07-09 02:00 +0200

#1434809 — [PATCH 4/4] irqtime: drop local_irq_save/restore from irqtime_account_irq

From: Rik van Riel <riel@redhat.com>

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@redhat.com>
---
 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();
+		}
+		return;
+	}
+
+	/*
+	 * Softirq context may get interrupted by hardirq context, on the
+	 * same CPU. At softirq entry time the amount of time this CPU spent
+	 * in hardirq context is stored. At softirq exit time, the time spent
+	 * in hardirq context during the softirq is subtracted.
+	 */
+	prev_softirq_start = __this_cpu_read(softirq_start_time);
+	prev_hardirq = __this_cpu_read(prev_hardirq_time);
+	leaving_softirq = in_serving_softirq();
+
+	do {
+		u64 now = sched_clock_cpu(cpu);
+
+		hardirq_time = READ_ONCE(per_cpu(cpu_hardirq_time, cpu));
+		__this_cpu_write(softirq_start_time, now);
+		__this_cpu_write(prev_hardirq_time, hardirq_time);
+
+		if (leaving_softirq) {
+			/*
+			 * Subtract hardirq time that happened during this
+			 * softirq.
+			 */
+			s64 hi_delta = hardirq_time - prev_hardirq;
+			delta = now - prev_softirq_start - hi_delta;
+		}
+		/* Loop around if interrupted by a hardirq. */
+	} while (hardirq_time != READ_ONCE(per_cpu(cpu_hardirq_time, cpu)));
+
 	/*
 	 * We do not account for softirq time from ksoftirqd here.
 	 * We want to continue accounting softirq time to ksoftirqd thread
 	 * in that case, so as not to confuse scheduler with a special task
 	 * that do not consume any time, but still wants to run.
 	 */
-	if (hardirq_count())
-		__this_cpu_add(cpu_hardirq_time, delta);
-	else if (in_serving_softirq() && curr != this_cpu_ksoftirqd())
+	softirq_time_write_begin();
+	if (leaving_softirq && curr != this_cpu_ksoftirqd())
 		__this_cpu_add(cpu_softirq_time, delta);
-
-	irq_time_write_end();
-	local_irq_restore(flags);
+	softirq_time_write_end();
 }
 EXPORT_SYMBOL_GPL(irqtime_account_irq);
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index ec2e8d23527e..cad4df9835f7 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1752,38 +1752,62 @@ DECLARE_PER_CPU(u64, cpu_softirq_time);
 
 #ifndef CONFIG_64BIT
 DECLARE_PER_CPU(seqcount_t, irq_time_seq);
+DECLARE_PER_CPU(seqcount_t, softirq_time_seq);
 
-static inline void irq_time_write_begin(void)
+static inline void hardirq_time_write_begin(void)
 {
 	__this_cpu_inc(irq_time_seq.sequence);
 	smp_wmb();
 }
 
-static inline void irq_time_write_end(void)
+static inline void hardirq_time_write_end(void)
 {
 	smp_wmb();
 	__this_cpu_inc(irq_time_seq.sequence);
 }
 
+static inline void softirq_time_write_begin(void)
+{
+	__this_cpu_inc(softirq_time_seq.sequence);
+	smp_wmb();
+}
+
+static inline void softirq_time_write_end(void)
+{
+	smp_wmb();
+	__this_cpu_inc(softirq_time_seq.sequence);
+}
+
 static inline u64 irq_time_read(int cpu)
 {
 	u64 irq_time;
-	unsigned seq;
+	unsigned hi_seq;
+	unsigned si_seq;
 
 	do {
-		seq = read_seqcount_begin(&per_cpu(irq_time_seq, cpu));
+		hi_seq = read_seqcount_begin(&per_cpu(irq_time_seq, cpu));
+		si_seq = read_seqcount_begin(&per_cpu(softirq_time_seq, cpu));
 		irq_time = per_cpu(cpu_softirq_time, cpu) +
 			   per_cpu(cpu_hardirq_time, cpu);
-	} while (read_seqcount_retry(&per_cpu(irq_time_seq, cpu), seq));
+	} while (read_seqcount_retry(&per_cpu(irq_time_seq, cpu), hi_seq) ||
+		 read_seqcount_retry(&per_cpu(softirq_time_seq, cpu), si_seq));
 
 	return irq_time;
 }
 #else /* CONFIG_64BIT */
-static inline void irq_time_write_begin(void)
+static inline void hardirq_time_write_begin(void)
+{
+}
+
+static inline void hardirq_time_write_end(void)
+{
+}
+
+static inline void softirq_time_write_begin(void)
 {
 }
 
-static inline void irq_time_write_end(void)
+static inline void softirq_time_write_end(void)
 {
 }
 
-- 
2.7.4

[toc] | [next] | [standalone]

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

On Thu, Jun 30, 2016 at 03:35:50PM -0400, riel@redhat.com wrote:
> From: Rik van Riel <riel@redhat.com>
> 
> 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@redhat.com>

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.

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

On Fri, 2016-07-08 at 14:30 +0200, Frederic Weisbecker wrote:
> On Thu, Jun 30, 2016 at 03:35:50PM -0400, riel@redhat.com wrote:
> > From: Rik van Riel <riel@redhat.com>
> > 
> > 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@redhat.com>
> 
> So the purpose is to get rid of local_irq_save/restore()?
> Is it really worth such complication?

local_irq_save/restore are quite slow, and look like the
largest source of overhead in irq time accounting.

However, I have not gotten numbers yet, and have no problem
with this patch being dropped for now.

> > ---
> >  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.

Where does it break?

enter hardirq A -> hardirq_start_time = now

enter hardirq B -> hardirq_start_time = now,
                   account already elapsed time

leave hardirq B -> account elapsed time, set
                   hardirq_start_time = now

leave hardirq A -> account elapsed time

What am I missing, except a softirq-style do-while
loop to account for hardirq A being interrupted by
hardirq B while updating the statistics?

-- 

All Rights Reversed.

[toc] | [prev] | [next] | [standalone]

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

On Fri, Jul 08, 2016 at 09:19:47AM -0400, Rik van Riel wrote:
> On Fri, 2016-07-08 at 14:30 +0200, Frederic Weisbecker wrote:
> > > +	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.
> 
> Where does it break?
> 
> enter hardirq A -> hardirq_start_time = now
> 
> enter hardirq B -> hardirq_start_time = now,
>                    account already elapsed time
> 
> leave hardirq B -> account elapsed time, set
>                    hardirq_start_time = now
> 
> leave hardirq A -> account elapsed time
> 
> What am I missing, except a softirq-style do-while
> loop to account for hardirq A being interrupted by
> hardirq B while updating the statistics?

How about:

enter hardirq A -> delta = sched_clock_cpu(cpu) - __this_cpu_read(hardirq_start_time);
   enter hardirq B -> delta = sched_clock_cpu(cpu) - __this_cpu_read(hardirq_start_time);
   __this_cpu_add(hardirq_start_time, delta);
   ...
   exit hardirq B
__this_cpu_add(hardirq_start_time, delta);

Also seqcount writers can't nest.

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

On 08/07/2016 15:19, Rik van Riel wrote:
> On Fri, 2016-07-08 at 14:30 +0200, Frederic Weisbecker wrote:
>> On Thu, Jun 30, 2016 at 03:35:50PM -0400, riel@redhat.com wrote:
>>> From: Rik van Riel <riel@redhat.com>
>>>
>>> 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@redhat.com>
>>
>> So the purpose is to get rid of local_irq_save/restore()?
>> Is it really worth such complication?
> 
> local_irq_save/restore are quite slow, and look like the
> largest source of overhead in irq time accounting.

I'm looking at an upstream tree, without your patches applied,
but it seems to me that irqtime_account_irq is always called with
interrupts disabled:

irqtime_account_irq
	-> account_irq_enter_time
		-> __irq_enter
			-> HARDIRQ_ENTER	[1]
			-> irq_enter		[3]
		-> __do_softirq			[1]
	-> account_irq_exit_time
		-> __do_softirq			[1]
		-> __irq_exit
			-> HARDIRQ_EXIT		[1]
		-> irq_exit			[2]

	[1] = does local_irq_disable/enable
	[2] = contains WARN_ON_ONCE(!irqs_disabled())
	[3] = calls rcu_irq_enter(), which checks irqs_disabled()

I don't think your first two patches change this, so perhaps it's enough
to remove that local_irq-save/restore?  Either this, or ENEEDWEEKEND...

Paolo

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

On Fri, 2016-07-08 at 16:34 +0200, Paolo Bonzini wrote:
> 
> On 08/07/2016 15:19, Rik van Riel wrote:
> > On Fri, 2016-07-08 at 14:30 +0200, Frederic Weisbecker wrote:
> > > On Thu, Jun 30, 2016 at 03:35:50PM -0400, riel@redhat.com wrote:
> > > > From: Rik van Riel <riel@redhat.com>
> > > > 
> > > > 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@redhat.com>
> > > 
> > > So the purpose is to get rid of local_irq_save/restore()?
> > > Is it really worth such complication?
> > 
> > local_irq_save/restore are quite slow, and look like the
> > largest source of overhead in irq time accounting.
> 
> I'm looking at an upstream tree, without your patches applied,
> but it seems to me that irqtime_account_irq is always called with
> interrupts disabled:
> 
> irqtime_account_irq
> 	-> account_irq_enter_time
> 		-> __irq_enter
> 			-> HARDIRQ_ENTER	[1]
> 			-> irq_enter		[3]
> 		-> __do_softirq			[1]
> 	-> account_irq_exit_time
> 		-> __do_softirq			[1]
> 		-> __irq_exit
> 			-> HARDIRQ_EXIT		[1]
> 		-> irq_exit			[2]
> 
> 	[1] = does local_irq_disable/enable
> 	[2] = contains WARN_ON_ONCE(!irqs_disabled())
> 	[3] = calls rcu_irq_enter(), which checks irqs_disabled()
> 
> I don't think your first two patches change this, so perhaps it's
> enough
> to remove that local_irq-save/restore?  Either this, or
> ENEEDWEEKEND...

I think you are right!

__do_softirq() calls account_irq_enter_time() with irqs
already disabled, and also has irqs disabled when it
calls account_irq_exit_time()

This appears to be true for both ksoftirqd and softirq
from irq context.

This could simplify my patch series a lot :)

-- 

All Rights Reversed.

[toc] | [prev] | [next] | [standalone]

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

On Fri, Jul 08, 2016 at 11:56:49AM -0400, Rik van Riel wrote:
> On Fri, 2016-07-08 at 16:34 +0200, Paolo Bonzini wrote:
> > I'm looking at an upstream tree, without your patches applied,
> > but it seems to me that irqtime_account_irq is always called with
> > interrupts disabled:
> > 
> > irqtime_account_irq
> > 	-> account_irq_enter_time
> > 		-> __irq_enter
> > 			-> HARDIRQ_ENTER	[1]
> > 			-> irq_enter		[3]
> > 		-> __do_softirq			[1]
> > 	-> account_irq_exit_time
> > 		-> __do_softirq			[1]
> > 		-> __irq_exit
> > 			-> HARDIRQ_EXIT		[1]
> > 		-> irq_exit			[2]
> > 
> > 	[1] = does local_irq_disable/enable
> > 	[2] = contains WARN_ON_ONCE(!irqs_disabled())
> > 	[3] = calls rcu_irq_enter(), which checks irqs_disabled()
> > 
> > I don't think your first two patches change this, so perhaps it's
> > enough
> > to remove that local_irq-save/restore?  Either this, or
> > ENEEDWEEKEND...

Good catch Paolo!

> 
> I think you are right!
> 
> __do_softirq() calls account_irq_enter_time() with irqs
> already disabled, and also has irqs disabled when it
> calls account_irq_exit_time()
> 
> This appears to be true for both ksoftirqd and softirq
> from irq context.

Indeed! And irq_enter()/irq_exit() have irqs disabled requirements.

The other users are __irq_enter() and __irq_exit() called by
lockdep selftests which takes care about it too.

I just did a boot test with a WARN_ON_ONCE(!irqs_disabled())
on account_irq_enter_time() and it triggered no issue.

> 
> This could simplify my patch series a lot :)

Definetly! ;-)

Would you mind resending it?

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web