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: <a713ba13-634d-3061-933c-25a8d62eb0be@intel.com>
Date:   Tue, 6 Dec 2022 10:56:51 -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 1/2] x86/resctrl: Update task closid/rmid with
 task_call_func()
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-2-peternewman@google.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20221129111055.953833-2-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?Y2RUN2VlUk9jSzlUZEZKdnEwOG1wNUJCcnlUdDlWc2dTVm9iZWxBSWxkQmtK?=
 =?utf-8?B?TjZkYnlJZGFSV1IwU2h5cm9mSm8yZWN4SnBIQVhYcWpPS0JZdnpvb3Z0RlN0?=
 =?utf-8?B?NFI0V2F1cXRCemptRXkwKzhCc2NBeWRVTlZvTzIwZ1gvNWhYRFpNYllXdTVN?=
 =?utf-8?B?ZlA2UGZDdXE5Zmw3NGlzTGNjR3BPS2NwK0RCTEJWejRFZGNGc0VXckVlZDdB?=
 =?utf-8?B?aFpQdEdFODVJTldQbmZKZzdsUkt3ak5qWXRLYzRzVE52L1d4bVEzQWhhWWRo?=
 =?utf-8?B?S1JyckY4YlFUNjVmcmllOUw2cG1aelpoRmZHS0gwZXpFS2dsRnVueVZEblBt?=
 =?utf-8?B?MWpmT3JNTjBUUS9XUm1zbW9uT0ZnZmxJN1ovSi8vVFlPdU8wM2I2aUdZeWJM?=
 =?utf-8?B?cWNrRnF2NmdMR0swRzVsNkU4UlB3NHNTTTU5eGVoYklhY3JaY2wxQkhYV1l4?=
 =?utf-8?B?SGFXaFNwclQxWENWbVBrb0ZIV2pjNkdMWGtlMDVueGpReTZFcXR1K2Y2cVJh?=
 =?utf-8?B?NWJPRmorWXR3Qnp2NGkzVkZxRVJ1TWd0Qm1LRkp3QkFCeEtUOEI1TEJGK3h5?=
 =?utf-8?B?dFNhQVI5dUF5aGVTcSt2eFVpL0lmdWpFR1Q1Yy9taVFVOU1HZmd1L3UvclMy?=
 =?utf-8?B?RDZEcGFzd29FSXRjd2E3dGp2L3hKbU1kSTh3NmYyWllhc2xERGZPWVZQMHd3?=
 =?utf-8?B?Mk83Y096VFdPVU5EcWovY2FBYlkzTDRRZHhrby9BM1NLbW9QbkExSmtSSzVS?=
 =?utf-8?B?UE45dTh4OTVoU1dKcmxFZndGT1QyZUNhQ2k4RGF6amVZZE90U3BXUkczUURS?=
 =?utf-8?B?dk5oUXNuU2NMd1htaHNIcXhZUFhoaUZha2hqUG0wNXVFcTlJRndWVHdsVDMv?=
 =?utf-8?B?L3YvN2RBQTlVSmo1bWExUTZsTlI2Zzc2ODQzQzdtcVZxQ2RzUmJleW1mc2Jj?=
 =?utf-8?B?WWZtTWhkNnhYRXRueTI3VU5PY2thY1F4TUhIOExOTW55TE9WZ2xBVFFZRGxL?=
 =?utf-8?B?UWpid1g5TDhKYUNtbXZGVVQybFlNd2dTVm1JZ0Vsam81UGxVT1YxUUsyMUdM?=
 =?utf-8?B?Y2xONGlsN2cwcXp2ZE4rVmVZZHJPNEtwVk1oTW4wMzBCb2RCZWVOSGtsK1hP?=
 =?utf-8?B?V1Ira2k5RXk5NkliaUN5RThMdkswQVVTM2oxOGg4aTFrc0VKcVJyWWxlaGFz?=
 =?utf-8?B?YzBkRnJ5bnBWYWt0b1dRakpwdUNSYVFPWDljRnZiSCtsSC90MENDbjliMmxB?=
 =?utf-8?B?anYwbThhRkxnQlFBL05RLzVpT1dYUGtJZkpWdkpBalhlWWh0QXIvUkR3UFBW?=
 =?utf-8?B?LzM3UjNhbkRkNG15K3VHTTZleXlMekExZnRRLzVIOWQ0bWhDTCt5RmllbWR1?=
 =?utf-8?B?NXE5aWlvV0YzZTdyUlRncEl4a09ZRFk0NjhqekVEMGxMQ1A4dnJmRVAyK0VY?=
 =?utf-8?B?MFkzUThUUk1JYjRWTzJKUDRRWnVvUHZqWVduamRGNXJCR0FqandLakllc1Fm?=
 =?utf-8?B?ZVlHK095SGdPQ3EveXlBOHRUUkNYSVpKbFBvUzEzUXZleUxPMzlKUytUaWha?=
 =?utf-8?B?UWNYMklSZnJCSktCZTlackN2QUJXVllxbVBQakdpeTJtQzBlYzE3WmdpVWMx?=
 =?utf-8?B?YUJnS2ludHN4cjQ2ODRQdmE5OWRjUVVYUHpoMC9yV1hSMFlmZHplc2xZd3RU?=
 =?utf-8?B?YUhGb0lOdTJvZ3RhQjUxTjdJcytmUkxwcm5QSjRvZHNXVnZYSmtWM1NlQkUw?=
 =?utf-8?B?cWZGUTgyeHJSR1h4UTM2b2NnTG9waHFmbG14aW5EU2Z6ZVlaYzBBTDFjUEpD?=
 =?utf-8?B?a1RNQ3pHckVYZEczRWVlaFF1bW5hNGZyWnlwV1JnNUlxV2VKb3dqWXIzaXBq?=
 =?utf-8?B?S05JTFR0V0VuVmR4aFJHOGVjU1N6VzFDQVVpZnM5dmJpd2tlWm5EenhyZUpI?=
 =?utf-8?B?azlVV3RENGVjR3ZVMHNTM28zazJ0U2JXeE5PbzYzUVRCMWEraE1QaVhqS0JG?=
 =?utf-8?B?eHZCNm91ZXNaNWorbFdMdVAvUHlMNVY4VzRSRUVsc2dtYUZMUkh3VHlJMDZY?=
 =?utf-8?B?bTBHeW9yUFR1RVVaNFVjVG9CcVJMQTkvM0xWOHp5VGNIVHRKbFBTYUc4QmZD?=
 =?utf-8?B?SEVKK1VNMHprUlpFSjFieUVwaU1tL0tTYk81MFp5UlBmcnJWaUNVQS8wVWtP?=
 =?utf-8?B?SXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bba6f79c-bbc9-4784-0639-08dad7bba426
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:56:54.3532
 (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: JAkNQrt7LC5f1xkLgowlm+RVCZYNK/SkjMeVVz6fJVBT7rOBrwDtcjQh/ue4Y6F+0stul/AATgf8mG/9QgE2BXqc87krPdksW97IVWpnZLs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR11MB5573
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.4 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_HI,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:
> When the user moves a running task to a new rdtgroup using the tasks
> file interface, the resulting change in CLOSID/RMID must be immediately
> propagated to the PQR_ASSOC MSR on the task's CPU.
> 
> It is possible for a task to wake up or migrate while it is being moved
> to a new group. If __rdtgroup_move_task() fails to observe that a task
> has begun running or misses that it migrated to a new CPU, the task will
> continue to use the old CLOSID or RMID until it switches in again.
> 
> __rdtgroup_move_task() assumes that if the task migrates off of its CPU
> before it can IPI the task, then the task has already observed the
> updated CLOSID/RMID. Because this is done locklessly and an x86 CPU can
> delay stores until after loads, the following incorrect scenarios are
> possible:
> 
>  1. __rdtgroup_move_task() stores the new closid and rmid in
>     the task structure after it loads task_curr() and task_cpu().

Stating how this scenario encounters the problem would help
so perhaps something like (please feel free to change):
"If the task starts running between a reordered task_curr() check and
the CLOSID/RMID update then it will start running with the old CLOSID/RMID
until it is switched again because __rdtgroup_move_task() failed to determine 
that it needs to be interrupted to obtain the new CLOSID/RMID."

>  2. resctrl_sched_in() loads t->{closid,rmid} before the calling context
>     switch stores new task_curr() and task_cpu() values.

This scenario is not clear to me. Could you please provide more detail about it?
I was trying to follow the context_switch() flow and resctrl_sched_in() is
one of the last things done (context_switch()->switch_to()->resctrl_sched_in()).
From what I can tell rq->curr, as used by task_curr() is set before
even context_switch() is called ... and since the next task is picked from
the CPU's runqueue (and set_task_cpu() sets the task's cpu when moved to
a runqueue) it seems to me that the value used by task_cpu() would also
be set early (before context_switch() is called). It is thus not clear to
me how the above reordering could occur so an example would help a lot.

