Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4085f4f3-7a3e-8af8-1ae3-1040ca78f59f@intel.com>
Date:   Wed, 9 Aug 2023 15:41:01 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.14.0
Subject: Re: [PATCH v5 24/24] x86/resctrl: Separate arch and fs resctrl locks
Content-Language: en-US
To:     James Morse <james.morse@arm.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        H Peter Anvin <hpa@zytor.com>,
        Babu Moger <Babu.Moger@amd.com>,
        <shameerali.kolothum.thodi@huawei.com>,
        D Scott Phillips OS <scott@os.amperecomputing.com>,
        <carl@os.amperecomputing.com>, <lcherian@marvell.com>,
        <bobo.shaobowang@huawei.com>, <tan.shaopeng@fujitsu.com>,
        <xingxin.hx@openanolis.org>, <baolin.wang@linux.alibaba.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Xin Hao <xhao@linux.alibaba.com>, <peternewman@google.com>,
        <dfustini@baylibre.com>
References: <20230728164254.27562-1-james.morse@arm.com>
 <20230728164254.27562-25-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230728164254.27562-25-james.morse@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UnU3R1ZvK1oycGZxQmFHc2xQZXRjUCtTbjVBN003UUtQVllQQUZVNTZYZU1C?=
 =?utf-8?B?S2lIWWVUMEE3R0tod0Rtd1RoMExQbHNlZFpHT0NVaUVLWXM2eGNMZTZFTUl6?=
 =?utf-8?B?bERwQ01pOStBbFE1WE5OVDgrcDZ1cXVjUTQ2RDBNdEtGQ0NOQ1g5L0VxVWk1?=
 =?utf-8?B?MUF6RTRBSVNQUjlmYjNlSWhDVFRSSVFMMWduT0cxUUhHZTNtNlQ4U1dXME12?=
 =?utf-8?B?SXdhQ3AwTU9QWEJjTDJHenN3WTM3SWJCaWJkSHUvcEdDenJmVDNMb25DR05x?=
 =?utf-8?B?aWxJZytGOCtqQngzT0RBcjVUbGk4T2RhdHJWMmFNTUNLOXd1MFNLNDdJUmFK?=
 =?utf-8?B?clh3M1RMUWxrVUFWRkJPYWZkdG1OVWV0L2tUUXJvZk53YWVQamFLdGhDRkRU?=
 =?utf-8?B?TDRPWjN4eGcrZjNKdkYzdHlYRjNDTUNJbXhhNmh3ZWJmZTlFMFVMbVhNUGdQ?=
 =?utf-8?B?cmxPZFA4VS96K2ZPaXdLV0x5T1dKRFZCdEhrWHd6SnpnaDJsT0wyKzFLMzFz?=
 =?utf-8?B?YVYwdGxxbnRZejgwSkMzNXRiZnZ0OUFTVUhaNGE5b3VqZGlCdGtPSWFwM1JY?=
 =?utf-8?B?VXdhOVFSQzVBWnpDSWw1azFzbE84cXdycjRjd0RncjZNMHdoaG1NSENiM1pS?=
 =?utf-8?B?VWRSV2RCbiszbHZsZTdsc3FCclhCNnd1WlVrTjVSejNnSmFiV0dYU084M1R0?=
 =?utf-8?B?WGdIL2lIeUUwZHY1TjgrS2Q1NnBEalJoekUxcm0xQlU3ZzQ5dGg0bzQ4ZTEx?=
 =?utf-8?B?aEVkODF6WURKYmlvbDlDemhKUFVEMmFMNXhoczM3ejhOQnJ2MkE0ZTFRREd1?=
 =?utf-8?B?R0EyQTArc1daa29hZnM0aC9Obkw5bUsrOThsRjJENlc1bWtHWG02OE1JbWtL?=
 =?utf-8?B?bUFaSUdXbVNmRnhPT0o3NGZWamdDNzhHT2h2dTlrOWwzcDJsQkxQZmJBTkt0?=
 =?utf-8?B?d01wRHJER1VzWlN2YW9PYTRKbmRnRTJCVXJEQVZiWjBBSTRuenJKWDhIUmJo?=
 =?utf-8?B?c3FNdUgrRjdtTkg5QWJ5UlhmUENSOWxHOXU1dWcvcjNKYXpJZVNVU1RiV0Rz?=
 =?utf-8?B?cXgvWGcyTjVRWUROV1orY1ZxQmQ3R25DNXY3SEkyMkFnRVcyY0cxOWhFaGsy?=
 =?utf-8?B?VmY5aTBFYjlWSjNid25LR1hrTUtZZ0xycHhpN2h4TmVNaGVYSUd2K1dCc0lN?=
 =?utf-8?B?WmgxS2c1SGdyYmJBN1Y4QllSdGFzM2hBcHdsL1RYeXdoUytUTllkMVNkZ2J2?=
 =?utf-8?B?czZaQjRkNzQzbFBQTFM5WHArelNGVDVZOHpKMlVZOTVBN2FrWmozbC8zb0F5?=
 =?utf-8?B?ZHFUUXVFOVh2N2c4U2piL3hJVnhxbm80QzV1TnpFWkdHNmZIMXF1Wm10cmRP?=
 =?utf-8?B?SUg5UUpwb3VYWjAxZWZGNThWeVYxZ0tNemlMZDJ4K3JQUUJqTjhtVUVrUVI3?=
 =?utf-8?B?cTJ6ZmdBQ0RkRWdZWUliNnJHOEtydFBuaUpaNDRYdy95RUcyaVk4UGFrK242?=
 =?utf-8?B?Nk1IOHd1NktOcnR6RDdhcUZLNGRuZ2pwTUo0cW81d2p1ZlR5S0JYOEw2akRu?=
 =?utf-8?B?bkJvaUkvcHh3UWRHcEZia3EvdmxheHpRTjczYTdkZVJSSzVIcHJTOGdPRTFm?=
 =?utf-8?B?TTloTUdVai9jWDdUU0lVZEZYTFp3QUNrSkw0eW14Unl4UXJnbDZwbkJ2MjZj?=
 =?utf-8?B?ZUVnTDAwRnc1OHpQSHIreWpTZ0NTSFlsWWY4UTRRdS9tV0ZVb2hwSnAwSmdT?=
 =?utf-8?B?dEQvL0N6NlIwNlBJQ1RML1M5R3BGMGFZUTdvdmVUeUFRb2hNaE14c3BRUE1K?=
 =?utf-8?B?c3p2cHdMdTNOWnR0bDl3ak9xN3pCeEpuOUZOWTVzQloyUkdJazRpdi9KQ2o1?=
 =?utf-8?B?UFByTXc1MEFjRE9mTW5VcTk3dFRZWGo2V3JpY3RhdUdKdlVKTjJISWF2WmdF?=
 =?utf-8?B?eCtTQ1V1N2RWdG5ya2JvcUFpRENSVmNMS3I1bVZWU2lpbDZPWVF1RzFUZ3la?=
 =?utf-8?B?Q09jeVJ3K05zcVBjYTRUQmRMVlZGK0RlaGVqQy9saE1EcUtGbjF0Q3k1WVE0?=
 =?utf-8?B?blRna1lEWDNTNWgzclJKS1NSay9IN3Bqa3JJcDdCb3V0enBEM3NIM3FRQlRM?=
 =?utf-8?B?Vm5tOG1vU3o0QlQ5NEhpUmxabU4rS3RFWU9EcExzaU4yY1hWMFpqSXorY0hv?=
 =?utf-8?B?K2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a6e47375-2fcc-4880-24af-08db9929b76f
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Aug 2023 22:41:05.5022
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: UMnz8pDrH0B1U2OyZkXbc20ALuFL/w/kkyMDUUxMU2dSNe+fv23seIc+M5TEaRbwLEKqC2MLngYHbiGQaoLCC94XJhyojrfh13Ng3p1n88Q=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7549
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi James,

