Messages in this thread | | | Date | Thu, 27 Apr 2023 15:20:15 +0100 | Subject | Re: [PATCH v3 18/19] x86/resctrl: Add cpu offline callback for resctrl work | From | James Morse <> |
| |
Hi Ilpo,
On 21/03/2023 15:32, Ilpo Järvinen wrote: > On Mon, 20 Mar 2023, James Morse wrote: > >> The resctrl architecture specific code may need to free a domain when >> a CPU goes offline, it also needs to reset the CPUs PQR_ASSOC register. >> The resctrl filesystem code needs to move the overflow and limbo work >> to run on a different CPU, and clear this CPU from the cpu_mask of >> control and monitor groups. >> >> Currently this is all done in core.c and called from >> resctrl_offline_cpu(), making the split between architecture and >> filesystem code unclear. >> >> Move the filesystem work into a filesystem helper called >> resctrl_offline_cpu(), and rename the one in core.c >> resctrl_arch_offline_cpu(). >> >> The rdtgroup_mutex is unlocked and locked again in the call in >> preparation for changing the locking rules for the architecture >> code. >> >> resctrl_offline_cpu() is called before any of the resource/domains >> are updated, and makes use of the exclude_cpu feature that was >> previously added.
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c >> index aafe4b74587c..4e5fc89dab6d 100644 >> --- a/arch/x86/kernel/cpu/resctrl/core.c >> +++ b/arch/x86/kernel/cpu/resctrl/core.c >> @@ -578,22 +578,6 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r) >> >> return; >> } >> - >> - if (r == &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl) { >> - if (is_mbm_enabled() && cpu == d->mbm_work_cpu) { >> - cancel_delayed_work(&d->mbm_over); >> - /* >> - * exclude_cpu=-1 as this CPU has already been removed >> - * by cpumask_clear_cpu()d >> - */ > > This was added in 17/19 and now removed (not moved) in 18/19. Please avoid > such back-and-forth churn.
This is the cost of making small incremental changes that should be easier to review. The intermediate step was a little odd, so came with a comment. (I normally mark those as 'temporary', but didn't bother this time as they are adjacent patches)
If you'd prefer, I can merge these patches together... but from Reinette's feedback its likely I'll split them up even more.
Thanks,
James
| |