Re: [PATCH 2/3] x86/resctrl: Update PQR_ASSOC MSR synchronously when moving task to resource group

[Reinette Chatre]

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@xxxxxxxxxx>
> > Reported-by: Valentin Schneider <valentin.schneider@xxxxxxx>
> > Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> > Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
>
> Some pedantic comments below; with James' task_curr() + task_cpu()
> suggestion:
>
> Reviewed-by: Valentin Schneider <valentin.schneider@xxxxxxx>

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