On 7/28/2023 9:42 AM, James Morse wrote:
> resctrl has one mutex that is taken by the architecture specific code,
> and the filesystem parts. The two interact via cpuhp, where the
> architecture code updates the domain list. Filesystem handlers that
> walk the domains list should not run concurrently with the cpuhp
> callback modifying the list.
> 
> Exposing a lock from the filesystem code means the interface is not
> cleanly defined, and creates the possibility of cross-architecture
> lock ordering headaches. The interaction only exists so that certain
> filesystem paths are serialised against cpu hotplug. The cpu hotplug

cpu hotplug -> CPU hotplug

> code already has a mechanism to do this using cpus_read_lock().
> 
> MPAM's monitors have an overflow interrupt, so it needs to be possible
> to walk the domains list in irq context. RCU is ideal for this,
> but some paths need to be able to sleep to allocate memory.
> 
> Because resctrl_{on,off}line_cpu() take the rdtgroup_mutex as part
> of a cpuhp callback, cpus_read_lock() must always be taken first.
> rdtgroup_schemata_write() already does this.
> 
> Most of the filesystem code's domain list walkers are currently
> protected by the rdtgroup_mutex taken in rdtgroup_kn_lock_live().
> The exceptions are rdt_bit_usage_show() and the mon_config helpers
> which take the lock directly.
> 
> Make the domain list protected by RCU. An architecture-specific
> lock prevents concurrent writers. rdt_bit_usage_show() can
> walk the domain list under rcu_read_lock(). The mon_config helpers
> send multiple IPIs, take the cpus_read_lock() in these cases.
> 
> The other filesystem list walkers need to be able to sleep.
> Add cpus_read_lock() to rdtgroup_kn_lock_live() so that the
> cpuhp callbacks can't be invoked when file system operations are
> occurring.
> 
> Add lockdep_assert_cpus_held() in the cases where the
> rdtgroup_kn_lock_live() call isn't obvious.
> 
> Resctrl's domain online/offline calls now need to take the
> rdtgroup_mutex themselves.
> 
> Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
> Signed-off-by: James Morse <james.morse@arm.com>

