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

Groups > linux.kernel > #1424226 > unrolled thread

[PATCH 5/5] 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 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq riel@redhat.com - 2016-06-16 18:10 +0200
    Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Rik van Riel <riel@redhat.com> - 2016-06-22 00:30 +0200
      Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Rik van Riel <riel@redhat.com> - 2016-06-22 00:40 +0200
      Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Peter Zijlstra <peterz@infradead.org> - 2016-06-22 01:00 +0200
    Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Peter Zijlstra <peterz@infradead.org> - 2016-06-22 00:50 +0200
      Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Rik van Riel <riel@redhat.com> - 2016-06-23 00:00 +0200
        Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Paolo Bonzini <pbonzini@redhat.com> - 2016-06-23 16:00 +0200
          Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Rik van Riel <riel@redhat.com> - 2016-06-23 17:30 +0200

#1424226 — [PATCH 5/5] 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 | 63 ++++++++++++++++++++++++++++++++++++++++----------
 kernel/sched/sched.h   | 28 ++++++++++++++--------
 2 files changed, 70 insertions(+), 21 deletions(-)

diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 07f847267e4e..ba18d51a091c 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,72 @@ 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;
-	s64 delta;
+	u64 prev_softirq_start;
+	u64 prev_hardirq;
+	u64 hardirq_time;
+	s64 delta = 0;
 	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);
+	/*
+	 * Softirq context may get interrupted by hardirq context,
+	 * on the same CPU. At softirq entry time the amount of time
+	 * spent in hardirq context is stored. At softirq exit time,
+	 * the time spent in hardirq context during the softirq is
+	 * subtracted.
+	 */
+	prev_hardirq = __this_cpu_read(prev_hardirq_time);
+	prev_softirq_start = __this_cpu_read(softirq_start_time);
+
+	if (irqtype == HARDIRQ_OFFSET) {
+		delta = sched_clock_cpu(cpu) - __this_cpu_read(hardirq_start_time);
+		__this_cpu_add(hardirq_start_time, delta);
+	} else do {
+		u64 now = sched_clock_cpu(cpu);
+		hardirq_time = READ_ONCE(per_cpu(cpu_hardirq_time, cpu));
+
+		delta = now - prev_softirq_start;
+		if (in_serving_softirq()) {
+			/*
+			 * Leaving softirq context. Avoid double counting by
+			 * subtracting hardirq time from this interval.
+			 */
+			s64 hi_delta = hardirq_time - prev_hardirq;
+			delta -= hi_delta;
+		} else {
+			/* Entering softirq context. Note start times. */
+			__this_cpu_write(softirq_start_time, now);
+			__this_cpu_write(prev_hardirq_time, hardirq_time);
+		}
+		/*
+		 * If a hardirq happened during this calculation, it may not
+		 * have gotten a consistent snapshot. Try again.
+		 */
+	} while (hardirq_time != READ_ONCE(per_cpu(cpu_hardirq_time, cpu)));
 
