From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S932796AbcERRhY (ORCPT <rfc822;w@1wt.eu>);
	Wed, 18 May 2016 13:37:24 -0400
Received: from www.linutronix.de ([62.245.132.108]:43577 "EHLO
	Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752779AbcERRhX (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 18 May 2016 13:37:23 -0400
Date: Wed, 18 May 2016 19:35:22 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: David Carrillo-Cisneros <davidcc@google.com>
cc: Peter Zijlstra <peterz@infradead.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        Vikas Shivappa <vikas.shivappa@linux.intel.com>,
        Matt Fleming <matt@codeblueprint.co.uk>,
        Tony Luck <tony.luck@intel.com>, Stephane Eranian <eranian@google.com>,
        Paul Turner <pjt@google.com>, x86@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 07/32] perf/x86/intel/cqm: add helpers for per-package
 locking
In-Reply-To: <1463007752-116802-8-git-send-email-davidcc@google.com>
Message-ID: <alpine.DEB.2.11.1605181931540.3851@nanos>
References: <1463007752-116802-1-git-send-email-davidcc@google.com> <1463007752-116802-8-git-send-email-davidcc@google.com>
User-Agent: Alpine 2.11 (DEB 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
X-Linutronix-Spam-Score: -1.0
X-Linutronix-Spam-Level: -
X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required,  ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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