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

Metzger, Markus T
2009-04-03T11:37:29Z

>-----Original Message-----
>From: Ingo Molnar [mailto:mingo@elte.hu]
>Sent: Friday, April 03, 2009 1:32 PM
>To: Metzger, Markus T
>Cc: tglx@linutronix.de; hpa@zytor.com; markus.t.metzger@gmail.com; roland@redhat.com;
>eranian@googlemail.com; oleg@redhat.com; Villacis, Juan; ak@linux.jf.intel.com; linux-
>kernel@vger.kernel.org
>Subject: Re: [patch 04/18] x86, bts: wait until traced task has been scheduled out
>
>
>* Metzger, Markus T <markus.t.metzger@intel.com> wrote:
>
>> >-----Original Message-----
>> >From: Ingo Molnar [mailto:mingo@elte.hu]
>> >Sent: Thursday, April 02, 2009 9:18 PM
>> >To: Metzger, Markus T
>> >Cc: tglx@linutronix.de; hpa@zytor.com; markus.t.metzger@gmail.com; roland@redhat.com;
>> >eranian@googlemail.com; oleg@redhat.com; Villacis, Juan; ak@linux.jf.intel.com; linux-
>> >kernel@vger.kernel.org
>> >Subject: Re: [patch 04/18] x86, bts: wait until traced task has been scheduled out
>> >
>> >
>> >* markus.t.metzger@intel.com <markus.t.metzger@intel.com> 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@redhat.com>
>> >> Signed-off-by: Markus Metzger <markus.t.metzger@intel.com>
>> >> ---
>> >> 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.