commit b00bc0b237055b breaking perf subsystem

commit b00bc0b237055b breaking perf subsystem

[Arjan van de Ven]

commit b00bc0b237055b4c45816325ee14f0bd83e6f590
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Mon Nov 2 13:01:56 2009 +0100

    uids: Prevent tear down race

has been bisected to break the perf system between -rc5 and -rc7;
and reverting this patch in rc7 also fixes the issue.

now this patch is.... fun and seemingly unrelated to perf, so I
am pretty sure something else is going on.

the symptom of the breakage is that a userland client of perf
(in my case, powertop) does not get to see new events if there
is only, say, one or two of them. The data_head pointer just does
not get updated.

Based on the patch, I have a suspicion that this pointer gets updated
from synchronize RCU context, and this patch just makes that happen
more.

If that is the case, what we really need is a new ioctl to perf that
will cause all pending buffer state to be flushed to the ring buffer,
that applications using perf can then just always call before looking
at the ring. As a concept, that sounds like a good idea to me anyway...

comments/suggestions/saying-I'm-full-of-sh*t ?



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: commit b00bc0b237055b breaking perf subsystem

[Peter Zijlstra]

On Thu, 2009-11-19 at 11:50 -0800, Arjan van de Ven wrote:
> commit b00bc0b237055b4c45816325ee14f0bd83e6f590
> Author: Thomas Gleixner <tglx@linutronix.de>
> Date:   Mon Nov 2 13:01:56 2009 +0100
> 
>     uids: Prevent tear down race
> 
> has been bisected to break the perf system between -rc5 and -rc7;
> and reverting this patch in rc7 also fixes the issue.
> 
> now this patch is.... fun and seemingly unrelated to perf, so I
> am pretty sure something else is going on.
> 
> the symptom of the breakage is that a userland client of perf
> (in my case, powertop) does not get to see new events if there
> is only, say, one or two of them. The data_head pointer just does
> not get updated.
> 
> Based on the patch, I have a suspicion that this pointer gets updated
> from synchronize RCU context, and this patch just makes that happen
> more.
> 
> If that is the case, what we really need is a new ioctl to perf that
> will cause all pending buffer state to be flushed to the ring buffer,
> that applications using perf can then just always call before looking
> at the ring. As a concept, that sounds like a good idea to me anyway...
> 
> comments/suggestions/saying-I'm-full-of-sh*t ?

We should be updating the data_head pointer after every writen event
(with the exception of nested writes, where we delay publishing until
the last writer is done, so as to not expose incomplete data).

  perf_output_begin(); /* reserve space */
  perf_output_copy();  /* write bits to space */
  perf_output_end();   /* commit and publish */
    perf_output_unlock();
      /* complicated stuff to publish data_head */
      
Colour me puzzled on how this particular commit can have any effect on
data_head.

Re: commit b00bc0b237055b breaking perf subsystem

[Thomas Gleixner]

On Thu, 19 Nov 2009, Arjan van de Ven wrote:
> commit b00bc0b237055b4c45816325ee14f0bd83e6f590
> Author: Thomas Gleixner <tglx@linutronix.de>
> Date:   Mon Nov 2 13:01:56 2009 +0100
> 
>     uids: Prevent tear down race
> 
> has been bisected to break the perf system between -rc5 and -rc7;
> and reverting this patch in rc7 also fixes the issue.

Sigh, today seems to be the day of bisect madness.

I'm not buying direct related perf breakage from fixing that race.

Yes, I'm paranoid, but:
     Does your config have CONFIG_USER_SCHED=y ?

> now this patch is.... fun and seemingly unrelated to perf, so I
> am pretty sure something else is going on.

I fully agree, but ...
 
> the symptom of the breakage is that a userland client of perf
> (in my case, powertop) does not get to see new events if there
> is only, say, one or two of them. The data_head pointer just does
> not get updated.
> 
> Based on the patch, I have a suspicion that this pointer gets updated
> from synchronize RCU context, and this patch just makes that happen
> more.

... I have no idea how you associate that patch with the data_head
pointer and why this patch increases the sync RCU update frequency.

> If that is the case, what we really need is a new ioctl to perf that
> will cause all pending buffer state to be flushed to the ring buffer,
> that applications using perf can then just always call before looking
> at the ring. As a concept, that sounds like a good idea to me anyway...
> 
> comments/suggestions/saying-I'm-full-of-sh*t ?

I fear the latter unless you come up with some coherent explanation of
connecting that patch to rcu and the data_head pointer.

Thanks,

	tglx