linux-kernel - Re: [PATCH v1 31/31] x86/resctrl: Move the resctrl filesystem code to /fs/resctrl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <be03a7e9-fb0b-4edb-b22e-4452255992ad@intel.com>
Date: Thu, 11 Apr 2024 10:45:01 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Dave Martin <Dave.Martin@....com>, Fenghua Yu <fenghua.yu@...el.com>
CC: James Morse <james.morse@....com>, <x86@...nel.org>,
	<linux-kernel@...r.kernel.org>, Thomas Gleixner <tglx@...utronix.de>, "Ingo
 Molnar" <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, H Peter Anvin
	<hpa@...or.com>, Babu Moger <Babu.Moger@....com>,
	<shameerali.kolothum.thodi@...wei.com>, D Scott Phillips OS
	<scott@...amperecomputing.com>, <carl@...amperecomputing.com>,
	<lcherian@...vell.com>, <bobo.shaobowang@...wei.com>,
	<tan.shaopeng@...itsu.com>, <baolin.wang@...ux.alibaba.com>, Jamie Iles
	<quic_jiles@...cinc.com>, Xin Hao <xhao@...ux.alibaba.com>,
	<peternewman@...gle.com>, <dfustini@...libre.com>, <amitsinght@...vell.com>,
	David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>
Subject: Re: [PATCH v1 31/31] x86/resctrl: Move the resctrl filesystem code to
 /fs/resctrl

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@....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