RE: [patch 04/18] x86, bts: wait until traced task has beenscheduled out

[Metzger, Markus T]

>-----Original Message-----
>From: Ingo Molnar [mailto:mingo@xxxxxxx]
>Sent: Friday, April 03, 2009 1:32 PM
>To: Metzger, Markus T
>Cc: tglx@xxxxxxxxxxxxx; hpa@xxxxxxxxx; markus.t.metzger@xxxxxxxxx; roland@xxxxxxxxxx;
>eranian@xxxxxxxxxxxxxx; oleg@xxxxxxxxxx; Villacis, Juan; ak@xxxxxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx
>Subject: Re: [patch 04/18] x86, bts: wait until traced task has been scheduled out
>
>
>* Metzger, Markus T <markus.t.metzger@xxxxxxxxx> wrote:
>
>> >-----Original Message-----
>> >From: Ingo Molnar [mailto:mingo@xxxxxxx]
>> >Sent: Thursday, April 02, 2009 9:18 PM
>> >To: Metzger, Markus T
>> >Cc: tglx@xxxxxxxxxxxxx; hpa@xxxxxxxxx; markus.t.metzger@xxxxxxxxx; roland@xxxxxxxxxx;
>> >eranian@xxxxxxxxxxxxxx; oleg@xxxxxxxxxx; Villacis, Juan; ak@xxxxxxxxxxxxxxxxxx; linux-
>> >kernel@xxxxxxxxxxxxxxx
>> >Subject: Re: [patch 04/18] x86, bts: wait until traced task has been scheduled out
>> >
>> >
>> >* markus.t.metzger@xxxxxxxxx <markus.t.metzger@xxxxxxxxx> wrote:
>> >
>> >> In order to stop branch tracing for a running task, we need to
>> >> first clear the branch tracing control bits before we may free the
>> >> tracing buffer. If the traced task is running, the cpu might still
>> >> trace that task after the branch trace control bits have cleared.
>> >>
>> >> Wait until the traced task has been scheduled out before
>> >> proceeding.
>> >>
>> >> A similar problem affects the task debug store context. We first
>> >> remove the context, then we need to wait until the task has been
>> >> scheduled out before we can free the context memory.
>> >>
>> >>
>> >> Reviewed-by: Oleg Nesterov <oleg@xxxxxxxxxx>
>> >> Signed-off-by: Markus Metzger <markus.t.metzger@xxxxxxxxx>
>> >> ---
>> >>  arch/x86/kernel/ds.c |   40 	40 +	0 -	0 !
>> >>  1 file changed, 40 insertions(+)
>> >>
>> >> Index: b/arch/x86/kernel/ds.c
>> >> ===================================================================
>> >> --- a/arch/x86/kernel/ds.c
>> >> +++ b/arch/x86/kernel/ds.c
>> >> @@ -250,6 +250,40 @@ static DEFINE_PER_CPU(struct ds_context
>> >>  #define system_context per_cpu(system_context_array, smp_processor_id())
>> >>
>> >>
>> >> +/*
>> >> + * Wait for the traced task to unschedule.
>> >> + *
>> >> + * This guarantees that the bts trace configuration has been
>> >> + * synchronized with the cpu executing the task.
>> >> + */
>> >> +static void wait_to_unschedule(struct task_struct *task)
>> >> +{
>> >
>> >this should be in sched.c and task_is_running() should not be
>> >exported from there.
>> >
>> >I.e. your original patch which i objected to is probably the right
>> >one, but should be named something like "task_wait_context_switch()"
>> >- which signals its purpose: that it is to wait for the task to
>> >context-switch at least once, so that its ptrace state is installed
>> >(or deinstalled) for sure.
>>
>> OK.
>>
>> I'll move it to sched.c.
>>
>> In that case, I would use task_running() without holding the rq
>> lock, since we don't really care whether we read an old value or
>> not. Would that be OK with you?
>
>i'd have to see that in full context - reading non-locked results
>can in essence result in stale old values being read out, regardless
>of current reality. task_running() is normally used within the rq
>lock.

I need to take the rq lock, anyway, since rq->curr is switched
before the context switch.
I do not try to optimize and read without the rq-lock first, since
its too likely that I have to repeat the read with lock held, anyway.

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/