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

Thomas Gleixner
2016-05-18T17:37:24Z

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