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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.11.1605181931540.3851@nanos>
Date:	Wed, 18 May 2016 19:35:22 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	David Carrillo-Cisneros <davidcc@...gle.com>
cc:	Peter Zijlstra <peterz@...radead.org>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Ingo Molnar <mingo@...hat.com>,
	Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
	Matt Fleming <matt@...eblueprint.co.uk>,
	Tony Luck <tony.luck@...el.com>,
	Stephane Eranian <eranian@...gle.com>,
	Paul Turner <pjt@...gle.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 07/32] perf/x86/intel/cqm: add helpers for per-package
 locking

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@...gle.com>
> Signed-off-by: David Carrillo-Cisneros <davidcc@...gle.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