RE: [patch 01/18] x86, bts: fix race when bts tracer is removed

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

>-----Original Message-----
>From: Ingo Molnar [mailto:mingo@elte.hu]
>Sent: Thursday, April 02, 2009 8:45 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 01/18] x86, bts: fix race when bts tracer is removed
>
>
>* markus.t.metzger@intel.com <markus.t.metzger@intel.com> wrote:
>
>> +static inline void ds_take_timestamp(struct ds_context *context,
>> + enum bts_qualifier qualifier,
>> + struct task_struct *task)
>> +{
>> + struct bts_tracer *tracer = context->bts_master;
>> + barrier();
>
>why the barrier()?

See http://lkml.org/lkml/2009/3/31/544

Oleg: "In theory, we need barrier() after reading ->bts_master.

(actually, I did see the bug reports when the compiler read the pointer
twice with the code like above)."

>> +
>> + if (tracer && (tracer->flags & BTS_TIMESTAMPS)) {
>> + struct bts_struct ts = {
>> + .qualifier = qualifier,
>> + .variant.timestamp.jiffies = jiffies_64,
>> + .variant.timestamp.pid = task->pid
>> + };
>> + bts_write(tracer, &ts);
>> + }
>
>Why do we have .variant.timestamp.pid ? A PID is not a timestamp. It
>might be .event.jiffies and .event.pid perhaps.

OK.

>Also, the whole function could be cleaned up by:
>
> 1) returning early if !tracer || !(tracer->flags & BTS_TIMESTAMPS).
>
> 2) Doing a cleaner initialization - something like:
>
>struct bts_struct ts = {
> .qualifier = qualifier,
> .variant.event.jiffies = jiffies_64,
> .variant.event.pid = task->pid
>};
>
>Also, raw use of jiffies_64 is buggy and racy. Why does this use
>jiffies to begin with - why not some finer grained time?

What would be a good time to use?

thanks,
markus.