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

Ingo Molnar
2009-04-02T18:45:48Z

* 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()?

> +
> + 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.

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?

Ingo