-	irq_time_write_begin();
+	irq_time_write_begin(irqtype);
 	/*
 	 * 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())
+	if (irqtype == HARDIRQ_OFFSET && hardirq_count())
 		__this_cpu_add(cpu_hardirq_time, delta);
-	else if (in_serving_softirq() && curr != this_cpu_ksoftirqd())
+	else if (irqtype == SOFTIRQ_OFFSET && in_serving_softirq() &&
+				curr != this_cpu_ksoftirqd())
 		__this_cpu_add(cpu_softirq_time, delta);
 
-	irq_time_write_end();
-	local_irq_restore(flags);
+	irq_time_write_end(irqtype);
 }
 EXPORT_SYMBOL_GPL(irqtime_account_irq);
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index ec2e8d23527e..65d013447e0c 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1752,38 +1752,48 @@ 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 irq_time_write_begin(int irqtype)
 {
-	__this_cpu_inc(irq_time_seq.sequence);
+	if (irqtype == HARDIRQ_OFFSET)
+		__this_cpu_inc(irq_time_seq.sequence);
+	else
+		__this_cpu_inc(softirq_time_seq.sequence);
 	smp_wmb();
 }
 
-static inline void irq_time_write_end(void)
+static inline void irq_time_write_end(int irqtype)
 {
 	smp_wmb();
-	__this_cpu_inc(irq_time_seq.sequence);
+	if (irqtype == HARDIRQ_OFFSET)
+		__this_cpu_inc(irq_time_seq.sequence);
+	else
+		__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 irq_time_write_begin(int irqtype)
 {
 }
 
-static inline void irq_time_write_end(void)
+static inline void irq_time_write_end(int irqtype)
 {
 }
 
-- 
2.5.5

[toc] | [next] | [standalone]

#1428234 — Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq

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

On Tue, 2016-06-21 at 23:49 +0200, Peter Zijlstra wrote:
> On Thu, Jun 16, 2016 at 12:06:07PM -0400, riel@redhat.com wrote:
> > 
> > @@ -53,36 +56,72 @@ 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)
> >  {
> > +	u64 prev_softirq_start;
> > +	u64 prev_hardirq;
> > +	u64 hardirq_time;
> > +	s64 delta = 0;
> We appear to always assign to delta, so this initialization seems
> superfluous.
> 
> > 
> >  	int cpu;
> >  
> >  	if (!sched_clock_irqtime)
> >  		return;
> >  
> >  	cpu = smp_processor_id();
> Per this smp_processor_id() usage, preemption is disabled.

This code is called from the timer code. Surely preemption
is already disabled?

Should I change this into raw_smp_processor_id()?

> > 
> > +	/*
> > +	 * Softirq context may get interrupted by hardirq context,
> > +	 * on the same CPU. At softirq entry time the amount of
> > time
> > +	 * spent in hardirq context is stored. At softirq exit
> > time,
> > +	 * the time spent in hardirq context during the softirq is
> > +	 * subtracted.
> > +	 */
> > +	prev_hardirq = __this_cpu_read(prev_hardirq_time);
> > +	prev_softirq_start = __this_cpu_read(softirq_start_time);
> > +
> > +	if (irqtype == HARDIRQ_OFFSET) {
> > +		delta = sched_clock_cpu(cpu) -
> > __this_cpu_read(hardirq_start_time);
> > +		__this_cpu_add(hardirq_start_time, delta);
> > +	} else do {
> > +		u64 now = sched_clock_cpu(cpu);
> > +		hardirq_time = READ_ONCE(per_cpu(cpu_hardirq_time,
> > cpu));
> Which makes this per_cpu(,cpu) usage somewhat curious. What's wrong
> with
> __this_cpu_read() ?

Is __this_cpu_read() as fast as per_cpu(,cpu) on all
architectures?

> > 
> > +
> > +		delta = now - prev_softirq_start;
> > +		if (in_serving_softirq()) {
> > +			/*
> > +			 * Leaving softirq context. Avoid double
> > counting by
> > +			 * subtracting hardirq time from this
> > interval.
> > +			 */
> > +			s64 hi_delta = hardirq_time -
> > prev_hardirq;
> > +			delta -= hi_delta;
> > +		} else {
> > +			/* Entering softirq context. Note start
> > times. */
> > +			__this_cpu_write(softirq_start_time, now);
> > +			__this_cpu_write(prev_hardirq_time,
> > hardirq_time);
> > +		}
> > +		/*
> > +		 * If a hardirq happened during this calculation,
> > it may not
> > +		 * have gotten a consistent snapshot. Try again.
> > +		 */
> > +	} while (hardirq_time !=
> > READ_ONCE(per_cpu(cpu_hardirq_time, cpu)));
> That whole thing is somewhat hard to read; but its far too late for
> me
> to suggest anything more readable :/

I only had 2 1/2 hours of sleep last night, so I will not
try to rewrite it now, but I will see if there is anything
I can do to make it more readable tomorrow.

If you have any ideas before then, please let me know :)

