Messages in this thread | | | Date | Thu, 27 Apr 2023 17:51:46 +0300 (EEST) | From | Ilpo Järvinen <> | Subject | Re: [PATCH v3 18/19] x86/resctrl: Add cpu offline callback for resctrl work |
| |
On Thu, 27 Apr 2023, James Morse wrote:
> 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)
Why not mention the oddity at the end of changelog then? That keeps the diffs clean of temporary comments.
> If you'd prefer, I can merge these patches together... but from > Reinette's feedback its likely I'll split them up even more.
I don't prefer merging.
-- i.
| |