...

> @@ -464,6 +467,9 @@ static void show_doms(struct seq_file *s, struct resctrl_schema *schema, int clo
>  	bool sep = false;
>  	u32 ctrl_val;
>  
> +	/* Walking r->domains, ensure it can't race with cpuhp */
> +	lockdep_assert_cpus_held();
> +
>  	seq_printf(s, "%*s:", max_name_width, schema->name);
>  	list_for_each_entry(dom, &r->domains, list) {
>  		if (sep)
> @@ -534,8 +540,8 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>  {
>  	int cpu;
>  
> -	/* When picking a CPU from cpu_mask, ensure it can't race with cpuhp */
> -	lockdep_assert_held(&rdtgroup_mutex);
> +	/* When picking a cpu from cpu_mask, ensure it can't race with cpuhp */

cpu -> CPU

> +	lockdep_assert_cpus_held();
>  
>  	/*
>  	 * Setup the parameters to pass to mon_event_count() to read the data.

...

> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index a256a96df487..47dcf2cb76ca 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -35,6 +35,10 @@
>  DEFINE_STATIC_KEY_FALSE(rdt_enable_key);
>  DEFINE_STATIC_KEY_FALSE(rdt_mon_enable_key);
>  DEFINE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
> +
> +/* Mutex to protect rdtgroup access. */
> +DEFINE_MUTEX(rdtgroup_mutex);
> +
>  static struct kernfs_root *rdt_root;
>  struct rdtgroup rdtgroup_default;
>  LIST_HEAD(rdt_all_groups);
> @@ -954,7 +958,8 @@ static int rdt_bit_usage_show(struct kernfs_open_file *of,
>  
>  	mutex_lock(&rdtgroup_mutex);
>  	hw_shareable = r->cache.shareable_bits;
> -	list_for_each_entry(dom, &r->domains, list) {
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(dom, &r->domains, list) {
>  		if (sep)
>  			seq_putc(seq, ';');
>  		sw_shareable = 0;

Does rdt_bit_usage_show() really need RCU? It is another filesystem callback and I
do not see a reason why it should access the domain list in a different way. It
can follow the same pattern as all the other resctrl filesystem ops and use
cpus_read_lock().

> @@ -1010,8 +1015,10 @@ static int rdt_bit_usage_show(struct kernfs_open_file *of,
>  		}
>  		sep = true;
>  	}
> +	rcu_read_unlock();
>  	seq_putc(seq, '\n');
>  	mutex_unlock(&rdtgroup_mutex);
> +

Unnecessary empty line.


>  	return 0;
>  }


Reinette