> > 
> > +	irq_time_write_begin(irqtype);
> >  	/*
> >  	 * 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 (irqtype == HARDIRQ_OFFSET && hardirq_count())
> >  		__this_cpu_add(cpu_hardirq_time, delta);
> > +	else if (irqtype == SOFTIRQ_OFFSET && in_serving_softirq()
> > &&
> > +				curr != this_cpu_ksoftirqd())
> >  		__this_cpu_add(cpu_softirq_time, delta);
> >  
> > +	irq_time_write_end(irqtype);
> Maybe split the whole thing on irqtype at the very start, instead of
> the
> endless repeated branches?

Let me try if I can make things more readable that way.

Thanks for the review!

Rik
-- 
All Rights Reversed.

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

#1428236 — Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq

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

On Wed, 2016-06-22 at 00:28 +0200, Peter Zijlstra wrote:
> On Tue, Jun 21, 2016 at 06:23:34PM -0400, Rik van Riel wrote:
> > 
> > > > +	/*
> > > > +	 * Softirq context may get interrupted by hardirq
> > > > context,
> > > > +	 * on the same CPU. At softirq entry time the amount
> > > > of
> > > > time
> > > > +	 * spent in hardirq context is stored. At softirq exit
> > > > time,
> > > > +	 * the time spent in hardirq context during the
> > > > softirq is
> > > > +	 * subtracted.
> > > > +	 */
> > > > +	prev_hardirq = __this_cpu_read(prev_hardirq_time);
> > > > +	prev_softirq_start =
> > > > __this_cpu_read(softirq_start_time);
> > > > +
> > > > +	if (irqtype == HARDIRQ_OFFSET) {
> > > > +		delta = sched_clock_cpu(cpu) -
> > > > __this_cpu_read(hardirq_start_time);
> > > > +		__this_cpu_add(hardirq_start_time, delta);
> > > > +	} else do {
> > > > +		u64 now = sched_clock_cpu(cpu);
> > > > +		hardirq_time =
> > > > READ_ONCE(per_cpu(cpu_hardirq_time,
> > > > cpu));
> > > Which makes this per_cpu(,cpu) usage somewhat curious. What's
> > > wrong
> > > with
> > > __this_cpu_read() ?
> > Is __this_cpu_read() as fast as per_cpu(,cpu) on all
> > architectures?
> Can't be slower. Don't get the argument though; you've used
> __this_cpu
> stuff all over the place, and here you use a per_cpu() for no reason.
> 
Good point. I will use __this_cpu_read here.

> > > That whole thing is somewhat hard to read; but its far too late
> > > for
> > > me
> > > to suggest anything more readable :/
> > I only had 2 1/2 hours of sleep last night, so I will not
> > try to rewrite it now, but I will see if there is anything
> > I can do to make it more readable tomorrow.
> > 
> > If you have any ideas before then, please let me know :)
> Heh, step away from the computer ... ;-)

No worries, I have booze with me. Everything will be
just fine! ;)

-- 
All Rights Reversed.

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

#1428251 — Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq

On Tue, Jun 21, 2016 at 06:23:34PM -0400, Rik van Riel wrote:
> > >  	cpu = smp_processor_id();
> > Per this smp_processor_id() usage, preemption is disabled.
> 
> This code is called from the timer code. Surely preemption
> is already disabled?

That's what I said.

> > > 
> > > +	/*
> > > +	 * Softirq context may get interrupted by hardirq context,
> > > +	 * on the same CPU. At softirq entry time the amount of
> > > time
> > > +	 * spent in hardirq context is stored. At softirq exit
> > > time,
> > > +	 * the time spent in hardirq context during the softirq is
> > > +	 * subtracted.
> > > +	 */
> > > +	prev_hardirq = __this_cpu_read(prev_hardirq_time);
> > > +	prev_softirq_start = __this_cpu_read(softirq_start_time);
> > > +
> > > +	if (irqtype == HARDIRQ_OFFSET) {
> > > +		delta = sched_clock_cpu(cpu) -
> > > __this_cpu_read(hardirq_start_time);
> > > +		__this_cpu_add(hardirq_start_time, delta);
> > > +	} else do {
> > > +		u64 now = sched_clock_cpu(cpu);
> > > +		hardirq_time = READ_ONCE(per_cpu(cpu_hardirq_time,
> > > cpu));
> > Which makes this per_cpu(,cpu) usage somewhat curious. What's wrong
> > with
> > __this_cpu_read() ?
> 
> Is __this_cpu_read() as fast as per_cpu(,cpu) on all
> architectures?

Can't be slower. Don't get the argument though; you've used __this_cpu
stuff all over the place, and here you use a per_cpu() for no reason.

> > > 
> > > +
> > > +		delta = now - prev_softirq_start;
> > > +		if (in_serving_softirq()) {
> > > +			/*
> > > +			 * Leaving softirq context. Avoid double
> > > counting by
> > > +			 * subtracting hardirq time from this
> > > interval.
> > > +			 */
> > > +			s64 hi_delta = hardirq_time -
> > > prev_hardirq;
> > > +			delta -= hi_delta;
> > > +		} else {
> > > +			/* Entering softirq context. Note start
> > > times. */
> > > +			__this_cpu_write(softirq_start_time, now);
> > > +			__this_cpu_write(prev_hardirq_time,
> > > hardirq_time);
> > > +		}
> > > +		/*
> > > +		 * If a hardirq happened during this calculation,
> > > it may not
> > > +		 * have gotten a consistent snapshot. Try again.
> > > +		 */
> > > +	} while (hardirq_time !=
> > > READ_ONCE(per_cpu(cpu_hardirq_time, cpu)));
> > That whole thing is somewhat hard to read; but its far too late for
> > me
> > to suggest anything more readable :/
> 
> I only had 2 1/2 hours of sleep last night, so I will not
> try to rewrite it now, but I will see if there is anything
> I can do to make it more readable tomorrow.
> 
> If you have any ideas before then, please let me know :)

