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

Metzger, Markus T
2009-04-03T06:24:23Z

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

thanks,
markus.