Messages in this thread | | | Date | Fri, 12 Apr 2024 17:20:56 +0100 | From | Dave Martin <> | Subject | Re: [PATCH v1 31/31] x86/resctrl: Move the resctrl filesystem code to /fs/resctrl |
| |
On Thu, Apr 11, 2024 at 10:45:01AM -0700, Reinette Chatre wrote: > Hi Dave, > > On 4/11/2024 7:30 AM, Dave Martin wrote: > > On Tue, Mar 26, 2024 at 12:44:26PM -0700, Fenghua Yu wrote: > >> Hi, James, > >> > >> On 3/21/24 09:51, James Morse wrote: > >>> resctrl is linux's defacto interface for managing cache and bandwidth > >>> policies for groups of tasks. > >>> > >>> To allow other architectures to make use of this pseudo filesystem, > >>> move it live in /fs/resctrl instead of /arch/x86. > >>> > >>> This move leaves behind the parts of resctrl that form the architecture > >>> interface for x86. > >>> > >>> Signed-off-by: James Morse <james.morse@arm.com> > >>> --- > >>> Discussion needed on how/when to merge this, as it would conflict with > >>> all outstanding series. It's probably worth deferring to some opportune > >>> time, but is included here for illustration. > >>> --- > >>> arch/x86/kernel/cpu/resctrl/core.c | 15 - > >>> arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 505 --- > >>> arch/x86/kernel/cpu/resctrl/internal.h | 310 -- > >>> arch/x86/kernel/cpu/resctrl/monitor.c | 821 ----- > >>> arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 1093 ------ > >>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3994 -------------------- > >>> fs/resctrl/ctrlmondata.c | 527 +++ > >>> fs/resctrl/internal.h | 340 ++ > >>> fs/resctrl/monitor.c | 843 +++++ > >>> fs/resctrl/psuedo_lock.c | 1122 ++++++ > >>> fs/resctrl/rdtgroup.c | 4013 +++++++++++++++++++++ > >>> 11 files changed, 6845 insertions(+), 6738 deletions(-) > >>> > >> > >> checkpatch reports warnings and checks on this patch. Please fix them. e.g. > >> > >> CHECK: Blank lines aren't necessary before a close brace '}' > >> #13340: FILE: fs/resctrl/rdtgroup.c:3184: > >> + > >> + } > > > > Thanks for spotting these... > > > > However, this is a "move code around with no functional change" patch, > > so I think that it should paste the original code across verbatim > > without trying to address style violations. (Otherwise, there is no > > hope of checking whether this patch is correct or not...) > > I agree that this patch is too big for it to do more than just move > code (please see next comments though). > > > > > For the above example, see: > > 47820e73f5b3 ("x86/resctrl: Initialize a new resource group with default MBA values") > > > > Other than code that is moved or cloned from previously existing code, > > do you see any new style problems actually introduced by this patch? > > > > > > Notwithstanding the above, this series will conflict with a lot of the > > in-flight changes pending for resctrl, so it could be a good opportunity > > to fix some legacy style nits. > > > > Reinette, do you have a view on this? If legacy style problems get > > addressed in the moved code, are they essential for this series or could > > that be done in a follow-up? > > On its path upstream this series will be scrutinized by various checkers and > to ensure a smooth merge I would like to recommend that this series aim to > get as clean slate as possible from the basic checkers. > > Could a patch addressing these legacy issues precede this patch instead? > > I do not think all need to be addressed though. Some of the spelling warnings > are false positives and the camel case appears to be the custom for filesystem > parameter code. > > It is not obvious to me that all are legacy issues though ... could you > take a second look at the "WARNING: Use #include <linux/resctrl.h> > instead of <asm/resctrl.h>" ones? > > Reinette
Ack, that does make sense, and it's probably better than letting the dust settle on this series before applying further cleanups.
I'll make a note to review.
Some of the #include <asm/*> look to have been inherited from the previous x86 arch code where they would have been more appropriate, but perhaps some can change to <linux/*> now that more of the definitions are in the common headers.
Cheers ---Dave
| |