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: <60e7ba72-edff-9bf7-0541-403f25b5bc51@intel.com>
Date:   Tue, 6 Dec 2022 10:57:05 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.5.1
Subject: Re: [PATCH v4 2/2] x86/resctrl: IPI all online CPUs for group updates
Content-Language: en-US
To:     Peter Newman <peternewman@google.com>, <fenghua.yu@intel.com>
CC:     <bp@alien8.de>, <derkling@google.com>, <eranian@google.com>,
        <hpa@zytor.com>, <james.morse@arm.com>, <jannh@google.com>,
        <kpsingh@google.com>, <linux-kernel@vger.kernel.org>,
        <mingo@redhat.com>, <tglx@linutronix.de>, <x86@kernel.org>
References: <20221129111055.953833-1-peternewman@google.com>
 <20221129111055.953833-3-peternewman@google.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20221129111055.953833-3-peternewman@google.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?RWhPNWZxK0w4RW45L3ZTTVZUTVJlS3VIZ0NDZmltTzBlUE9yNE85MFJ6YWxO?=
 =?utf-8?B?bHdGVHpDWmZBMEFIRmd2NTZKUWNraXg4blhXSEk3Wm1xdHV5d0J1bUlvcXcy?=
 =?utf-8?B?RjBiODZNSmsyZi9MbmZobTBia2ZmMXo0MldCeXIxblVUcktvNjB6VDRTWFla?=
 =?utf-8?B?dVdKWXlDNWxNeCt2b3d2OGdBTjZDVU5QcTZOTGZlUVYzNkpsSzVkeHR5eGlG?=
 =?utf-8?B?eE4rSVN1bGRSejFadENDdXJjKytza3hKM1pSOVdXMGNpSzJoSXZNVkVZdjMz?=
 =?utf-8?B?RklMb3VFeHYyUG9Rb2lFWUt3TGFEVmlhTDRuWnhoU0JFaDc0UjlwV2N1WGUv?=
 =?utf-8?B?WXkyMFB0b25JenFkZU1DTW5MNEZSVzBUUGtZZ0dGSm8rVlF6M01GVVUvd2hT?=
 =?utf-8?B?MS9SbEdLWXlSaXB3WFpGM05sK3ZpeDkvT2xsUnE2UWliRTBwN0NkbHFRRFJs?=
 =?utf-8?B?L2ZjUlJ2MEE0enNPSGFxUFlMYkYwSENDTXlCeU93cjNQOHRySjRsSkdKTHBD?=
 =?utf-8?B?VmZWMWtXKzFFdThna01hZGt5NWp0aU9tU0NRY216elpwelh0ekl1QTVLYVBW?=
 =?utf-8?B?SktpU2VGeXNYZ1NjT0Uvb3BBeTgzNmN6Lyt4cU4xTC9Nam8zR2tJaDlGVnNv?=
 =?utf-8?B?eDZsYXBaaGQrSmJ3a29lRnBCaDA2Q1FDYWNaT2lBTTkvdExqR3ZYNDRTRFBQ?=
 =?utf-8?B?VFpjdzJldkZsWWJpclRBWVBrRlZMZmRublVYekdScGpRRjV3NmRkVnRaU0FU?=
 =?utf-8?B?VjhuSUlGTHJpVlJkMGtnajJaemkrdjhVandUcUNvaWxVandSYUs5b3BHR05k?=
 =?utf-8?B?c3Nyb1RJWE1nTUw4akpBTXlEaXpneHY1NW11ejFXZ3pMVUpOeE1PK1BNeU5r?=
 =?utf-8?B?eFVHUExJWHVQcDl3bzIvejdHN21RTDE0eXdhVnJRdHpsZjU2UUtqczIzclFF?=
 =?utf-8?B?TlY5NWRTTWF1a1h0T2oxT1hYaTFvUmUxUEFrb2FkaGlnOG9QR3BnY1JsRGFi?=
 =?utf-8?B?U3RpdElkUm5KTzRCaEJCSlpNWDZMQ21KMmE3S20rRmhuM1JXZFdrSzlwMGVv?=
 =?utf-8?B?dC9OaDVJOFhDbkgwOTMrSVJTcEk4d2tQWFlMOTNweFFKSHNtWm9MVmZpeGYw?=
 =?utf-8?B?bnAxd2RoL1hYWGovUU5KaGw4K3NvaFFtMHg5YVFQN1I5YXlVQm1paVRUT3Rh?=
 =?utf-8?B?cmFLUkVEaFVsRXoycWtQL2FQcU1XMzJ6V1ZHSTlaa09naXBUWUNRR0VjTnNR?=
 =?utf-8?B?V3VPa1pTM05nak9yUkcwRzE1bGJUZ04yc3JMelRhQjRLV1M4aTBOUUZXS0ht?=
 =?utf-8?B?RUM0RkU3RzBvajlteXZZSWx1Q201by9PcmZWQml6Qmg3M1VTL0cveCsrb2lO?=
 =?utf-8?B?TmhDQWVYem9DTFlncUdrL29RWUwxc3NUYnIwVjQ3bHhybGYxMXhhVXZwYTUy?=
 =?utf-8?B?b2pZcytYQmpZV1NtdExsRjhCZ0JrSEIxRmU0cElDdkg2d3J3OStQdFVZY0h4?=
 =?utf-8?B?SmxCZzdkODFNZjdsZDBBY0lZMXBFUjA1Rys3U1dlUVloNlUrZHZla1I5MnFr?=
 =?utf-8?B?emNsZmFlK0o4Znd0MENBckhWSlVadG0xRC9qb3Fld2VCR0llNUtYdUJoMTJi?=
 =?utf-8?B?M0NGcVdjRzFRYzJMWCtSWjM3VGFteEN1ZFBObHpnYlorUW9aMWhkeFdRYXJz?=
 =?utf-8?B?cExDOExEdUl3OXFOUmZsRGRZSkNYQTRSZS9NWlNJTGZFOVc2T0hxTEpMTGlo?=
 =?utf-8?B?Q0dVaVQ2QzlMSHUzRHJqWkRJckRDK0wvVVZnZXBiRm9wMDVYenlldUJJWHl5?=
 =?utf-8?B?UDVqMDk4eFFVRkNEQ29ONGgvZmNNVzVtczIwU0RyR3lGNkNyQURHeFpycURM?=
 =?utf-8?B?NHBSZFdDNEg3L2NWRWQ2Q0hCc0t5K0tuQ0FWWUhpbzVJY21uOFpXUVNTbS83?=
 =?utf-8?B?eldwcDIxS2IveU1uR0hJQjB5Mi9KdGVKRUtBOEZjeWlQR1N6ODhQL2JoeHg0?=
 =?utf-8?B?VDM0Kzh6OWI0SGhHYzFLVTdoSFR3ZXgyTDZ4SUdZVE40eEpjSUw4UEVTV01B?=
 =?utf-8?B?aXBjLzVHaFVRZzBqclZzeWdvNStQazlwUDlGaDgzM3E1WGFXMElCMkJENndq?=
 =?utf-8?B?Zm5IMmgwS2dvcytid0NlV3FPTG43V1p0emJWY2VoSHdjZXhLVk5hdzZZMXEz?=
 =?utf-8?B?NFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 28e853a2-d5b8-4af2-5e28-08dad7bbacab
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2022 18:57:08.3520
 (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: tgpPqjXYnwcrAf1fcBWCMDogUkMPyJFOXHTscmDk7I9uOlyjcxDom3NOP+cYCFSGiDJejV69a0HgAXvhsPe+nVwoyFF7RkSkcTp4vi0nvfg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR11MB5573
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE 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 Peter,

On 11/29/2022 3:10 AM, Peter Newman wrote:
> Removing a CTRL_MON or MON group directory moves all tasks to the parent
> group. The rmdir implementation therefore interrupts any running
> tasks which were in the deleted group to update their CLOSID/RMID to
> those of the parent.
> 
> The rmdir operation iterates over all tasks in the deleted group while
> read-locking the tasklist_lock to ensure that no newly-created child
> tasks remain in the deleted group.

The above describes the current behavior. This is great context. What
follows in the changelog is a description of different fixes. This is
unexpected because there is no description of a problem with the current
behavior.

Could you please describe the problem with the current implementation? Next
you could state the two possible solutions and then I think the reader would
be ready to parse what is written below.


> Calling task_call_func() to perform
> the updates on every task in the deleted group, similar to the recent
> fix in __rdtgroup_move_task(), would result in a much longer
> tasklist_lock critical section.


I so still think it would help to state that this additional locking
does not help to provide precise CPU mask. Especially since
the next paragraph may be interpreted that a precise CPU mask
is lost by giving up the additional locking.

> To avoid this, stop attempting to construct a precise mask of CPUs
> hosting the moved tasks in rdt_move_group_tasks(). Its callers instead
> perform the PQR_ASSOC MSR update on all online CPUs to ensure all
> affected tasks are notified.
> 
> To measure the impact of the rdt_move_group_tasks() implementation
> options, the following command was run in an rdtgroup to produce a
> 1600-task workload:
> 
>  # mkdir /sys/fs/resctrl/test
>  # echo $$ > /sys/fs/resctrl/test/tasks
>  # perf bench sched messaging -g 40 -l 100000
> 
> Results collected using:
> 
>  # perf stat rmdir /sys/fs/resctrl/test
> 
> CPU: Intel(R) Xeon(R) Platinum P-8136 CPU @ 2.00GHz (112 threads)
> 
> Calling task_call_func() on all tasks in the deleted group increased
> task-clock time from 1.54 to 2.35 ms, while the IPI broadcast reduced
> the time to 1.31 ms.

Thank you very much for doing this testing.

> 
> Restructuring resctrl groups is assumed to be a rare act of system-level
> reconfiguration by the user, so the impact of additional IPIs resulting
> from this change to a CPU-isolated workload is not a concern.
> 
> Signed-off-by: Peter Newman <peternewman@google.com>
> Reviewed-by: James Morse <james.morse@arm.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 52 +++++++-------------------
>  1 file changed, 13 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 59b7ffcd53bb..4a3c0b315484 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2401,12 +2401,10 @@ static int reset_all_ctrls(struct rdt_resource *r)
>   * Move tasks from one to the other group. If @from is NULL, then all tasks
>   * in the systems are moved unconditionally (used for teardown).
>   *
> - * If @mask is not NULL the cpus on which moved tasks are running are set
> - * in that mask so the update smp function call is restricted to affected
> - * cpus.
> + * Following this operation, the caller should update PQR_ASSOC MSR and per-CPU
> + * storage on all online CPUs.
>   */
> -static void rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to,
> -				 struct cpumask *mask)
> +static void rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to)
>  {
>  	struct task_struct *p, *t;
>  
> @@ -2416,16 +2414,6 @@ static void rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to,
>  		    is_rmid_match(t, from)) {
>  			WRITE_ONCE(t->closid, to->closid);
>  			WRITE_ONCE(t->rmid, to->mon.rmid);
> -
> -			/*
> -			 * If the task is on a CPU, set the CPU in the mask.
> -			 * The detection is inaccurate as tasks might move or
> -			 * schedule before the smp function call takes place.
> -			 * In such a case the function call is pointless, but
> -			 * there is no other side effect.
> -			 */
> -			if (IS_ENABLED(CONFIG_SMP) && mask && task_curr(t))
> -				cpumask_set_cpu(task_cpu(t), mask);
>  		}
>  	}
>  	read_unlock(&tasklist_lock);
> @@ -2456,7 +2444,7 @@ static void rmdir_all_sub(void)
>  	struct rdtgroup *rdtgrp, *tmp;
>  
>  	/* Move all tasks to the default resource group */
> -	rdt_move_group_tasks(NULL, &rdtgroup_default, NULL);
> +	rdt_move_group_tasks(NULL, &rdtgroup_default);
>  
>  	list_for_each_entry_safe(rdtgrp, tmp, &rdt_all_groups, rdtgroup_list) {
>  		/* Free any child rmids */
> @@ -3115,23 +3103,19 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
>  	return -EPERM;
>  }
>  
> -static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
> +static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp)
>  {
>  	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
>  	int cpu;
>  
>  	/* Give any tasks back to the parent group */
> -	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
> +	rdt_move_group_tasks(rdtgrp, prdtgrp);
>  
>  	/* Update per cpu rmid of the moved CPUs first */
>  	for_each_cpu(cpu, &rdtgrp->cpu_mask)
>  		per_cpu(pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;
> -	/*
> -	 * Update the MSR on moved CPUs and CPUs which have moved
> -	 * task running on them.
> -	 */
> -	cpumask_or(tmpmask, tmpmask, &rdtgrp->cpu_mask);
> -	update_closid_rmid(tmpmask, NULL);
> +
> +	update_closid_rmid(cpu_online_mask, NULL);
>  
>  	rdtgrp->flags = RDT_DELETED;
>  	free_rmid(rdtgrp->mon.rmid);
> @@ -3156,12 +3140,12 @@ static int rdtgroup_ctrl_remove(struct rdtgroup *rdtgrp)
>  	return 0;
>  }
>  
> -static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
> +static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp)
>  {
>  	int cpu;
>  
>  	/* Give any tasks back to the default group */
> -	rdt_move_group_tasks(rdtgrp, &rdtgroup_default, tmpmask);
> +	rdt_move_group_tasks(rdtgrp, &rdtgroup_default);
>  
>  	/* Give any CPUs back to the default group */
>  	cpumask_or(&rdtgroup_default.cpu_mask,
> @@ -3173,12 +3157,7 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  		per_cpu(pqr_state.default_rmid, cpu) = rdtgroup_default.mon.rmid;
>  	}
>  
> -	/*
> -	 * Update the MSR on moved CPUs and CPUs which have moved
> -	 * task running on them.
> -	 */
> -	cpumask_or(tmpmask, tmpmask, &rdtgrp->cpu_mask);
> -	update_closid_rmid(tmpmask, NULL);
> +	update_closid_rmid(cpu_online_mask, NULL);
>  
>  	closid_free(rdtgrp->closid);
>  	free_rmid(rdtgrp->mon.rmid);
> @@ -3197,12 +3176,8 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
>  {
>  	struct kernfs_node *parent_kn = kn->parent;
>  	struct rdtgroup *rdtgrp;
> -	cpumask_var_t tmpmask;
>  	int ret = 0;
>  
> -	if (!zalloc_cpumask_var(&tmpmask, GFP_KERNEL))
> -		return -ENOMEM;
> -
>  	rdtgrp = rdtgroup_kn_lock_live(kn);
>  	if (!rdtgrp) {
>  		ret = -EPERM;
> @@ -3222,18 +3197,17 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
>  		    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
>  			ret = rdtgroup_ctrl_remove(rdtgrp);
>  		} else {
> -			ret = rdtgroup_rmdir_ctrl(rdtgrp, tmpmask);
> +			ret = rdtgroup_rmdir_ctrl(rdtgrp);
>  		}
>  	} else if (rdtgrp->type == RDTMON_GROUP &&
>  		 is_mon_groups(parent_kn, kn->name)) {
> -		ret = rdtgroup_rmdir_mon(rdtgrp, tmpmask);
> +		ret = rdtgroup_rmdir_mon(rdtgrp);
>  	} else {
>  		ret = -EPERM;
>  	}
>  
>  out:
>  	rdtgroup_kn_unlock(kn);
> -	free_cpumask_var(tmpmask);
>  	return ret;
>  }
>  

The change looks good to me.

Reinette