Re: [RFC][PATCH 09/11] perf: Default PMU ops

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, Jun 24, 2010 at 04:28:13PM +0200, Peter Zijlstra wrote:
> Provide default implementations for the pmu txn methods, this allows
> us to remove some conditional code.
> 
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  kernel/perf_event.c |   48 ++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 12 deletions(-)
> 
> Index: linux-2.6/kernel/perf_event.c
> ===================================================================
> --- linux-2.6.orig/kernel/perf_event.c
> +++ linux-2.6/kernel/perf_event.c
> @@ -656,21 +656,14 @@ group_sched_in(struct perf_event *group_
>  {
>  	struct perf_event *event, *partial_group = NULL;
>  	struct pmu *pmu = group_event->pmu;
> -	bool txn = false;
>  
>  	if (group_event->state == PERF_EVENT_STATE_OFF)
>  		return 0;
>  
> -	/* Check if group transaction availabe */
> -	if (pmu->start_txn)
> -		txn = true;
> -
> -	if (txn)
> -		pmu->start_txn(pmu);
> +	pmu->start_txn(pmu);
>  
>  	if (event_sched_in(group_event, cpuctx, ctx)) {
> -		if (txn)
> -			pmu->cancel_txn(pmu);
> +		pmu->cancel_txn(pmu);
>  		return -EAGAIN;
>  	}
>  
> @@ -684,7 +677,7 @@ group_sched_in(struct perf_event *group_
>  		}
>  	}
>  
> -	if (!txn || !pmu->commit_txn(pmu))
> +	if (!pmu->commit_txn(pmu))
>  		return 0;
>  
>  group_error:
> @@ -699,8 +692,7 @@ group_error:
>  	}
>  	event_sched_out(group_event, cpuctx, ctx);
>  
> -	if (txn)
> -		pmu->cancel_txn(pmu);
> +	pmu->cancel_txn(pmu);
>  
>  	return -EAGAIN;
>  }
> @@ -4755,6 +4747,26 @@ static struct list_head pmus;
>  static DEFINE_MUTEX(pmus_lock);
>  static struct srcu_struct pmus_srcu;
>  
> +static void perf_pmu_nop(struct pmu *pmu)
> +{
> +}
> +
> +static void perf_pmu_start_txn(struct pmu *pmu)
> +{
> +	perf_pmu_disable(pmu);
> +}
> +
> +static int perf_pmu_commit_txn(struct pmu *pmu)
> +{
> +	perf_pmu_enable(pmu);
> +	return 0;
> +}
> +
> +static void perf_pmu_cancel_txn(struct pmu *pmu)
> +{
> +	perf_pmu_enable(pmu);
> +}


So why do you need perf_pmu_*able wrappers now that you brings stubs
if none is provided?

Actually, one problem is that it makes calling two indirect nops
for software events.

Should the txn things really map to the enable/disable ops is the
off-case? Probably better let pmu implementations deal with that.
If they didn't provide txn implementations, it means they don't need it,
hence it should directly map to a nop.