RE: [tip:tracing/hw-branch-tracing] tracing/hw-branch-tracing: convert bts-tracer mutex to a spinlock

RE: [tip:tracing/hw-branch-tracing] tracing/hw-branch-tracing: convert bts-tracer mutex to a spinlock

[Metzger, Markus T]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2044 bytes --]

>-----Original Message-----
>From: Ingo Molnar [mailto:mingo@elte.hu]
>Sent: Wednesday, February 25, 2009 9:21 AM


>bts_hotcpu_handler() is called with irqs disabled, so using mutex_lock()
>is a no-no.
>
>All the BTS codepaths here are atomic (they do not schedule), so using
>a spinlock is the right solution.

I introduced the lock to protect against a race between bts_trace_start/stop()
and bts_hotcpu_handler().

Assuming that the hw-branch-tracer is removed and at the same time a cpu
comes online, we might be left with a disabled tracer but still trace
that new cpu.

I wonder whether a simple get/put_online_cpus() would suffice, i.e.

static void bts_trace_start(struct trace_array *tr)
{
	get_online_cpus();

 	on_each_cpu(bts_trace_start_cpu, NULL, 1);
 	trace_hw_branches_enabled = 1;

	put_online_cpus();
}



> static void trace_bts_prepare(struct trace_iterator *iter)
> {
>-	mutex_lock(&bts_tracer_mutex);
>+	spin_lock(&bts_tracer_lock);
>
> 	on_each_cpu(trace_bts_cpu, iter->tr, 1);
>
>-	mutex_unlock(&bts_tracer_mutex);
>+	spin_unlock(&bts_tracer_lock);
> }

Whereas start/stop are relatively fast, the above operation is rather
expensive. Would it make sense to use schedule_on_each_cpu() instead
of on_each_cpu()?

regards,
markus.

---------------------------------------------------------------------Intel GmbHDornacher Strasse 185622 Feldkirchen/Muenchen GermanySitz der Gesellschaft: Feldkirchen bei MuenchenGeschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes SchwadererRegistergericht: Muenchen HRB 47456 Ust.-IdNr.VAT Registration No.: DE129385895Citibank Frankfurt (BLZ 502 109 00) 600119052
This e-mail and any attachments may contain confidential material forthe sole use of the intended recipient(s). Any review or distributionby others is strictly prohibited. If you are not the intendedrecipient, please contact the sender and delete all copies.ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [tip:tracing/hw-branch-tracing] tracing/hw-branch-tracing: convert bts-tracer mutex to a spinlock

[Ingo Molnar]

* Metzger, Markus T <markus.t.metzger@intel.com> wrote:

> > static void trace_bts_prepare(struct trace_iterator *iter)
> > {
> >-	mutex_lock(&bts_tracer_mutex);
> >+	spin_lock(&bts_tracer_lock);
> >
> > 	on_each_cpu(trace_bts_cpu, iter->tr, 1);
> >
> >-	mutex_unlock(&bts_tracer_mutex);
> >+	spin_unlock(&bts_tracer_lock);
> > }
> 
> Whereas start/stop are relatively fast, the above operation is 
> rather expensive. Would it make sense to use 
> schedule_on_each_cpu() instead of on_each_cpu()?

it's perfectly fine to do that on_each_cpu() under the spinlock. 
schedule_on_each_cpu() would likely be more expensive - and for 
no good reason.

	Ingo

RE: [tip:tracing/hw-branch-tracing] tracing/hw-branch-tracing: convert bts-tracer mutex to a spinlock

[Metzger, Markus T]

>-----Original Message-----
>From: Ingo Molnar [mailto:mingo@elte.hu]
>Sent: Wednesday, February 25, 2009 10:58 AM
>To: Metzger, Markus T


>* Metzger, Markus T <markus.t.metzger@intel.com> wrote:
>
>> > static void trace_bts_prepare(struct trace_iterator *iter)
>> > {
>> >-    mutex_lock(&bts_tracer_mutex);
>> >+    spin_lock(&bts_tracer_lock);
>> >
>> >     on_each_cpu(trace_bts_cpu, iter->tr, 1);
>> >
>> >-    mutex_unlock(&bts_tracer_mutex);
>> >+    spin_unlock(&bts_tracer_lock);
>> > }
>>
>> Whereas start/stop are relatively fast, the above operation is
>> rather expensive. Would it make sense to use
>> schedule_on_each_cpu() instead of on_each_cpu()?
>
>it's perfectly fine to do that on_each_cpu() under the spinlock.
>schedule_on_each_cpu() would likely be more expensive - and for
>no good reason.

OK. 

And I assume you like the spinlock better than the
get/put_online_cpus(), as well.

regards,
markus.

---------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [tip:tracing/hw-branch-tracing] tracing/hw-branch-tracing: convert bts-tracer mutex to a spinlock

[Ingo Molnar]

* Metzger, Markus T <markus.t.metzger@intel.com> wrote:

