Message-ID: <cc452592-491f-3ddf-983a-8669ad12df9a@intel.com>
Date:   Tue, 15 Jun 2021 09:16:25 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     James Morse <james.morse@....com>, x86@...nel.org,
        linux-kernel@...r.kernel.org
Cc:     Fenghua Yu <fenghua.yu@...el.com>,
        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,
        Jamie Iles <jamie@...iainc.com>,
        D Scott Phillips OS <scott@...amperecomputing.com>,
        lcherian@...vell.com, "Luck, Tony" <tony.luck@...el.com>
Subject: Re: [PATCH v4 00/24] x86/resctrl: Merge the CDP resources

Hi James,

On 6/14/2021 1:09 PM, James Morse wrote:
> This series re-folds the resctrl code so the CDP resources (L3CODE et al)
> behaviour is all contained in the filesystem parts, with a minimum amount
> of arch specific code.
> 
> Arm have some CPU support for dividing caches into portions, and
> applying bandwidth limits at various points in the SoC. The collective term
> for these features is MPAM: Memory Partitioning and Monitoring.
> 
> MPAM is similar enough to Intel RDT, that it should use the defacto linux
> interface: resctrl. This filesystem currently lives under arch/x86, and is
> tightly coupled to the architecture.
> Ultimately, my plan is to split the existing resctrl code up to have an
> arch<->fs abstraction, then move all the bits out to fs/resctrl. From there
> MPAM can be wired up.
> 
> x86 might have two resources with cache controls, (L2 and L3) but has
> extra copies for CDP: L{2,3}{CODE,DATA}, which are marked as enabled
> if CDP is enabled for the corresponding cache.
> 
> MPAM has an equivalent feature to CDP, but its a property of the CPU,
> not the cache. Resctrl needs to have x86's odd/even behaviour, as that
> its the ABI, but this isn't how the MPAM hardware works. It is entirely
> possible that an in-kernel user of MPAM would not be using CDP, whereas
> resctrl is.
> 
> Pretending L3CODE and L3DATA are entirely separate resources is a neat
> trick, but doing this is specific to x86.
> Doing this leaves the arch code in control of various parts of the
> filesystem ABI: the resources names, and the way the schemata are parsed.
> Allowing this stuff to vary between architectures is bad for user space.
> 
> This series collapses the CODE/DATA resources, moving all the user-visible
> resctrl ABI into what becomes the filesystem code. CDP becomes the type of
> configuration being applied to a cache. This is done by adding a
> struct resctrl_schema to the parts of resctrl that will move to fs. This
> holds the arch-code resource that is in use for this schema, along with
> other properties like the name, and whether the configuration being applied
> is CODE/DATA/BOTH.
> 
> This lets us fold the extra resources out of the arch code so that they
> don't need to be duplicated if the equivalent feature to CDP is missing, or
> implemented in a different way.
> 
> 
> The first two patches split the resource and domain structs to have an
> arch specific 'hw' portion, and the rest that is visible to resctrl.
> Future series massage the resctrl code so there are no accesses to 'hw'
> structures in the parts of resctrl that will move to fs, providing helpers
> where necessary.
> 
> This series adds temporary scaffolding, which it removes a few patches
> later. This is to allow things like the ctrlval arrays and resources to be
> merged separately, which should make is easier to bisect. These things
> are marked temporary, and should all be gone by the end of the series.
> 
> This series is a little rough around the monitors, would a fake
> struct resctrl_schema for the monitors simplify things, or be a source
> of bugs?
> 
> This series is based on v5.12-rc6, and can be retrieved from:
> git://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git mpam/resctrl_merge_cdp/v4
> 
> v3: https://lore.kernel.org/lkml/20210519162424.27654-1-james.morse@arm.com/
> v2: https://lore.kernel.org/lkml/20210312175849.8327-1-james.morse@arm.com/
> v1: https://lore.kernel.org/lkml/20201030161120.227225-1-james.morse@arm.com/
> 
> Parts were previously posted as an RFC here:
> https://lore.kernel.org/lkml/20200214182947.39194-1-james.morse@arm.com/
> 

For the most part I think this series looks good. The one thing I am 
concerned about is the resctrl user interface change. On a system that 
supports L3 CDP there is a visible change when CDP is not enabled:

Before this series:
# cat schemata
     L3:0=fffff;1=fffff

After this series:
# cat schemata
L3:0=fffff;1=fffff

There are a few user space tools that parse the resctrl schemata file 
and it may be easier to keep the interface consistent than to find and 
audit them all to ensure they will keep working.

Apart from that, I do think that the dmesg change that Babu pointed out 
deserves a mention in the cover letter. I agree with your response in 
this regard but this is indeed a user visible change and if anybody has 
issue with that then mentioning it in the cover letter will hopefully 
catch it sooner.

A heads-up is that there are some kernel-doc fixups in the works that 
will conflict with your series. You yourself fix at least one of these 
kernel-doc issues in this series - the description of mbm_width in the 
first patch. I will ask the submitter of the kernel-doc fixups to use 
your text to help with the merging.

Finally, I did catch a few typos that I will respond to individually.

Thank you very much James

Reinette

Powered by blists - more mailing lists