> 
> Use task_call_func() in __rdtgroup_move_task() to serialize updates to
> the closid and rmid fields in the task_struct with context switch.

Is there a reason why there is a switch between the all caps CLOSID/RMID
at the beginning to the no caps here? 

> Signed-off-by: Peter Newman <peternewman@google.com>
> Reviewed-by: James Morse <james.morse@arm.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 78 ++++++++++++++++----------
>  1 file changed, 47 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index e5a48f05e787..59b7ffcd53bb 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -528,6 +528,31 @@ static void rdtgroup_remove(struct rdtgroup *rdtgrp)
>  	kfree(rdtgrp);
>  }
>  
> +static int update_locked_task_closid_rmid(struct task_struct *t, void *arg)
> +{
> +	struct rdtgroup *rdtgrp = arg;
> +
> +	/*
> +	 * Although task_call_func() serializes the writes below with the paired
> +	 * reads in resctrl_sched_in(), resctrl_sched_in() still needs
> +	 * READ_ONCE() due to rdt_move_group_tasks(), so use WRITE_ONCE() here
> +	 * to conform.
> +	 */
> +	if (rdtgrp->type == RDTCTRL_GROUP) {
> +		WRITE_ONCE(t->closid, rdtgrp->closid);
> +		WRITE_ONCE(t->rmid, rdtgrp->mon.rmid);
> +	} else if (rdtgrp->type == RDTMON_GROUP) {
> +		WRITE_ONCE(t->rmid, rdtgrp->mon.rmid);
> +	}
> +
> +	/*
> +	 * If the task is current on a CPU, the PQR_ASSOC MSR needs to be
> +	 * updated to make the resource group go into effect. If the task is not
> +	 * current, the MSR will be updated when the task is scheduled in.
> +	 */
> +	return task_curr(t);
> +}
> +
>  static void _update_task_closid_rmid(void *task)
>  {
>  	/*
> @@ -538,10 +563,24 @@ static void _update_task_closid_rmid(void *task)
>  		resctrl_sched_in();
>  }
>  
> -static void update_task_closid_rmid(struct task_struct *t)
> +static void update_task_closid_rmid(struct task_struct *t,
> +				    struct rdtgroup *rdtgrp)
>  {
> -	if (IS_ENABLED(CONFIG_SMP) && task_curr(t))
> -		smp_call_function_single(task_cpu(t), _update_task_closid_rmid, t, 1);
> +	/*
> +	 * Serialize the closid and rmid update with context switch. If
> +	 * task_call_func() indicates that the task was running during
> +	 * update_locked_task_closid_rmid(), then interrupt it.
> +	 */
> +	if (task_call_func(t, update_locked_task_closid_rmid, rdtgrp) &&
> +	    IS_ENABLED(CONFIG_SMP))
> +		/*
> +		 * If the task has migrated away from the CPU indicated by
> +		 * task_cpu() below, then it has already switched in on the
> +		 * new CPU using the updated closid and rmid and the call below
> +		 * is unnecessary, but harmless.
> +		 */
> +		smp_call_function_single(task_cpu(t),
> +					 _update_task_closid_rmid, t, 1);
>  	else
>  		_update_task_closid_rmid(t);
>  }
> @@ -557,39 +596,16 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
>  		return 0;
>  
>  	/*
> -	 * Set the task's closid/rmid before the PQR_ASSOC MSR can be
> -	 * updated by them.
> -	 *
> -	 * For ctrl_mon groups, move both closid and rmid.
>  	 * For monitor groups, can move the tasks only from
>  	 * their parent CTRL group.
>  	 */
> -
> -	if (rdtgrp->type == RDTCTRL_GROUP) {
> -		WRITE_ONCE(tsk->closid, rdtgrp->closid);
> -		WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
> -	} else if (rdtgrp->type == RDTMON_GROUP) {
> -		if (rdtgrp->mon.parent->closid == tsk->closid) {
> -			WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
> -		} else {
> -			rdt_last_cmd_puts("Can't move task to different control group\n");
> -			return -EINVAL;
> -		}
> +	if (rdtgrp->type == RDTMON_GROUP &&
> +	    rdtgrp->mon.parent->closid != tsk->closid) {
> +		rdt_last_cmd_puts("Can't move task to different control group\n");
> +		return -EINVAL;
>  	}
>  
> -	/*
> -	 * Ensure the task's closid and rmid are written before determining if
> -	 * the task is current that will decide if it will be interrupted.
> -	 */
> -	barrier();
> -
> -	/*
> -	 * By now, the task's closid and rmid are set. If the task is current
> -	 * on a CPU, the PQR_ASSOC MSR needs to be updated to make the resource
> -	 * group go into effect. If the task is not current, the MSR will be
> -	 * updated when the task is scheduled in.
> -	 */
> -	update_task_closid_rmid(tsk);
> +	update_task_closid_rmid(tsk, rdtgrp);
>  
>  	return 0;
>  }

The change looks good to me.

Reinette