Re: [RFC][PATCH 8/8] perf: Rework the PMU methods

[Frederic Weisbecker <fweisbec@gmail.com>]

On Wed, Jun 16, 2010 at 06:00:35PM +0200, Peter Zijlstra wrote:
> -static void x86_pmu_stop(struct perf_event *event)
> +static void x86_pmu_stop(struct perf_event *event, int flags)
>  {
> -	struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> -	struct hw_perf_event *hwc = &event->hw;
> -	int idx = hwc->idx;
> -
>  	if (!__test_and_clear_bit(idx, cpuc->active_mask))
> -		return;



Do you still need active_mask now that you have HES_STOPPED?



> @@ -4069,6 +4051,9 @@ static int perf_swevent_match(struct per
>  				struct perf_sample_data *data,
>  				struct pt_regs *regs)
>  {
> +	if (event->hw.state)
> +		return 0;
> +
>  	if (event->attr.type != type)
>  		return 0;
>  
> @@ -4211,7 +4196,7 @@ static void perf_swevent_read(struct per
>  {
>  }
>  
> -static int perf_swevent_enable(struct perf_event *event)
> +static int perf_swevent_add(struct perf_event *event, int flags)
>  {
>  	struct hw_perf_event *hwc = &event->hw;
>  	struct perf_cpu_context *cpuctx;
> @@ -4224,6 +4209,8 @@ static int perf_swevent_enable(struct pe
>  		perf_swevent_set_period(event);
>  	}
>  
> +	hwc->state = !(flags & PERF_EF_START);
> +
>  	head = find_swevent_head(cpuctx, event);
>  	if (WARN_ON_ONCE(!head))
>  		return -EINVAL;
> @@ -4233,13 +4220,19 @@ static int perf_swevent_enable(struct pe
>  	return 0;
>  }
>  
> -static void perf_swevent_disable(struct perf_event *event)
> +static void perf_swevent_del(struct perf_event *event, int flags)
>  {
>  	hlist_del_rcu(&event->hlist_entry);
>  }
>  
> -static void perf_swevent_void(struct perf_event *event)
> +static void perf_swevent_start(struct perf_event *event, int flags)
> +{
> +	event->hw.state = 0;
> +}
> +
> +static void perf_swevent_stop(struct perf_event *event, int flags)
>  {
> +	event->hw.state = 1;
>  }


So, instead of doing this and add yet another check in the fast path,
what about just playing with the hlist insertion and deletion?

And if we have PERF_EF_RELOAD in start or PERF_EF_UPDATE in stop,
we simply do nothing, as we know it's just about updating the counter
or reset the interrupt, things that software events just don't care.

And in ->add, you can also do nothing if PERF_EF_START.

It would be nice to have a PERF_EF_STOP as well in ->del, so that
each pmu don't need to maintain an internal state.
If we assume the generic code will never imbalance add/start/stop/del,
or call start after add(PERF_EF_START), or things like this, pmus
like this don't need to ever touch event->hw.state. It's only
necessary for hardware events that call their start/stop internally.


>  static inline void perf_tp_register(void)
> @@ -4546,7 +4537,7 @@ void perf_bp_event(struct perf_event *bp
>  
>  	perf_sample_data_init(&sample, bp->attr.bp_addr);
>  
> -	if (!perf_exclude_event(bp, regs))
> +	if (!bp->hw.state && !perf_exclude_event(bp, regs))
>  		perf_swevent_add(bp, 1, 1, &sample, regs);



Same thing here, and same for trace events.



>  }
>  #endif
> @@ -4591,12 +4582,12 @@ static void perf_swevent_start_hrtimer(s
>  	if (hwc->sample_period) {
>  		u64 period;
>  
> -		if (hwc->remaining) {
> -			if (hwc->remaining < 0)
> +		if (hwc->period_left) {
> +			if (hwc->period_left < 0)
>  				period = 10000;
>  			else
> -				period = hwc->remaining;
> -			hwc->remaining = 0;
> +				period = hwc->period_left;
> +			hwc->period_left = 0;



If remaining can be replaced by period_left, it should probably be done
in another patch.