Paul Mackerras [pau...@samba.org] wrote:
| > +   /*
| > +    * If this event was disabled in record_and_restart() because we
| > +    * exceeded the ->event_limit, this is probably a good time to
| > +    * re-enable the event ? If we don't reenable the event, we will
| > +    * never notify the user again about this event.
| > +    */
| 
| The comment seems a bit tentative. :)  If the PERF_EF_START bit is set
| then we are being told to restart the event.
| 
| >     if (!(ef_flags & PERF_EF_START))
| >             event->hw.state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
| > +   else
| > +           event->hw.state &= ~PERF_HES_STOPPED;
| 
| This looks fine, though I think you could equally well just set
| event->hw.state to 0 in the else clause.  That would clear the
| UPTODATE flag too, which is appropriate since we are about to put the
| event on a hardware counter.

Agree. I submitted a new patch with better comments and patch description
and cleared the state to 0.

Thanks for the review.

Sukadev

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Reply via email to