Heh, step away from the computer ... ;-)

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

#1428242 — Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq

On Thu, Jun 16, 2016 at 12:06:07PM -0400, riel@redhat.com wrote:
> @@ -53,36 +56,72 @@ 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)
>  {
> +	u64 prev_softirq_start;
> +	u64 prev_hardirq;
> +	u64 hardirq_time;
> +	s64 delta = 0;

We appear to always assign to delta, so this initialization seems
superfluous.

>  	int cpu;
>  
>  	if (!sched_clock_irqtime)
>  		return;
>  
>  	cpu = smp_processor_id();

Per this smp_processor_id() usage, preemption is disabled.

> +	/*
> +	 * Softirq context may get interrupted by hardirq context,
> +	 * on the same CPU. At softirq entry time the amount of time
> +	 * spent in hardirq context is stored. At softirq exit time,
> +	 * the time spent in hardirq context during the softirq is
> +	 * subtracted.
> +	 */
> +	prev_hardirq = __this_cpu_read(prev_hardirq_time);
> +	prev_softirq_start = __this_cpu_read(softirq_start_time);
> +
> +	if (irqtype == HARDIRQ_OFFSET) {
> +		delta = sched_clock_cpu(cpu) - __this_cpu_read(hardirq_start_time);
> +		__this_cpu_add(hardirq_start_time, delta);
> +	} else do {
> +		u64 now = sched_clock_cpu(cpu);
> +		hardirq_time = READ_ONCE(per_cpu(cpu_hardirq_time, cpu));

Which makes this per_cpu(,cpu) usage somewhat curious. What's wrong with
__this_cpu_read() ?

> +
> +		delta = now - prev_softirq_start;
> +		if (in_serving_softirq()) {
> +			/*
> +			 * Leaving softirq context. Avoid double counting by
> +			 * subtracting hardirq time from this interval.
> +			 */
> +			s64 hi_delta = hardirq_time - prev_hardirq;
> +			delta -= hi_delta;
> +		} else {
> +			/* Entering softirq context. Note start times. */
> +			__this_cpu_write(softirq_start_time, now);
> +			__this_cpu_write(prev_hardirq_time, hardirq_time);
> +		}
> +		/*
> +		 * If a hardirq happened during this calculation, it may not
> +		 * have gotten a consistent snapshot. Try again.
> +		 */
> +	} while (hardirq_time != READ_ONCE(per_cpu(cpu_hardirq_time, cpu)));

That whole thing is somewhat hard to read; but its far too late for me
to suggest anything more readable :/

> +	irq_time_write_begin(irqtype);
>  	/*
>  	 * 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 (irqtype == HARDIRQ_OFFSET && hardirq_count())
>  		__this_cpu_add(cpu_hardirq_time, delta);
> +	else if (irqtype == SOFTIRQ_OFFSET && in_serving_softirq() &&
> +				curr != this_cpu_ksoftirqd())
>  		__this_cpu_add(cpu_softirq_time, delta);
>  
> +	irq_time_write_end(irqtype);

Maybe split the whole thing on irqtype at the very start, instead of the
endless repeated branches?

>  }
>  EXPORT_SYMBOL_GPL(irqtime_account_irq);

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

#1429128 — Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq

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

On Tue, 2016-06-21 at 23:49 +0200, Peter Zijlstra wrote:
> On Thu, Jun 16, 2016 at 12:06:07PM -0400, riel@redhat.com wrote:
> > 
> > @@ -53,36 +56,72 @@ 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)
> >  {
> > +	u64 prev_softirq_start;
> > +	u64 prev_hardirq;
> > +	u64 hardirq_time;
> > +	s64 delta = 0;
> We appear to always assign to delta, so this initialization seems
> superfluous.

It gets rid of a compiler warning, since gcc is not
smart enough to know that the result of in_softirq()
will be the same throughout the function.

Using a bool leaving_softirq = in_softirq() also
gets rid of the warning, and makes the function a
little more readable, so I am doing that.

> > +	if (irqtype == HARDIRQ_OFFSET) {
> > +		delta = sched_clock_cpu(cpu) -
> > __this_cpu_read(hardirq_start_time);
> > +		__this_cpu_add(hardirq_start_time, delta);
> > +	} else do {
> > +		u64 now = sched_clock_cpu(cpu);
> > +		hardirq_time = READ_ONCE(per_cpu(cpu_hardirq_time,
> > cpu));
> Which makes this per_cpu(,cpu) usage somewhat curious. What's wrong
> with
> __this_cpu_read() ?

I played around with it a bit, and it seems that
__this_cpu_read does not want to nest inside
READ_ONCE.  Nobody else seems to be doing that,
either.

Back to READ_ONCE(per_cpu(,cpu)) it is...

> Maybe split the whole thing on irqtype at the very start, instead of
> the
> endless repeated branches?

I untangled the whole thing in the next version,
which I will post after testing.

-- 
All rights reversed

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

#1429860 — Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq

On 22/06/2016 23:55, Rik van Riel wrote:
> > > +		hardirq_time = READ_ONCE(per_cpu(cpu_hardirq_time,
> > > cpu));
> > Which makes this per_cpu(,cpu) usage somewhat curious. What's wrong
> > with __this_cpu_read() ?
>
> I played around with it a bit, and it seems that
> __this_cpu_read does not want to nest inside
> READ_ONCE.  Nobody else seems to be doing that,
> either.

According to arch/x86/include/asm/percpu.h, this_cpu_read always has 
READ_ONCE semantics, but I cannot find that in include/asm-generic
/percpu.h.  It probably just works because of all the layers of goo, but
something like this (101% untested) would make me feel safer:

diff --git a/include/asm-generic/percpu.h b/include/asm-generic/percpu.h
index 4d9f233c4ba8..d057568f1926 100644
--- a/include/asm-generic/percpu.h
+++ b/include/asm-generic/percpu.h
@@ -67,7 +67,7 @@ extern void setup_per_cpu_areas(void);
 
 #define raw_cpu_generic_to_op(pcp, val, op)				\
 do {									\
-	*raw_cpu_ptr(&(pcp)) op val;					\
+	ACCESS_ONCE(*raw_cpu_ptr(&(pcp))) op val;			\
 } while (0)
 
 #define raw_cpu_generic_add_return(pcp, val)				\
@@ -109,7 +109,7 @@ do {
 ({									\
 	typeof(pcp) __ret;						\
 	preempt_disable();						\
-	__ret = *this_cpu_ptr(&(pcp));					\
+	__ret = READ_ONCE(this_cpu_ptr(&(pcp)));			\
 	preempt_enable();						\
 	__ret;								\
 })
@@ -118,7 +118,7 @@ do {
 do {									\
 	unsigned long __flags;						\
 	raw_local_irq_save(__flags);					\
-	*raw_cpu_ptr(&(pcp)) op val;					\
+	ACCESS_ONCE(*raw_cpu_ptr(&(pcp))) op val;			\
 	raw_local_irq_restore(__flags);					\
 } while (0)
 
@@ -168,16 +168,16 @@ do {
 })
 
 #ifndef raw_cpu_read_1
-#define raw_cpu_read_1(pcp)		(*raw_cpu_ptr(&(pcp)))
+#define raw_cpu_read_1(pcp)		READ_ONCE(raw_cpu_ptr(&(pcp)))
 #endif
 #ifndef raw_cpu_read_2
-#define raw_cpu_read_2(pcp)		(*raw_cpu_ptr(&(pcp)))
+#define raw_cpu_read_2(pcp)		READ_ONCE(raw_cpu_ptr(&(pcp)))
 #endif
 #ifndef raw_cpu_read_4
-#define raw_cpu_read_4(pcp)		(*raw_cpu_ptr(&(pcp)))
+#define raw_cpu_read_4(pcp)		READ_ONCE(raw_cpu_ptr(&(pcp)))
 #endif
 #ifndef raw_cpu_read_8
-#define raw_cpu_read_8(pcp)		(*raw_cpu_ptr(&(pcp)))
+#define raw_cpu_read_8(pcp)		READ_ONCE(raw_cpu_ptr(&(pcp)))
 #endif
 
 #ifndef raw_cpu_write_1


> Back to READ_ONCE(per_cpu(,cpu)) it is...

What about READ_ONCE(this_cpu_ptr())?

Paolo

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

#1429920 — Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq

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

On Thu, 2016-06-23 at 15:52 +0200, Paolo Bonzini wrote:
> 
> On 22/06/2016 23:55, Rik van Riel wrote:
> > 
> > > 
> > > > 
> > > > +		hardirq_time =
> > > > READ_ONCE(per_cpu(cpu_hardirq_time,
> > > > cpu));
> > > Which makes this per_cpu(,cpu) usage somewhat curious. What's
> > > wrong
> > > with __this_cpu_read() ?
> > I played around with it a bit, and it seems that
> > __this_cpu_read does not want to nest inside
> > READ_ONCE.  Nobody else seems to be doing that,
> > either.
> According to arch/x86/include/asm/percpu.h, this_cpu_read always has 
> READ_ONCE semantics, but I cannot find that in include/asm-generic
> /percpu.h.  It probably just works because of all the layers of goo,
> but
> something like this (101% untested) would make me feel safer:
> 
Are READ_ONCE semantics desired for every
per-cpu read?

> > Back to READ_ONCE(per_cpu(,cpu)) it is...
> What about READ_ONCE(this_cpu_ptr())?

I tried that yesterday, because it looked like
it would work. Unfortunately, it does not.

  CC      kernel/sched/cputime.o
kernel/sched/cputime.c: In function ‘irqtime_account_irq’:
kernel/sched/cputime.c:107:71: warning: initialization makes pointer
from integer without a cast [-Wint-conversion]
kernel/sched/cputime.c:107:298: error: invalid type argument of unary
‘*’ (have ‘u64 {aka long long unsigned int}’)
kernel/sched/cputime.c:107:428: warning: initialization makes pointer
from integer without a cast [-Wint-conversion]
kernel/sched/cputime.c:107:655: error: invalid type argument of unary
‘*’ (have ‘u64 {aka long long unsigned int}’)
kernel/sched/cputime.c:107:749: warning: initialization makes pointer
from integer without a cast [-Wint-conversion]
kernel/sched/cputime.c:107:976: error: invalid type argument of unary
‘*’ (have ‘u64 {aka long long unsigned int}’)
kernel/sched/cputime.c:107:1087: warning: initialization makes pointer
from integer without a cast [-Wint-conversion]
kernel/sched/cputime.c:107:1314: error: invalid type argument of unary
‘*’ (have ‘u64 {aka long long unsigned int}’)
kernel/sched/cputime.c:107:1408: warning: initialization makes pointer
from integer without a cast [-Wint-conversion]
kernel/sched/cputime.c:107:1635: error: invalid type argument of unary
‘*’ (have ‘u64 {aka long long unsigned int}’)
kernel/sched/cputime.c: In function ‘irqtime_account_hi_update’:
kernel/sched/cputime.c:145:175: warning: comparison of distinct pointer
types lacks a cast
kernel/sched/cputime.c: In function ‘irqtime_account_si_update’:
kernel/sched/cputime.c:162:182: warning: comparison of distinct pointer
types lacks a cast
scripts/Makefile.build:291: recipe for target 'kernel/sched/cputime.o'
failed
make[1]: *** [kernel/sched/cputime.o] Error 1
Makefile:1594: recipe for target 'kernel/sched/' failed
make: *** [kernel/sched/] Error 2

-- 
All rights reversed

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web