> >-----Original Message-----
> >From: Ingo Molnar [mailto:mingo@elte.hu]
> >Sent: Wednesday, February 25, 2009 10:58 AM
> >To: Metzger, Markus T
> 
> 
> >* Metzger, Markus T <markus.t.metzger@intel.com> wrote:
> >
> >> > static void trace_bts_prepare(struct trace_iterator *iter)
> >> > {
> >> >-    mutex_lock(&bts_tracer_mutex);
> >> >+    spin_lock(&bts_tracer_lock);
> >> >
> >> >     on_each_cpu(trace_bts_cpu, iter->tr, 1);
> >> >
> >> >-    mutex_unlock(&bts_tracer_mutex);
> >> >+    spin_unlock(&bts_tracer_lock);
> >> > }
> >>
> >> Whereas start/stop are relatively fast, the above operation is
> >> rather expensive. Would it make sense to use
> >> schedule_on_each_cpu() instead of on_each_cpu()?
> >
> >it's perfectly fine to do that on_each_cpu() under the spinlock.
> >schedule_on_each_cpu() would likely be more expensive - and for
> >no good reason.
> 
> OK. 
> 
> And I assume you like the spinlock better than the
> get/put_online_cpus(), as well.

yeah - and get/put_online_cpus is sleepable too, so it doesnt 
really help unless i'm missing something ...

	Ingo

RE: [tip:tracing/hw-branch-tracing] tracing/hw-branch-tracing: convert bts-tracer mutex to a spinlock

[Metzger, Markus T]

>-----Original Message-----
>From: Ingo Molnar [mailto:mingo@elte.hu]
>Sent: Wednesday, February 25, 2009 12:11 PM
>To: Metzger, Markus T
>Cc: hpa@zytor.com; mingo@redhat.com; tglx@linutronix.de; linux-kernel@vger.kernel.org; linux-tip-
>commits@vger.kernel.org


>> And I assume you like the spinlock better than the
>> get/put_online_cpus(), as well.
>
>yeah - and get/put_online_cpus is sleepable too, so it doesnt
>really help unless i'm missing something ...

I suggested to use get/put_online_cpus() instead of the lock.

The hotplug code waits until the cpu_hotplug.refcount is zero
and it holds the cpu_hotplug.lock during hotplug operations
(see cpu_hotplug_begin/done()).

In turn, get_online_cpus() needs to grab the cpu_hotplug.lock
to increment the cpu_hotplug.refcount.

Thus, we will use the cpu_hotplug.lock instead of our own lock.

regards,
markus.

---------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [tip:tracing/hw-branch-tracing] tracing/hw-branch-tracing: convert bts-tracer mutex to a spinlock

[Ingo Molnar]

* Metzger, Markus T <markus.t.metzger@intel.com> wrote:

> >-----Original Message-----
> >From: Ingo Molnar [mailto:mingo@elte.hu]
> >Sent: Wednesday, February 25, 2009 12:11 PM
> >To: Metzger, Markus T
> >Cc: hpa@zytor.com; mingo@redhat.com; tglx@linutronix.de; linux-kernel@vger.kernel.org; linux-tip-
> >commits@vger.kernel.org
> 
> 
> >> And I assume you like the spinlock better than the
> >> get/put_online_cpus(), as well.
> >
> >yeah - and get/put_online_cpus is sleepable too, so it doesnt
> >really help unless i'm missing something ...
> 
> I suggested to use get/put_online_cpus() instead of the lock.
> 
> The hotplug code waits until the cpu_hotplug.refcount is zero
> and it holds the cpu_hotplug.lock during hotplug operations
> (see cpu_hotplug_begin/done()).
> 
> In turn, get_online_cpus() needs to grab the cpu_hotplug.lock
> to increment the cpu_hotplug.refcount.
> 
> Thus, we will use the cpu_hotplug.lock instead of our own lock.

... which, if you use it in the exact same spots as now still 
does a potential sleep with irqs disabled => bad.

We might be able to not take the hotplug lock in the affected 
codepath, but we should really not expand on the use of that 
lock and should make this code self-sufficient.

	Ingo

RE: [tip:tracing/hw-branch-tracing] tracing/hw-branch-tracing: convert bts-tracer mutex to a spinlock

[Metzger, Markus T]

>-----Original Message-----
>From: Ingo Molnar [mailto:mingo@elte.hu]
>Sent: Wednesday, February 25, 2009 12:46 PM
>To: Metzger, Markus T


>> I suggested to use get/put_online_cpus() instead of the lock.
>>
>> The hotplug code waits until the cpu_hotplug.refcount is zero
>> and it holds the cpu_hotplug.lock during hotplug operations
>> (see cpu_hotplug_begin/done()).
>>
>> In turn, get_online_cpus() needs to grab the cpu_hotplug.lock
>> to increment the cpu_hotplug.refcount.
>>
>> Thus, we will use the cpu_hotplug.lock instead of our own lock.
>
>... which, if you use it in the exact same spots as now still
>does a potential sleep with irqs disabled => bad.

We don't need to get_online_cpus() in the hotplug handler;
actually, we must not.


>We might be able to not take the hotplug lock in the affected
>codepath, but we should really not expand on the use of that
>lock and should make this code self-sufficient.

You're right. An own spinlock is definitely safer and much
easier to understand.


thanks and regards,
markus.

---------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.