lkml.org 
[lkml]   [2020]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/3] x86/resctrl: Update PQR_ASSOC MSR synchronously when moving task to resource group
From
Date
Hi Valentin,

On 12/11/2020 12:46 PM, Valentin Schneider wrote:
>
> On 03/12/20 23:25, Reinette Chatre wrote:
>> Fixes: e02737d5b826 ("x86/intel_rdt: Add tasks files")
>> Reported-by: Shakeel Butt <shakeelb@google.com>
>> Reported-by: Valentin Schneider <valentin.schneider@arm.com>
>> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> Cc: stable@vger.kernel.org
>
> Some pedantic comments below; with James' task_curr() + task_cpu()
> suggestion:
>
> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>

Thank you very much.


>> ---
...

>> + if (rdtgrp->type == RDTCTRL_GROUP) {
>> + tsk->closid = rdtgrp->closid;
>> + tsk->rmid = rdtgrp->mon.rmid;
>> + } else if (rdtgrp->type == RDTMON_GROUP) {
>> + if (rdtgrp->mon.parent->closid == tsk->closid) {
>> tsk->rmid = rdtgrp->mon.rmid;
>> - } else if (rdtgrp->type == RDTMON_GROUP) {
>> - if (rdtgrp->mon.parent->closid == tsk->closid) {
>> - tsk->rmid = rdtgrp->mon.rmid;
>> - } else {
>> - rdt_last_cmd_puts("Can't move task to different control group\n");
>> - ret = -EINVAL;
>> - }
>> + } else {
>> + rdt_last_cmd_puts("Can't move task to different control group\n");
>> + return -EINVAL;
>> }
>> + } else {
>> + rdt_last_cmd_puts("Invalid resource group type\n");
>> + return -EINVAL;
>> }
>
> James already pointed out this should be a WARN_ON_ONCE(), but is that the
> right place to assert rdtgrp->type validity?
>
> I see only a single assignment to rdtgrp->type in mkdir_rdt_prepare();
> could we fail the group creation there instead if the passed rtype is
> invalid?

Yes, there is that single assignment in mkdir_rdt_prepare() and looking
at how mkdir_rdt_prepare() is called it is only ever called with
RDTMON_GROUP or RDTCTRL_GROUP. This additional error checking was added
as part of this fix but unrelated to the fix itself. Since you and James
both pointed out flaws with it and it is unnecessary I will remove it
here and maintain the original checking that continues to be sufficient.

>> - return ret;
>> +
>> + /*
>> + * 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);
>
> We need the above writes to be compile-ordered before the IPI is sent.
> There *is* a preempt_disable() down in smp_call_function_single() that
> gives us the required barrier(), can we deem that sufficient or would we
> want one before update_task_closid_rmid() for the sake of clarity?
>

Apologies, it is not clear to me why the preempt_disable() would be
insufficient. If it is not then there may be a few other areas (where
resctrl calls smp_call_function_xxx()) that needs to be re-evaluated.

Thank you very much

Reinette

\
 
 \ /
  Last update: 2020-12-14 19:45    [W:0.087 / U:0.772 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site