linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Babu Moger <babu.moger@amd.com>
To: James Morse <james.morse@arm.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org
Cc: Fenghua Yu <fenghua.yu@intel.com>,
	Reinette Chatre <reinette.chatre@intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	H Peter Anvin <hpa@zytor.com>,
	shameerali.kolothum.thodi@huawei.com,
	Jamie Iles <jamie@nuviainc.com>,
	D Scott Phillips OS <scott@os.amperecomputing.com>,
	lcherian@marvell.com
Subject: Re: [PATCH v4 00/24] x86/resctrl: Merge the CDP resources
Date: Tue, 15 Jun 2021 12:50:56 -0500	[thread overview]
Message-ID: <18859c3b-a060-5358-a4cd-2a0081b3442c@amd.com> (raw)
In-Reply-To: <20210614200941.12383-1-james.morse@arm.com>

Hi James,
Thanks for the patches. Sanity tested the patches on AMD machine here.
Everything looks good. I have few comments on some of the individual patches.
Thanks

On 6/14/21 3:09 PM, James Morse wrote:
> Hi folks,
> 
> Changes since v3? Fixed  the 'unsigned u32', remove a few spaces and fixed a
> few spelling mistakes.
> ----
> 
> 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/
> 
> James Morse (24):
>   x86/resctrl: Split struct rdt_resource
>   x86/resctrl: Split struct rdt_domain
>   x86/resctrl: Add a separate schema list for resctrl
>   x86/resctrl: Pass the schema in info dir's private pointer
>   x86/resctrl: Label the resources with their configuration type
>   x86/resctrl: Walk the resctrl schema list instead of an arch list
>   x86/resctrl: Store the effective num_closid in the schema
>   x86/resctrl: Add resctrl_arch_get_num_closid()
>   x86/resctrl: Pass the schema to resctrl filesystem functions
>   x86/resctrl: Swizzle rdt_resource and resctrl_schema in
>     pseudo_lock_region
>   x86/resctrl: Move the schemata names into struct resctrl_schema
>   x86/resctrl: Group staged configuration into a separate struct
>   x86/resctrl: Allow different CODE/DATA configurations to be staged
>   x86/resctrl: Rename update_domains() resctrl_arch_update_domains()
>   x86/resctrl: Add a helper to read a closid's configuration
>   x86/resctrl: Add a helper to read/set the CDP configuration
>   x86/resctrl: Pass configuration type to resctrl_arch_get_config()
>   x86/resctrl: Make ctrlval arrays the same size
>   x86/resctrl: Apply offset correction when config is staged
>   x86/resctrl: Calculate the index from the configuration type
>   x86/resctrl: Merge the ctrl_val arrays
>   x86/resctrl: Remove rdt_cdp_peer_get()
>   x86/resctrl: Expand resctrl_arch_update_domains()'s msr_param range
>   x86/resctrl: Merge the CDP resources
> 
>  arch/x86/kernel/cpu/resctrl/core.c        | 269 +++++--------
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 164 +++++---
>  arch/x86/kernel/cpu/resctrl/internal.h    | 234 ++++-------
>  arch/x86/kernel/cpu/resctrl/monitor.c     |  44 ++-
>  arch/x86/kernel/cpu/resctrl/pseudo_lock.c |  12 +-
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 448 ++++++++++++----------
>  include/linux/resctrl.h                   | 181 +++++++++
>  7 files changed, 758 insertions(+), 594 deletions(-)
> 

      parent reply	other threads:[~2021-06-15 17:51 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-14 20:09 [PATCH v4 00/24] x86/resctrl: Merge the CDP resources James Morse
2021-06-14 20:09 ` [PATCH v4 01/24] x86/resctrl: Split struct rdt_resource James Morse
2021-06-15 18:07   ` Reinette Chatre
2021-06-14 20:09 ` [PATCH v4 02/24] x86/resctrl: Split struct rdt_domain James Morse
2021-06-15 17:51   ` Babu Moger
2021-06-17 17:02     ` James Morse
2021-06-15 18:07   ` Reinette Chatre
2021-06-14 20:09 ` [PATCH v4 03/24] x86/resctrl: Add a separate schema list for resctrl James Morse
2021-06-15 17:51   ` Babu Moger
2021-06-17 17:02     ` James Morse
2021-06-14 20:09 ` [PATCH v4 04/24] x86/resctrl: Pass the schema in info dir's private pointer James Morse
2021-06-14 20:09 ` [PATCH v4 05/24] x86/resctrl: Label the resources with their configuration type James Morse
2021-06-15 18:08   ` Reinette Chatre
2021-06-14 20:09 ` [PATCH v4 06/24] x86/resctrl: Walk the resctrl schema list instead of an arch list James Morse
2021-06-14 20:09 ` [PATCH v4 07/24] x86/resctrl: Store the effective num_closid in the schema James Morse
2021-06-14 20:09 ` [PATCH v4 08/24] x86/resctrl: Add resctrl_arch_get_num_closid() James Morse
2021-06-14 20:09 ` [PATCH v4 09/24] x86/resctrl: Pass the schema to resctrl filesystem functions James Morse
2021-06-15 18:08   ` Reinette Chatre
2021-06-14 20:09 ` [PATCH v4 10/24] x86/resctrl: Swizzle rdt_resource and resctrl_schema in pseudo_lock_region James Morse
2021-06-14 20:09 ` [PATCH v4 11/24] x86/resctrl: Move the schemata names into struct resctrl_schema James Morse
2021-06-15 18:08   ` Reinette Chatre
2021-06-14 20:09 ` [PATCH v4 12/24] x86/resctrl: Group staged configuration into a separate struct James Morse
2021-06-15 18:08   ` Reinette Chatre
2021-06-14 20:09 ` [PATCH v4 13/24] x86/resctrl: Allow different CODE/DATA configurations to be staged James Morse
2021-06-14 20:09 ` [PATCH v4 14/24] x86/resctrl: Rename update_domains() resctrl_arch_update_domains() James Morse
2021-06-14 20:09 ` [PATCH v4 15/24] x86/resctrl: Add a helper to read a closid's configuration James Morse
2021-06-14 20:09 ` [PATCH v4 16/24] x86/resctrl: Add a helper to read/set the CDP configuration James Morse
2021-06-14 20:09 ` [PATCH v4 17/24] x86/resctrl: Pass configuration type to resctrl_arch_get_config() James Morse
2021-06-14 20:09 ` [PATCH v4 18/24] x86/resctrl: Make ctrlval arrays the same size James Morse
2021-06-15 18:09   ` Reinette Chatre
2021-06-17 17:03     ` James Morse
2021-06-14 20:09 ` [PATCH v4 19/24] x86/resctrl: Apply offset correction when config is staged James Morse
2021-06-14 20:09 ` [PATCH v4 20/24] x86/resctrl: Calculate the index from the configuration type James Morse
2021-06-14 20:09 ` [PATCH v4 21/24] x86/resctrl: Merge the ctrl_val arrays James Morse
2021-06-14 20:09 ` [PATCH v4 22/24] x86/resctrl: Remove rdt_cdp_peer_get() James Morse
2021-06-14 20:09 ` [PATCH v4 23/24] x86/resctrl: Expand resctrl_arch_update_domains()'s msr_param range James Morse
2021-06-14 20:09 ` [PATCH v4 24/24] x86/resctrl: Merge the CDP resources James Morse
2021-06-15 16:16 ` [PATCH v4 00/24] " Reinette Chatre
2021-06-15 16:48   ` James Morse
2021-06-15 17:25     ` Borislav Petkov
2021-06-15 18:05     ` Reinette Chatre
2021-06-17 17:03       ` James Morse
2021-06-15 17:50 ` Babu Moger [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=18859c3b-a060-5358-a4cd-2a0081b3442c@amd.com \
    --to=babu.moger@amd.com \
    --cc=bp@alien8.de \
    --cc=fenghua.yu@intel.com \
    --cc=hpa@zytor.com \
    --cc=james.morse@arm.com \
    --cc=jamie@nuviainc.com \
    --cc=lcherian@marvell.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=reinette.chatre@intel.com \
    --cc=scott@os.amperecomputing.com \
    --cc=shameerali.kolothum.thodi@huawei.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).