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
