Re: [PATCH v2 07/32] perf/x86/intel/cqm: add helpers for per-package locking

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 11 May 2016, David Carrillo-Cisneros wrote:
> Add helper macros and functions to acquire and release the locks
> and mutexes in pkg_data.

Why? Whatfor do we need nested locks?

> Reviewed-by: Stephane Eranian <eranian@google.com>
> Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> ---
>  arch/x86/events/intel/cqm.h | 78 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/arch/x86/events/intel/cqm.h b/arch/x86/events/intel/cqm.h
> index 08623b5..7837db0 100644
> --- a/arch/x86/events/intel/cqm.h
> +++ b/arch/x86/events/intel/cqm.h
> @@ -96,6 +96,84 @@ static inline u16 __cqm_pkgs_data_first_online(void)
>  #define __pkg_data(pmonr, member) cqm_pkgs_data[pmonr->pkg_id]->member
>  
>  /*
> + * Utility function and macros to manage per-package locks.
> + * Use macros to keep flags in caller's stace.

stace? 

You can do that with inlines as well.

> + * Hold lock in all the packages, required to alter the monr hierarchy

And what's monr? 

> + */
> +static inline void monr_hrchy_acquire_mutexes(void)
> +{
> +	int i;
> +
> +	cqm_pkg_id_for_each_online(i)
> +		mutex_lock_nested(&cqm_pkgs_data[i]->pkg_data_mutex, i);
> +}
> +
> +# define monr_hrchy_acquire_raw_spin_locks_irq_save(flags, i) \

Why on earth do you want to hand in 'i' ?

> +	do { \
> +		raw_local_irq_save(flags); \
> +		cqm_pkg_id_for_each_online(i) {\
> +			raw_spin_lock_nested( \
> +				&cqm_pkgs_data[i]->pkg_data_lock, i); \
> +		} \
> +	} while (0)

All of this can be done with readable inlines.

> +#ifdef CONFIG_LOCKDEP
> +static inline int monr_hrchy_count_held_raw_spin_locks(void)
> +{
> +	int i, nr_held = 0;
> +
> +	cqm_pkg_id_for_each_online(i) {
> +		if (lockdep_is_held(&cqm_pkgs_data[i]->pkg_data_lock))
> +			nr_held++;
> +	}
> +	return nr_held;
> +}

And we need this because it looks neat?

Thanks,

	tglx