Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1478824 > unrolled thread

[PATCH v2 00/33] Enable Intel Resource Allocation in Resource Director Technology

Started by"Fenghua Yu" <fenghua.yu@intel.com>
First post2016-09-08 09:00 +0200
Last post2016-09-08 12:20 +0200
Articles 20 on this page of 63 — 6 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH v2 00/33] Enable Intel Resource Allocation in Resource Director Technology  "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:00 +0200
    [PATCH v2 32/33] MAINTAINERS: Add maintainer for Intel RDT resource allocation "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:00 +0200
    [PATCH v2 30/33] x86/intel_rdt_rdtgroup.c: Process schemata input from resctrl interface "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:00 +0200
      Re: [PATCH v2 30/33] x86/intel_rdt_rdtgroup.c: Process schemata  input from resctrl interface Thomas Gleixner <tglx@linutronix.de> - 2016-09-09 00:30 +0200
    [PATCH v2 31/33] Documentation/kernel-parameters: Add kernel parameter "resctrl" for CAT "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:00 +0200
      Re: [PATCH v2 31/33] Documentation/kernel-parameters: Add kernel  parameter "resctrl" for CAT Thomas Gleixner <tglx@linutronix.de> - 2016-09-09 00:30 +0200
    [PATCH v2 28/33] x86/intel_rdt_rdtgroup.c: Read and write cpus "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 28/33] x86/intel_rdt_rdtgroup.c: Read and write cpus Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 22:30 +0200
    [PATCH v2 24/33] x86/intel_rdt_rdtgroup.c: Create info directory "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 24/33] x86/intel_rdt_rdtgroup.c: Create info  directory Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 18:10 +0200
    [PATCH v2 11/33] x86/intel_rdt: Hot cpu support for Cache Allocation "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 11/33] x86/intel_rdt: Hot cpu support for Cache  Allocation Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 12:10 +0200
    [PATCH v2 13/33] Define CONFIG_INTEL_RDT "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 13/33] Define CONFIG_INTEL_RDT Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 12:20 +0200
    [PATCH v2 08/33] x86/intel_rdt: Add Class of service management "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 08/33] x86/intel_rdt: Add Class of service  management Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 11:00 +0200
    [PATCH v2 22/33] x86/intel_rdt.c: Extend RDT to per cache and per resources "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 22/33] x86/intel_rdt.c: Extend RDT to per cache and  per resources Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 17:10 +0200
    [PATCH v2 20/33] x86/intel_rdt.h: Header for inter_rdt.c "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 20/33] x86/intel_rdt.h: Header for inter_rdt.c Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 14:40 +0200
    [PATCH v2 10/33] x86/intel_rdt: Implement scheduling support for Intel RDT "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 10/33] x86/intel_rdt: Implement scheduling support  for Intel RDT Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 12:00 +0200
    [PATCH v2 16/33] x86/intel_rdt: Class of service and capacity bitmask management for CDP "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 16/33] x86/intel_rdt: Class of service and capacity  bitmask management for CDP Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 12:40 +0200
    [PATCH v2 04/33] drivers/base/cacheinfo.c: Export some cacheinfo functions for others to use "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 04/33] drivers/base/cacheinfo.c: Export some cacheinfo  functions for others to use Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 10:30 +0200
    [PATCH v2 25/33] include/linux/resctrl.h: Define fork and exit functions in a new header file "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 25/33] include/linux/resctrl.h: Define fork and exit  functions in a new header file Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 18:20 +0200
    [PATCH v2 27/33] x86/intel_rdt_rdtgroup.c: Implement resctrl file system commands "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 27/33] x86/intel_rdt_rdtgroup.c: Implement resctrl  file system commands Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 22:20 +0200
      Re: [PATCH v2 27/33] x86/intel_rdt_rdtgroup.c: Implement resctrl  file system commands Fenghua Yu <fenghua.yu@intel.com> - 2016-09-09 00:30 +0200
    [PATCH v2 29/33] x86/intel_rdt_rdtgroup.c: Tasks iterator and write "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 29/33] x86/intel_rdt_rdtgroup.c: Tasks iterator and  write Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 23:00 +0200
    [PATCH v2 17/33] x86/intel_rdt: Hot cpu update for code data prioritization "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 17/33] x86/intel_rdt: Hot cpu update for code data  prioritization Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 12:40 +0200
    [PATCH v2 06/33] Documentation, x86: Documentation for Intel resource allocation user interface "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 06/33] Documentation, x86: Documentation for Intel  resource allocation user interface Borislav Petkov <bp@suse.de> - 2016-09-08 13:30 +0200
      Re: [PATCH v2 06/33] Documentation, x86: Documentation for Intel  resource allocation user interface Fenghua Yu <fenghua.yu@intel.com> - 2016-09-09 00:20 +0200
        Re: [PATCH v2 06/33] Documentation, x86: Documentation for Intel  resource allocation user interface Fenghua Yu <fenghua.yu@intel.com> - 2016-09-09 06:30 +0200
    [PATCH v2 03/33] x86, intel_cacheinfo: Enable cache id in x86 "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
    [PATCH v2 09/33] x86/intel_rdt: Add L3 cache capacity bitmask management "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 09/33] x86/intel_rdt: Add L3 cache capacity bitmask  management Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 11:50 +0200
    [PATCH v2 07/33] x86/intel_rdt: Add support for Cache Allocation detection "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 07/33] x86/intel_rdt: Add support for Cache Allocation  detection Borislav Petkov <bp@suse.de> - 2016-09-08 14:00 +0200
        RE: [PATCH v2 07/33] x86/intel_rdt: Add support for Cache  Allocation detection "Yu, Fenghua" <fenghua.yu@intel.com> - 2016-09-08 19:00 +0200
          Re: [PATCH v2 07/33] x86/intel_rdt: Add support for Cache Allocation  detection Borislav Petkov <bp@suse.de> - 2016-09-08 19:20 +0200
      Re: [PATCH v2 07/33] x86/intel_rdt: Add support for Cache Allocation  detection Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 15:20 +0200
        RE: [PATCH v2 07/33] x86/intel_rdt: Add support for Cache  Allocation detection "Yu, Fenghua" <fenghua.yu@intel.com> - 2016-09-08 16:00 +0200
    [PATCH v2 01/33] cacheinfo: Introduce cache id "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
    [PATCH v2 05/33] x86/intel_rdt: Cache Allocation documentation "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
    [PATCH v2 19/33] magic number for resctrl file system "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 19/33] magic number for resctrl file system Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 12:50 +0200
        Re: [PATCH v2 19/33] magic number for resctrl file system Borislav Petkov <bp@alien8.de> - 2016-09-08 12:50 +0200
    [PATCH v2 21/33] x86/intel_rdt_rdtgroup.h: Header for user interface "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 21/33] x86/intel_rdt_rdtgroup.h: Header for user  interface Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 14:50 +0200
    [PATCH v2 26/33] Task fork and exit for rdtgroup "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 26/33] Task fork and exit for rdtgroup Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 21:50 +0200
    [PATCH v2 18/33] sched.h: Add rg_list and rdtgroup in task_struct "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 18/33] sched.h: Add rg_list and rdtgroup in  task_struct Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 12:40 +0200
    [PATCH v2 02/33] Documentation, ABI: Add a document entry for cache id "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 02/33] Documentation, ABI: Add a document entry for  cache id Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 21:40 +0200
    [PATCH v2 12/33] x86/intel_rdt: Intel haswell Cache Allocation enumeration "Fenghua Yu" <fenghua.yu@intel.com> - 2016-09-08 09:10 +0200
      Re: [PATCH v2 12/33] x86/intel_rdt: Intel haswell Cache Allocation  enumeration Thomas Gleixner <tglx@linutronix.de> - 2016-09-08 12:20 +0200

Page 1 of 4  [1] 2 3 4  Next page →


#1478824 — [PATCH v2 00/33] Enable Intel Resource Allocation in Resource Director Technology

From"Fenghua Yu" <fenghua.yu@intel.com>
Date2016-09-08 09:00 +0200
Subject[PATCH v2 00/33] Enable Intel Resource Allocation in Resource Director Technology
Message-ID<sf1tv-2mB-5@gated-at.bofh.it>
From: Fenghua Yu <fenghua.yu@intel.com>

L3 cache allocation allows per task control over which areas of the last
level cache are available for allocation. It is the first resource that
can be controlled as part of Intel Resource Director Technology (RDT).
This patch series creates a framework that will make it easy to add
additional resources (like L2 cache).

See Intel Software Developer manual volume 3, chapter 17 for architectural
details. Also Documentation/x86/intel_rdt.txt and
Documentation/x86/intel_rdt_ui.txt (in parts 0001 & 0013 of this patch
series).

A previous implementation used "cgroups" as the user interface. This was
rejected.

The new interface:
1) Aligns better with the h/w capabilities provided
2) Gives finer control (per thread instead of process)
3) Gives control over kernel threads as well as user threads
4) Allows resource allocation policies to be tied to certain cpus across
all contexts (tglx request)

Note 1: that parts 1-12 are largely unchanged from what was posted last
year except for the removal of cgroup pieces and dynamic CAT/CDP switch.

Note 2: This patch set is an infrastructure for future multi-resources
support. Some code are not only for L3. For example, cat_l3_enabled
is checked after initialization and mount time. This doesn't make
sense for L3 only. But it's easy to add L2 support on top of the
current patch set.

Changes:
v2:
- Merge and reorder some patches
- "tasks" has higher priority than "cpus".
- Re-write UI doc.
- Add include/linux/resctrl.h
- Remove rg_list
- A few other changes.

Fenghua Yu (21):
  cacheinfo: Introduce cache id
  Documentation, ABI: Add a document entry for cache id
  x86, intel_cacheinfo: Enable cache id in x86
  drivers/base/cacheinfo.c: Export some cacheinfo functions for others
    to use
  Documentation, x86: Documentation for Intel resource allocation user
    interface
  sched.h: Add rg_list and rdtgroup in task_struct
  magic number for resctrl file system
  x86/intel_rdt.h: Header for inter_rdt.c
  x86/intel_rdt_rdtgroup.h: Header for user interface
  x86/intel_rdt.c: Extend RDT to per cache and per resources
  x86/intel_rdt_rdtgroup.c: User interface for RDT
  x86/intel_rdt_rdtgroup.c: Create info directory
  include/linux/resctrl.h: Define fork and exit functions in a new
    header file
  Task fork and exit for rdtgroup
  x86/intel_rdt_rdtgroup.c: Implement resctrl file system commands
  x86/intel_rdt_rdtgroup.c: Read and write cpus
  x86/intel_rdt_rdtgroup.c: Tasks iterator and write
  x86/intel_rdt_rdtgroup.c: Process schemata input from resctrl
    interface
  Documentation/kernel-parameters: Add kernel parameter "resctrl" for
    CAT
  MAINTAINERS: Add maintainer for Intel RDT resource allocation
  x86/Makefile: Build intel_rdt_rdtgroup.c

Vikas Shivappa (12):
  x86/intel_rdt: Cache Allocation documentation
  x86/intel_rdt: Add support for Cache Allocation detection
  x86/intel_rdt: Add Class of service management
  x86/intel_rdt: Add L3 cache capacity bitmask management
  x86/intel_rdt: Implement scheduling support for Intel RDT
  x86/intel_rdt: Hot cpu support for Cache Allocation
  x86/intel_rdt: Intel haswell Cache Allocation enumeration
  Define CONFIG_INTEL_RDT
  x86/intel_rdt: Intel Code Data Prioritization detection
  x86/intel_rdt: Adds support to enable Code Data Prioritization
  x86/intel_rdt: Class of service and capacity bitmask management for
    CDP
  x86/intel_rdt: Hot cpu update for code data prioritization

 Documentation/ABI/testing/sysfs-devices-system-cpu |   17 +
 Documentation/kernel-parameters.txt                |   13 +
 Documentation/x86/intel_rdt.txt                    |  109 ++
 Documentation/x86/intel_rdt_ui.txt                 |  164 +++
 MAINTAINERS                                        |    9 +
 arch/x86/Kconfig                                   |   13 +
 arch/x86/events/intel/cqm.c                        |   24 +-
 arch/x86/include/asm/cpufeature.h                  |   10 +-
 arch/x86/include/asm/cpufeatures.h                 |    9 +-
 arch/x86/include/asm/disabled-features.h           |    4 +-
 arch/x86/include/asm/intel_rdt.h                   |  129 ++
 arch/x86/include/asm/intel_rdt_rdtgroup.h          |  164 +++
 arch/x86/include/asm/pqr_common.h                  |   27 +
 arch/x86/include/asm/processor.h                   |    3 +
 arch/x86/include/asm/required-features.h           |    4 +-
 arch/x86/kernel/cpu/Makefile                       |    2 +
 arch/x86/kernel/cpu/common.c                       |   19 +
 arch/x86/kernel/cpu/intel_cacheinfo.c              |   20 +
 arch/x86/kernel/cpu/intel_rdt.c                    |  802 ++++++++++++
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c           | 1362 ++++++++++++++++++++
 arch/x86/kernel/cpu/intel_rdt_schemata.c           |  674 ++++++++++
 arch/x86/kernel/process_64.c                       |    6 +
 drivers/base/cacheinfo.c                           |    7 +-
 include/linux/cacheinfo.h                          |    5 +
 include/linux/resctrl.h                            |   12 +
 include/linux/sched.h                              |    3 +
 include/uapi/linux/magic.h                         |    2 +
 kernel/exit.c                                      |    2 +
 kernel/fork.c                                      |    2 +
 29 files changed, 3589 insertions(+), 28 deletions(-)
 create mode 100644 Documentation/x86/intel_rdt.txt
 create mode 100644 Documentation/x86/intel_rdt_ui.txt
 create mode 100644 arch/x86/include/asm/intel_rdt.h
 create mode 100644 arch/x86/include/asm/intel_rdt_rdtgroup.h
 create mode 100644 arch/x86/include/asm/pqr_common.h
 create mode 100644 arch/x86/kernel/cpu/intel_rdt.c
 create mode 100644 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
 create mode 100644 arch/x86/kernel/cpu/intel_rdt_schemata.c
 create mode 100644 include/linux/resctrl.h

-- 
2.5.0

[toc] | [next] | [standalone]


#1478825 — [PATCH v2 32/33] MAINTAINERS: Add maintainer for Intel RDT resource allocation

From"Fenghua Yu" <fenghua.yu@intel.com>
Date2016-09-08 09:00 +0200
Subject[PATCH v2 32/33] MAINTAINERS: Add maintainer for Intel RDT resource allocation
Message-ID<sf1tw-2mB-39@gated-at.bofh.it>
In reply to#1478824
From: Fenghua Yu <fenghua.yu@intel.com>

We create six new files for Intel RDT resource allocation:
arch/x86/kernel/cpu/intel_rdt.c
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
arch/x86/include/asm/intel_rdt.h
arch/x86/include/asm/intel_rdt_rdtgroup.h
Documentation/x86/intel_rdt.txt
Documentation/x86/intel_rdt_ui.txt

Add maintainer in MAINTAINERS to maintain the files.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index db814a8..de9e47a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9833,6 +9833,15 @@ L:	linux-rdma@vger.kernel.org
 S:	Supported
 F:	drivers/infiniband/sw/rdmavt
 
+RDT - RESOURCE ALLOCATION
+M:	Fenghua Yu <fenghua.yu@intel.com>
+L:	linux-kernel@vger.kernel.org
+S:	Supported
+F:	arch/x86/kernel/cpu/intel_rdt*
+F:	arch/x86/include/asm/intel_rdt*
+F:	Documentation/x86/intel_rdt*
+F:	include/linux/resctrl.h
+
 READ-COPY UPDATE (RCU)
 M:	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
 M:	Josh Triplett <josh@joshtriplett.org>
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]


#1478827 — [PATCH v2 30/33] x86/intel_rdt_rdtgroup.c: Process schemata input from resctrl interface

From"Fenghua Yu" <fenghua.yu@intel.com>
Date2016-09-08 09:00 +0200
Subject[PATCH v2 30/33] x86/intel_rdt_rdtgroup.c: Process schemata input from resctrl interface
Message-ID<sf1tw-2mB-45@gated-at.bofh.it>
In reply to#1478824
From: Fenghua Yu <fenghua.yu@intel.com>

There is one "schemata" file in each rdtgroup directory. User can input
schemata in the file to control how to allocate resources.

The input schemata first needs to pass validation. If there is no syntax
issue, kernel digests the input schemata and find CLOSID for each
domain for each resource.

A shared domain covers a few different resource domains which share
the same CLOSID. Kernel will find a CLOSID in each shared domain. If
an existing CLOSID and its CBMs match input schemata, the CLOSID is
shared by this rdtgroup. Otherwise, kernel tries to alloc a new
CLOSID for this rdtgroup. If a new CLOSID is available, update QoS MASK
MSRs. If no more CLOSID is available, kernel report ENODEV to user.

A shared domain is in preparation for multiple resources (like L2)
that will be added very soon.

User can read the schemata saved in the file.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/intel_rdt_rdtgroup.h |   6 +
 arch/x86/kernel/cpu/intel_rdt_schemata.c  | 674 ++++++++++++++++++++++++++++++
 2 files changed, 680 insertions(+)
 create mode 100644 arch/x86/kernel/cpu/intel_rdt_schemata.c

diff --git a/arch/x86/include/asm/intel_rdt_rdtgroup.h b/arch/x86/include/asm/intel_rdt_rdtgroup.h
index 43a3b83..782513e 100644
--- a/arch/x86/include/asm/intel_rdt_rdtgroup.h
+++ b/arch/x86/include/asm/intel_rdt_rdtgroup.h
@@ -17,6 +17,12 @@ extern struct list_head rdtgroup_lists;
 extern struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn);
 extern void rdtgroup_kn_unlock(struct kernfs_node *kn);
 
+/* Defiend in intel_rdt_schemata.c. */
+extern int get_default_resources(struct rdtgroup *rdtgrp);
+extern ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
+				char *buf, size_t nbytes, loff_t off);
+extern int rdtgroup_schemata_show(struct seq_file *s, void *v);
+
 /* cftype->flags */
 enum {
 	RFTYPE_WORLD_WRITABLE = (1 << 4),/* (DON'T USE FOR NEW FILES) S_IWUGO */
diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
new file mode 100644
index 0000000..4e624f0
--- /dev/null
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -0,0 +1,674 @@
+#include <linux/slab.h>
+#include <asm/intel_rdt_rdtgroup.h>
+
+struct resources {
+	struct cache_resource *l3;
+};
+
+static int get_res_type(char **res, enum resource_type *res_type)
+{
+	char *tok;
+
+	tok = strsep(res, ":");
+	if (tok == NULL)
+		return -EINVAL;
+
+	if (!strcmp(tok, "L3")) {
+		*res_type = RESOURCE_L3;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int divide_resources(char *buf, char *resources[RESOURCE_NUM])
+{
+	char *tok;
+	unsigned int resource_num = 0;
+	int ret = 0;
+	char *res;
+	char *res_block;
+	size_t size;
+	enum resource_type res_type;
+
+	size = strlen(buf) + 1;
+	res = kzalloc(size, GFP_KERNEL);
+	if (!res) {
+		ret = -ENOSPC;
+		goto out;
+	}
+
+	while ((tok = strsep(&buf, "\n")) != NULL) {
+		if (strlen(tok) == 0)
+			break;
+		if (resource_num++ >= 1) {
+			pr_info("More than one line of resource input!\n");
+			ret = -EINVAL;
+			goto out;
+		}
+		strcpy(res, tok);
+	}
+
+	res_block = res;
+	ret = get_res_type(&res_block, &res_type);
+	if (ret) {
+		pr_info("Unknown resource type!");
+		goto out;
+	}
+
+	if (res_block == NULL) {
+		pr_info("Invalid resource value!");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (res_type == RESOURCE_L3 && cat_enabled(CACHE_LEVEL3)) {
+		strcpy(resources[RESOURCE_L3], res_block);
+	} else {
+		pr_info("Invalid resource type!");
+		goto out;
+	}
+
+	ret = 0;
+
+out:
+	kfree(res);
+	return ret;
+}
+
+static bool cbm_validate(unsigned long var, int level)
+{
+	u32 maxcbmlen = max_cbm_len(level);
+	unsigned long first_bit, zero_bit;
+
+	if (bitmap_weight(&var, maxcbmlen) < min_bitmask_len)
+		return false;
+
+	if (var & ~max_cbm(level))
+		return false;
+
+	first_bit = find_first_bit(&var, maxcbmlen);
+	zero_bit = find_next_zero_bit(&var, maxcbmlen, first_bit);
+
+	if (find_next_bit(&var, maxcbmlen, zero_bit) < maxcbmlen)
+		return false;
+
+	return true;
+}
+
+static int get_input_cbm(char *tok, struct cache_resource *l,
+			 int input_domain_num, int level)
+{
+	int ret;
+
+	if (!cdp_enabled) {
+		if (tok == NULL)
+			return -EINVAL;
+
+		ret = kstrtoul(tok, 16,
+			       (unsigned long *)&l->cbm[input_domain_num]);
+		if (ret)
+			return ret;
+
+		if (!cbm_validate(l->cbm[input_domain_num], level))
+			return -EINVAL;
+	} else  {
+		char *input_cbm1_str;
+
+		input_cbm1_str = strsep(&tok, ",");
+		if (input_cbm1_str == NULL || tok == NULL)
+			return -EINVAL;
+
+		ret = kstrtoul(input_cbm1_str, 16,
+			       (unsigned long *)&l->cbm[input_domain_num]);
+		if (ret)
+			return ret;
+
+		if (!cbm_validate(l->cbm[input_domain_num], level))
+			return -EINVAL;
+
+		ret = kstrtoul(tok, 16,
+			       (unsigned long *)&l->cbm2[input_domain_num]);
+		if (ret)
+			return ret;
+
+		if (!cbm_validate(l->cbm2[input_domain_num], level))
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int get_cache_schema(char *buf, struct cache_resource *l, int level,
+			 struct rdtgroup *rdtgrp)
+{
+	char *tok, *tok_cache_id;
+	int ret;
+	int domain_num;
+	int input_domain_num;
+	int len;
+	unsigned int input_cache_id;
+	unsigned int cid;
+	unsigned int leaf;
+
+	if (!cat_enabled(level) && strcmp(buf, ";")) {
+		pr_info("Disabled resource should have empty schema\n");
+		return -EINVAL;
+	}
+
+	len = strlen(buf);
+	/*
+	 * Translate cache id based cbm from one line string with format
+	 * "<cache prefix>:<cache id0>=xxxx;<cache id1>=xxxx;..." for
+	 * disabled cdp.
+	 * Or
+	 * "<cache prefix>:<cache id0>=xxxxx,xxxxx;<cache id1>=xxxxx,xxxxx;..."
+	 * for enabled cdp.
+	 */
+	input_domain_num = 0;
+	while ((tok = strsep(&buf, ";")) != NULL) {
+		tok_cache_id = strsep(&tok, "=");
+		if (tok_cache_id == NULL)
+			goto cache_id_err;
+
+		ret = kstrtouint(tok_cache_id, 16, &input_cache_id);
+		if (ret)
+			goto cache_id_err;
+
+		leaf = level_to_leaf(level);
+		cid = cache_domains[leaf].shared_cache_id[input_domain_num];
+		if (input_cache_id != cid)
+			goto cache_id_err;
+
+		ret = get_input_cbm(tok, l, input_domain_num, level);
+		if (ret)
+			goto cbm_err;
+
+		input_domain_num++;
+		if (input_domain_num > get_domain_num(level)) {
+			pr_info("domain number is more than max %d\n",
+				MAX_CACHE_DOMAINS);
+			return -EINVAL;
+		}
+	}
+
+	domain_num = get_domain_num(level);
+	if (domain_num != input_domain_num) {
+		pr_info("%s input domain number %d doesn't match domain number %d\n",
+			"l3",
+			input_domain_num, domain_num);
+
+		return -EINVAL;
+	}
+
+	return 0;
+
+cache_id_err:
+	pr_info("Invalid cache id in field %d for L%1d\n", input_domain_num,
+		level);
+	return -EINVAL;
+
+cbm_err:
+	pr_info("Invalid cbm in field %d for cache L%d\n",
+		input_domain_num, level);
+	return -EINVAL;
+}
+
+static bool cbm_found(struct cache_resource *l, struct rdtgroup *r,
+		      int domain, int level)
+{
+	int closid;
+	int l3_domain;
+	u64 cctable_cbm;
+	u64 cbm;
+	int dindex;
+
+	closid = r->resource.closid[domain];
+
+	if (level == CACHE_LEVEL3) {
+		l3_domain = shared_domain[domain].l3_domain;
+		cbm = l->cbm[l3_domain];
+		dindex = get_dcbm_table_index(closid);
+		cctable_cbm = l3_cctable[l3_domain][dindex].cbm;
+		if (cdp_enabled) {
+			u64 icbm;
+			u64 cctable_icbm;
+			int iindex;
+
+			icbm = l->cbm2[l3_domain];
+			iindex = get_icbm_table_index(closid);
+			cctable_icbm = l3_cctable[l3_domain][iindex].cbm;
+
+			return cbm == cctable_cbm && icbm == cctable_icbm;
+		}
+
+		return cbm == cctable_cbm;
+	}
+
+	return false;
+}
+
+enum {
+	CURRENT_CLOSID,
+	REUSED_OWN_CLOSID,
+	REUSED_OTHER_CLOSID,
+	NEW_CLOSID,
+};
+
+/*
+ * Check if the reference counts are all ones in rdtgrp's domain.
+ */
+static bool one_refcnt(struct rdtgroup *rdtgrp, int domain)
+{
+	int refcnt;
+	int closid;
+
+	closid = rdtgrp->resource.closid[domain];
+	if (cat_l3_enabled) {
+		int l3_domain;
+		int dindex;
+
+		l3_domain = shared_domain[domain].l3_domain;
+		dindex = get_dcbm_table_index(closid);
+		refcnt = l3_cctable[l3_domain][dindex].clos_refcnt;
+		if (refcnt != 1)
+			return false;
+
+		if (cdp_enabled) {
+			int iindex;
+
+			iindex = get_icbm_table_index(closid);
+			refcnt = l3_cctable[l3_domain][iindex].clos_refcnt;
+
+			if (refcnt != 1)
+				return false;
+		}
+	}
+
+	return true;
+}
+
+/*
+ * Go through all shared domains. Check if there is an existing closid
+ * in all rdtgroups that matches l3 cbms in the shared
+ * domain. If find one, reuse the closid. Otherwise, allocate a new one.
+ */
+static int get_rdtgroup_resources(struct resources *resources_set,
+				  struct rdtgroup *rdtgrp)
+{
+	struct cache_resource *l3;
+	bool l3_cbm_found;
+	struct list_head *l;
+	struct rdtgroup *r;
+	u64 cbm;
+	int rdt_closid[MAX_CACHE_DOMAINS];
+	int rdt_closid_type[MAX_CACHE_DOMAINS];
+	int domain;
+	int closid;
+	int ret;
+
+	l3 = resources_set->l3;
+	memcpy(rdt_closid, rdtgrp->resource.closid,
+	       shared_domain_num * sizeof(int));
+	for (domain = 0; domain < shared_domain_num; domain++) {
+		if (rdtgrp->resource.valid) {
+			/*
+			 * If current rdtgrp is the only user of cbms in
+			 * this domain, will replace the cbms with the input
+			 * cbms and reuse its own closid.
+			 */
+			if (one_refcnt(rdtgrp, domain)) {
+				closid = rdtgrp->resource.closid[domain];
+				rdt_closid[domain] = closid;
+				rdt_closid_type[domain] = REUSED_OWN_CLOSID;
+				continue;
+			}
+
+			l3_cbm_found = true;
+
+			if (cat_l3_enabled)
+				l3_cbm_found = cbm_found(l3, rdtgrp, domain,
+							 CACHE_LEVEL3);
+
+			/*
+			 * If the cbms in this shared domain are already
+			 * existing in current rdtgrp, record the closid
+			 * and its type.
+			 */
+			if (l3_cbm_found) {
+				closid = rdtgrp->resource.closid[domain];
+				rdt_closid[domain] = closid;
+				rdt_closid_type[domain] = CURRENT_CLOSID;
+				continue;
+			}
+		}
+
+		/*
+		 * If the cbms are not found in this rdtgrp, search other
+		 * rdtgroups and see if there are matched cbms.
+		 */
+		l3_cbm_found = cat_l3_enabled ? false : true;
+		list_for_each(l, &rdtgroup_lists) {
+			r = list_entry(l, struct rdtgroup, rdtgroup_list);
+			if (r == rdtgrp || !r->resource.valid)
+				continue;
+
+			if (cat_l3_enabled)
+				l3_cbm_found = cbm_found(l3, r, domain,
+							 CACHE_LEVEL3);
+
+			if (l3_cbm_found) {
+				/* Get the closid that matches l3 cbms.*/
+				closid = r->resource.closid[domain];
+				rdt_closid[domain] = closid;
+				rdt_closid_type[domain] = REUSED_OTHER_CLOSID;
+				break;
+			}
+		}
+
+		if (!l3_cbm_found) {
+			/*
+			 * If no existing closid is found, allocate
+			 * a new one.
+			 */
+			ret = closid_alloc(&closid, domain);
+			if (ret)
+				goto err;
+			rdt_closid[domain] = closid;
+			rdt_closid_type[domain] = NEW_CLOSID;
+		}
+	}
+
+	/*
+	 * Now all closid are ready in rdt_closid. Update rdtgrp's closid.
+	 */
+	for_each_cache_domain(domain, 0, shared_domain_num) {
+		/*
+		 * Nothing is changed if the same closid and same cbms were
+		 * found in this rdtgrp's domain.
+		 */
+		if (rdt_closid_type[domain] == CURRENT_CLOSID)
+			continue;
+
+		/*
+		 * Put rdtgroup closid. No need to put the closid if we
+		 * just change cbms and keep the closid (REUSED_OWN_CLOSID).
+		 */
+		if (rdtgrp->resource.valid &&
+		    rdt_closid_type[domain] != REUSED_OWN_CLOSID) {
+			/* Put old closid in this rdtgrp's domain if valid. */
+			closid = rdtgrp->resource.closid[domain];
+			closid_put(closid, domain);
+		}
+
+		/*
+		 * Replace the closid in this rdtgrp's domain with saved
+		 * closid that was newly allocted (NEW_CLOSID), or found in
+		 * another rdtgroup's domains (REUSED_CLOSID), or found in
+		 * this rdtgrp (REUSED_OWN_CLOSID).
+		 */
+		closid = rdt_closid[domain];
+		rdtgrp->resource.closid[domain] = closid;
+
+		/*
+		 * Get the reused other rdtgroup's closid. No need to get the
+		 * closid newly allocated (NEW_CLOSID) because it's been
+		 * already got in closid_alloc(). And no need to get the closid
+		 * for resued own closid (REUSED_OWN_CLOSID).
+		 */
+		if (rdt_closid_type[domain] == REUSED_OTHER_CLOSID)
+			closid_get(closid, domain);
+
+		/*
+		 * If the closid comes from a newly allocated closid
+		 * (NEW_CLOSID), or found in this rdtgrp (REUSED_OWN_CLOSID),
+		 * cbms for this closid will be updated in MSRs.
+		 */
+		if (rdt_closid_type[domain] == NEW_CLOSID ||
+		    rdt_closid_type[domain] == REUSED_OWN_CLOSID) {
+			/*
+			 * Update cbm in cctable with the newly allocated
+			 * closid.
+			 */
+			if (cat_l3_enabled) {
+				int cpu;
+				struct cpumask *mask;
+				int dindex;
+				int l3_domain = shared_domain[domain].l3_domain;
+				int leaf = level_to_leaf(CACHE_LEVEL3);
+
+				cbm = l3->cbm[l3_domain];
+				dindex = get_dcbm_table_index(closid);
+				l3_cctable[l3_domain][dindex].cbm = cbm;
+				if (cdp_enabled) {
+					int iindex;
+
+					cbm = l3->cbm2[l3_domain];
+					iindex = get_icbm_table_index(closid);
+					l3_cctable[l3_domain][iindex].cbm = cbm;
+				}
+
+				mask =
+				&cache_domains[leaf].shared_cpu_map[l3_domain];
+
+				cpu = cpumask_first(mask);
+				smp_call_function_single(cpu, cbm_update_l3_msr,
+							 &closid, 1);
+			}
+		}
+	}
+
+	rdtgrp->resource.valid = true;
+
+	return 0;
+err:
+	/* Free previously allocated closid. */
+	for_each_cache_domain(domain, 0, shared_domain_num) {
+		if (rdt_closid_type[domain] != NEW_CLOSID)
+			continue;
+
+		closid_put(rdt_closid[domain], domain);
+
+	}
+
+	return ret;
+}
+
+static void init_cache_resource(struct cache_resource *l)
+{
+	l->cbm = NULL;
+	l->cbm2 = NULL;
+	l->closid = NULL;
+	l->refcnt = NULL;
+}
+
+static void free_cache_resource(struct cache_resource *l)
+{
+	kfree(l->cbm);
+	kfree(l->cbm2);
+	kfree(l->closid);
+	kfree(l->refcnt);
+}
+
+static int alloc_cache_resource(struct cache_resource *l, int level)
+{
+	int domain_num = get_domain_num(level);
+
+	l->cbm = kcalloc(domain_num, sizeof(*l->cbm), GFP_KERNEL);
+	l->cbm2 = kcalloc(domain_num, sizeof(*l->cbm2), GFP_KERNEL);
+	l->closid = kcalloc(domain_num, sizeof(*l->closid), GFP_KERNEL);
+	l->refcnt = kcalloc(domain_num, sizeof(*l->refcnt), GFP_KERNEL);
+	if (l->cbm && l->cbm2 && l->closid && l->refcnt)
+		return 0;
+
+	return -ENOMEM;
+}
+
+/*
+ * This function digests schemata given in text buf. If the schemata are in
+ * right format and there is enough closid, input the schemata in rdtgrp
+ * and update resource cctables.
+ *
+ * Inputs:
+ *	buf: string buffer containing schemata
+ *	rdtgrp: current rdtgroup holding schemata.
+ *
+ * Return:
+ *	0 on success or error code.
+ */
+static int get_resources(char *buf, struct rdtgroup *rdtgrp)
+{
+	char *resources[RESOURCE_NUM];
+	struct cache_resource l3;
+	struct resources resources_set;
+	int ret;
+	char *resources_block;
+	int i;
+	int size = strlen(buf) + 1;
+
+	resources_block = kcalloc(RESOURCE_NUM, size, GFP_KERNEL);
+	if (!resources_block)
+		return -ENOMEM;
+
+	for (i = 0; i < RESOURCE_NUM; i++)
+		resources[i] = (char *)(resources_block + i * size);
+
+	ret = divide_resources(buf, resources);
+	if (ret) {
+		kfree(resources_block);
+		return -EINVAL;
+	}
+
+	init_cache_resource(&l3);
+
+	if (cat_l3_enabled) {
+		ret = alloc_cache_resource(&l3, CACHE_LEVEL3);
+		if (ret)
+			goto out;
+
+		ret = get_cache_schema(resources[RESOURCE_L3], &l3,
+				       CACHE_LEVEL3, rdtgrp);
+		if (ret)
+			goto out;
+
+		resources_set.l3 = &l3;
+	} else
+		resources_set.l3 = NULL;
+
+	ret = get_rdtgroup_resources(&resources_set, rdtgrp);
+
+out:
+	kfree(resources_block);
+	free_cache_resource(&l3);
+
+	return ret;
+}
+
+static void gen_cache_prefix(char *buf, int level)
+{
+	sprintf(buf, "L%1d:", level == CACHE_LEVEL3 ? 3 : 2);
+}
+
+static int get_cache_id(int domain, int level)
+{
+	return cache_domains[level_to_leaf(level)].shared_cache_id[domain];
+}
+
+static void gen_cache_buf(char *buf, int level)
+{
+	int domain;
+	char buf1[32];
+	int domain_num;
+	u64 val;
+
+	gen_cache_prefix(buf, level);
+
+	domain_num = get_domain_num(level);
+
+	val = max_cbm(level);
+
+	for (domain = 0; domain < domain_num; domain++) {
+		sprintf(buf1, "%d=%lx", get_cache_id(domain, level),
+			(unsigned long)val);
+		strcat(buf, buf1);
+		if (cdp_enabled) {
+			sprintf(buf1, ",%lx", (unsigned long)val);
+			strcat(buf, buf1);
+		}
+		if (domain < domain_num - 1)
+			strcat(buf, ";");
+		else
+			strcat(buf, "\n");
+	}
+}
+
+/*
+ * Set up default schemata in a rdtgroup. All schemata in all resources are
+ * default values (all 1's) for all domains.
+ *
+ * Input: rdtgroup.
+ * Return: 0: successful
+ *	   non-0: error code
+ */
+int get_default_resources(struct rdtgroup *rdtgrp)
+{
+	char schema[1024];
+	int ret = 0;
+
+	if (cat_enabled(CACHE_LEVEL3)) {
+		gen_cache_buf(schema, CACHE_LEVEL3);
+
+		if (strlen(schema)) {
+			ret = get_resources(schema, rdtgrp);
+			if (ret)
+				return ret;
+		}
+		gen_cache_buf(rdtgrp->schema, CACHE_LEVEL3);
+	}
+
+	return ret;
+}
+
+ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
+			char *buf, size_t nbytes, loff_t off)
+{
+	int ret = 0;
+	struct rdtgroup *rdtgrp;
+	char *schema;
+
+	rdtgrp = rdtgroup_kn_lock_live(of->kn);
+	if (!rdtgrp)
+		return -ENODEV;
+
+	schema = kzalloc(sizeof(char) * strlen(buf) + 1, GFP_KERNEL);
+	if (!schema) {
+		ret = -ENOMEM;
+		goto out_unlock;
+	}
+
+	memcpy(schema, buf, strlen(buf) + 1);
+
+	ret = get_resources(buf, rdtgrp);
+	if (ret)
+		goto out;
+
+	memcpy(rdtgrp->schema, schema, strlen(schema) + 1);
+
+out:
+	kfree(schema);
+
+out_unlock:
+	rdtgroup_kn_unlock(of->kn);
+	return ret ?: nbytes;
+}
+
+int rdtgroup_schemata_show(struct seq_file *s, void *v)
+{
+	struct kernfs_open_file *of = s->private;
+	struct rdtgroup *rdtgrp;
+
+	rdtgrp = rdtgroup_kn_lock_live(of->kn);
+	seq_printf(s, "%s", rdtgrp->schema);
+	rdtgroup_kn_unlock(of->kn);
+	return 0;
+}
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]


#1479562 — Re: [PATCH v2 30/33] x86/intel_rdt_rdtgroup.c: Process schemata input from resctrl interface

FromThomas Gleixner <tglx@linutronix.de>
Date2016-09-09 00:30 +0200
SubjectRe: [PATCH v2 30/33] x86/intel_rdt_rdtgroup.c: Process schemata input from resctrl interface
Message-ID<sffZw-3f7-37@gated-at.bofh.it>
In reply to#1478827
On Thu, 8 Sep 2016, Fenghua Yu wrote:

> +struct resources {

Darn. The first look made me parse this as a redefinition of 'struct
resource' ... Can't you find an even more generic name for this?

> +	struct cache_resource *l3;
> +};
> +
> +static int get_res_type(char **res, enum resource_type *res_type)
> +{
> +	char *tok;
> +
> +	tok = strsep(res, ":");
> +	if (tok == NULL)

We still write: if (!tok) as anywhere else.

> +static int divide_resources(char *buf, char *resources[RESOURCE_NUM])
> +{
> +	char *tok;
> +	unsigned int resource_num = 0;
> +	int ret = 0;
> +	char *res;
> +	char *res_block;
> +	size_t size;
> +	enum resource_type res_type;

Sigh.

> +
> +	size = strlen(buf) + 1;
> +	res = kzalloc(size, GFP_KERNEL);
> +	if (!res) {
> +		ret = -ENOSPC;
> +		goto out;
> +	}
> +
> +	while ((tok = strsep(&buf, "\n")) != NULL) {
> +		if (strlen(tok) == 0)
> +			break;
> +		if (resource_num++ >= 1) {

How gets that ever greater 1?

> +			pr_info("More than one line of resource input!\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		strcpy(res, tok);
> +	}
> +
> +	res_block = res;
> +	ret = get_res_type(&res_block, &res_type);
> +	if (ret) {
> +		pr_info("Unknown resource type!");
> +		goto out;
> +	}
> +
> +	if (res_block == NULL) {
> +		pr_info("Invalid resource value!");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (res_type == RESOURCE_L3 && cat_enabled(CACHE_LEVEL3)) {
> +		strcpy(resources[RESOURCE_L3], res_block);
> +	} else {
> +		pr_info("Invalid resource type!");
> +		goto out;
> +	}
> +
> +	ret = 0;


You surely found the most convoluted solution for this. Whats wrong with:

	data = get_res_type(res, &type);
	if (IS_ERR(data)) {
		ret = PTR_ERR(data);
		goto out;
	} 

	ret = 0;
	switch (type) {
	case RESOURCE_L3:
		if (cat_enabled(CACHE_LEVEL3))
			strcpy(resources[RESOURCE_L3], data);
		break;
	default:
		ret = -EINVAL;
	}	

That's too simple to understand and too extensible for future resource
types, right?

> +out:
> +	kfree(res);
> +	return ret;
> +}

> +static int get_input_cbm(char *tok, struct cache_resource *l,
> +			 int input_domain_num, int level)
> +{
> +	int ret;
> +
> +	if (!cdp_enabled) {
> +		if (tok == NULL)
> +			return -EINVAL;
> +
> +		ret = kstrtoul(tok, 16,
> +			       (unsigned long *)&l->cbm[input_domain_num]);

What is this type cast for? Can't you just parse the data into a local
unsigned long and then store it after validation?

> +		if (ret)
> +			return ret;
> +
> +		if (!cbm_validate(l->cbm[input_domain_num], level))
> +			return -EINVAL;
> +	} else  {
> +		char *input_cbm1_str;
> +
> +		input_cbm1_str = strsep(&tok, ",");
> +		if (input_cbm1_str == NULL || tok == NULL)
> +			return -EINVAL;
> +
> +		ret = kstrtoul(input_cbm1_str, 16,
> +			       (unsigned long *)&l->cbm[input_domain_num]);
> +		if (ret)
> +			return ret;
> +
> +		if (!cbm_validate(l->cbm[input_domain_num], level))
> +			return -EINVAL;
> +
> +		ret = kstrtoul(tok, 16,
> +			       (unsigned long *)&l->cbm2[input_domain_num]);
> +		if (ret)
> +			return ret;
> +
> +		if (!cbm_validate(l->cbm2[input_domain_num], level))
> +			return -EINVAL;

So you have 3 copies of the same sequence now. At other places you split
out the most tiniest stuff into a gazillion of helper functions ...

Just create a parser helper and call it for any of those types. So the
whole thing boils down to:

static int parse_cbm_token(char *tok, u64 *cbm, int level)
{
	unsigned long data;
	int ret;

	ret = kstrtoul(tok, 16, &data);
	if (ret)
		return ret;
	if (!cbm_validate(data, level))
	   	return -EINVAL;
	*cbm = data;
	return 0;
}

static int parse_cbm(char *buf, struct cache_resource *cr, int domain,
		     int level)
{
	char *cbm1 = buf;
	int ret;

	if (cdp_enabled)
		cbm1 = strsep(&buf, ',');

	if (!cbm1 || !buf)
	   	return -EINVAL;

	ret = parse_cbm_token(cbm1, &cr->cbm[domain]);
	if (ret)
		return ret;

	if (cdp_enmabled)
		return parse_cbm_token(buf, &cr->cbm2[domain]);
	return 0;
}

Copy and paste is simpler than thinking, but the result is uglier and
harder to read.

> +static int get_cache_schema(char *buf, struct cache_resource *l, int level,
> +			 struct rdtgroup *rdtgrp)
> +{
> +	char *tok, *tok_cache_id;
> +	int ret;
> +	int domain_num;
> +	int input_domain_num;
> +	int len;
> +	unsigned int input_cache_id;
> +	unsigned int cid;
> +	unsigned int leaf;
> +
> +	if (!cat_enabled(level) && strcmp(buf, ";")) {
> +		pr_info("Disabled resource should have empty schema\n");

So an empty schema is a string which is != ";". Very interesting.

This enabled check here wants more thoughts. If a resource is disabled,
then the input line should be simply ignored. Otherwise you need to rewrite
scripts, config files just because you disabled a particular resource.

> +		return -EINVAL;
> +	}
> +}
> +
> +enum {
> +	CURRENT_CLOSID,
> +	REUSED_OWN_CLOSID,
> +	REUSED_OTHER_CLOSID,
> +	NEW_CLOSID,
> +};

Another random enum in the middle of the code and at a place where it is
completely disjunct from its usage.

> +
> +/*
> + * Check if the reference counts are all ones in rdtgrp's domain.
> + */
> +static bool one_refcnt(struct rdtgroup *rdtgrp, int domain)

A really self explaining function name - NOT!

> +/*
> + * Go through all shared domains. Check if there is an existing closid
> + * in all rdtgroups that matches l3 cbms in the shared
> + * domain. If find one, reuse the closid. Otherwise, allocate a new one.
> + */
> +static int get_rdtgroup_resources(struct resources *resources_set,
> +				  struct rdtgroup *rdtgrp)
> +{
> +	struct cache_resource *l3;
> +	bool l3_cbm_found;
> +	struct list_head *l;
> +	struct rdtgroup *r;
> +	u64 cbm;
> +	int rdt_closid[MAX_CACHE_DOMAINS];
> +	int rdt_closid_type[MAX_CACHE_DOMAINS];

Have you ever checked what the stack foot print of this whole callchain is?
One of the callers has already a char array[1024] on the stack.....

> +	int domain;
> +	int closid;
> +	int ret;
> +
> +	l3 = resources_set->l3;
> +	memcpy(rdt_closid, rdtgrp->resource.closid,
> +	       shared_domain_num * sizeof(int));

Can you please seperate stuff with new lines ocassionally to make it
readable?

> +	for (domain = 0; domain < shared_domain_num; domain++) {
> +		if (rdtgrp->resource.valid) {
> +			/*
> +			 * If current rdtgrp is the only user of cbms in
> +			 * this domain, will replace the cbms with the input
> +			 * cbms and reuse its own closid.
> +			 */
> +			if (one_refcnt(rdtgrp, domain)) {
> +				closid = rdtgrp->resource.closid[domain];
> +				rdt_closid[domain] = closid;
> +				rdt_closid_type[domain] = REUSED_OWN_CLOSID;
> +				continue;
> +			}
> +
> +			l3_cbm_found = true;
> +
> +			if (cat_l3_enabled)
> +				l3_cbm_found = cbm_found(l3, rdtgrp, domain,
> +							 CACHE_LEVEL3);
> +
> +			/*
> +			 * If the cbms in this shared domain are already
> +			 * existing in current rdtgrp, record the closid
> +			 * and its type.
> +			 */
> +			if (l3_cbm_found) {
> +				closid = rdtgrp->resource.closid[domain];
> +				rdt_closid[domain] = closid;
> +				rdt_closid_type[domain] = CURRENT_CLOSID;
> +				continue;
> +			}

This is unreadable once more.

     		   	if (find_cbm(l3, rdtgrp, domain, CACHE_LEVEL3) {
				closid = rdtgrp->resource.closid[domain];
				rdt_closid[domain] = closid;
				rdt_closid_type[domain] = CURRENT_CLOSID;
				continue;
			}

That requires that find_cbm() - which is a way more intuitive name than
cbm_found() - returns false when cat_l3_enabled is false. Which is trivial
and obvious ...
			
> +		}
> +
> +		/*
> +		 * If the cbms are not found in this rdtgrp, search other
> +		 * rdtgroups and see if there are matched cbms.
> +		 */
> +		l3_cbm_found = cat_l3_enabled ? false : true;

What the heck?

		l3_cbm_found = !cat_l3_enabled;

Is too simple obviously.

Aside of that silly conditional: If cat_l3_enables is false then
l3_cbm_found is true.

> +		list_for_each(l, &rdtgroup_lists) {
> +			r = list_entry(l, struct rdtgroup, rdtgroup_list);
> +			if (r == rdtgrp || !r->resource.valid)
> +				continue;
> +
> +			if (cat_l3_enabled)
> +				l3_cbm_found = cbm_found(l3, r, domain,
> +							 CACHE_LEVEL3);

And because this path is never taken when cat_l3_enabled is false.
 
> +
> +			if (l3_cbm_found) {

We happily get the closid for something which is not enabled at all. What
is the logic here? I can't find any in this convoluted mess.

> +				/* Get the closid that matches l3 cbms.*/
> +				closid = r->resource.closid[domain];
> +				rdt_closid[domain] = closid;
> +				rdt_closid_type[domain] = REUSED_OTHER_CLOSID;
> +				break;
> +			}
> +		}
> +		if (!l3_cbm_found) {
> +			/*
> +			 * If no existing closid is found, allocate
> +			 * a new one.
> +			 */
> +			ret = closid_alloc(&closid, domain);
> +			if (ret)
> +				goto err;
> +			rdt_closid[domain] = closid;
> +			rdt_closid_type[domain] = NEW_CLOSID;
> +		}
> +	}

I really don't want to imagine how this might look like when you add L2
support and if you have code doing this please hide it in the poison
cabinet forever,

> +	/*
> +	 * Now all closid are ready in rdt_closid. Update rdtgrp's closid.
> +	 */
> +	for_each_cache_domain(domain, 0, shared_domain_num) {
> +		/*
> +		 * Nothing is changed if the same closid and same cbms were
> +		 * found in this rdtgrp's domain.
> +		 */
> +		if (rdt_closid_type[domain] == CURRENT_CLOSID)
> +			continue;
> +
> +		/*
> +		 * Put rdtgroup closid. No need to put the closid if we
> +		 * just change cbms and keep the closid (REUSED_OWN_CLOSID).
> +		 */
> +		if (rdtgrp->resource.valid &&
> +		    rdt_closid_type[domain] != REUSED_OWN_CLOSID) {
> +			/* Put old closid in this rdtgrp's domain if valid. */
> +			closid = rdtgrp->resource.closid[domain];
> +			closid_put(closid, domain);
> +		}
> +
> +		/*
> +		 * Replace the closid in this rdtgrp's domain with saved
> +		 * closid that was newly allocted (NEW_CLOSID), or found in
> +		 * another rdtgroup's domains (REUSED_CLOSID), or found in
> +		 * this rdtgrp (REUSED_OWN_CLOSID).
> +		 */
> +		closid = rdt_closid[domain];
> +		rdtgrp->resource.closid[domain] = closid;
> +
> +		/*
> +		 * Get the reused other rdtgroup's closid. No need to get the
> +		 * closid newly allocated (NEW_CLOSID) because it's been
> +		 * already got in closid_alloc(). And no need to get the closid
> +		 * for resued own closid (REUSED_OWN_CLOSID).
> +		 */
> +		if (rdt_closid_type[domain] == REUSED_OTHER_CLOSID)
> +			closid_get(closid, domain);
> +
> +		/*
> +		 * If the closid comes from a newly allocated closid
> +		 * (NEW_CLOSID), or found in this rdtgrp (REUSED_OWN_CLOSID),
> +		 * cbms for this closid will be updated in MSRs.
> +		 */
> +		if (rdt_closid_type[domain] == NEW_CLOSID ||
> +		    rdt_closid_type[domain] == REUSED_OWN_CLOSID) {
> +			/*
> +			 * Update cbm in cctable with the newly allocated
> +			 * closid.
> +			 */
> +			if (cat_l3_enabled) {
> +				int cpu;
> +				struct cpumask *mask;
> +				int dindex;
> +				int l3_domain = shared_domain[domain].l3_domain;
> +				int leaf = level_to_leaf(CACHE_LEVEL3);
> +
> +				cbm = l3->cbm[l3_domain];
> +				dindex = get_dcbm_table_index(closid);
> +				l3_cctable[l3_domain][dindex].cbm = cbm;
> +				if (cdp_enabled) {
> +					int iindex;
> +
> +					cbm = l3->cbm2[l3_domain];
> +					iindex = get_icbm_table_index(closid);
> +					l3_cctable[l3_domain][iindex].cbm = cbm;
> +				}
> +
> +				mask =
> +				&cache_domains[leaf].shared_cpu_map[l3_domain];
> +
> +				cpu = cpumask_first(mask);
> +				smp_call_function_single(cpu, cbm_update_l3_msr,
> +							 &closid, 1);

Again, why don't ypu split that out into a seperate function instead of
having the forth indentation level and random line breaks?

> +static void init_cache_resource(struct cache_resource *l)
> +{
> +	l->cbm = NULL;
> +	l->cbm2 = NULL;
> +	l->closid = NULL;
> +	l->refcnt = NULL;

memset ?

> +}
> +
> +static void free_cache_resource(struct cache_resource *l)
> +{
> +	kfree(l->cbm);
> +	kfree(l->cbm2);
> +	kfree(l->closid);
> +	kfree(l->refcnt);
> +}
> +
> +static int alloc_cache_resource(struct cache_resource *l, int level)
> +{
> +	int domain_num = get_domain_num(level);
> +
> +	l->cbm = kcalloc(domain_num, sizeof(*l->cbm), GFP_KERNEL);
> +	l->cbm2 = kcalloc(domain_num, sizeof(*l->cbm2), GFP_KERNEL);
> +	l->closid = kcalloc(domain_num, sizeof(*l->closid), GFP_KERNEL);
> +	l->refcnt = kcalloc(domain_num, sizeof(*l->refcnt), GFP_KERNEL);
> +	if (l->cbm && l->cbm2 && l->closid && l->refcnt)
> +		return 0;
> +
> +	return -ENOMEM;
> +}
> +
> +/*
> + * This function digests schemata given in text buf. If the schemata are in
> + * right format and there is enough closid, input the schemata in rdtgrp
> + * and update resource cctables.
> + *
> + * Inputs:
> + *	buf: string buffer containing schemata
> + *	rdtgrp: current rdtgroup holding schemata.
> + *
> + * Return:
> + *	0 on success or error code.
> + */
> +static int get_resources(char *buf, struct rdtgroup *rdtgrp)
> +{
> +	char *resources[RESOURCE_NUM];
> +	struct cache_resource l3;
> +	struct resources resources_set;
> +	int ret;
> +	char *resources_block;
> +	int i;
> +	int size = strlen(buf) + 1;
> +
> +	resources_block = kcalloc(RESOURCE_NUM, size, GFP_KERNEL);
> +	if (!resources_block)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < RESOURCE_NUM; i++)
> +		resources[i] = (char *)(resources_block + i * size);

This is a recurring scheme in your code. Allocating a runtime sized array
and initializing pointers.

Darn, instead of open coding this in several places can't you just make a
single function which does exactly that?

> +	ret = divide_resources(buf, resources);
> +	if (ret) {
> +		kfree(resources_block);
> +		return -EINVAL;
> +	}
> +
> +	init_cache_resource(&l3);
> +
> +	if (cat_l3_enabled) {
> +		ret = alloc_cache_resource(&l3, CACHE_LEVEL3);
> +		if (ret)
> +			goto out;
> +
> +		ret = get_cache_schema(resources[RESOURCE_L3], &l3,
> +				       CACHE_LEVEL3, rdtgrp);
> +		if (ret)
> +			goto out;
> +
> +		resources_set.l3 = &l3;
> +	} else
> +		resources_set.l3 = NULL;



> +
> +	ret = get_rdtgroup_resources(&resources_set, rdtgrp);
> +
> +out:
> +	kfree(resources_block);
> +	free_cache_resource(&l3);
> +
> +	return ret;
> +}
> +
> +static void gen_cache_prefix(char *buf, int level)
> +{
> +	sprintf(buf, "L%1d:", level == CACHE_LEVEL3 ? 3 : 2);
> +}
> +
> +static int get_cache_id(int domain, int level)
> +{
> +	return cache_domains[level_to_leaf(level)].shared_cache_id[domain];
> +}
> +
> +static void gen_cache_buf(char *buf, int level)
> +{
> +	int domain;
> +	char buf1[32];
> +	int domain_num;
> +	u64 val;
> +
> +	gen_cache_prefix(buf, level);
> +
> +	domain_num = get_domain_num(level);
> +
> +	val = max_cbm(level);
> +
> +	for (domain = 0; domain < domain_num; domain++) {
> +		sprintf(buf1, "%d=%lx", get_cache_id(domain, level),
> +			(unsigned long)val);
> +		strcat(buf, buf1);

WTF?

	char *p = buf;

	p += sprintf(p, "....", ...);
	p += sprintf(p, "....", ...);
	p += sprintf(p, "....", ...);

Solves the same problem as this local buffer on the stack plus strcat().

> +/*
> + * Set up default schemata in a rdtgroup. All schemata in all resources are
> + * default values (all 1's) for all domains.
> + *
> + * Input: rdtgroup.
> + * Return: 0: successful
> + *	   non-0: error code
> + */
> +int get_default_resources(struct rdtgroup *rdtgrp)
> +{
> +	char schema[1024];

And that number is pulled out of thin air or what?

> +	int ret = 0;
> +
> +	if (cat_enabled(CACHE_LEVEL3)) {
> +		gen_cache_buf(schema, CACHE_LEVEL3);
> +
> +		if (strlen(schema)) {
> +			ret = get_resources(schema, rdtgrp);
> +			if (ret)
> +				return ret;
> +		}
> +		gen_cache_buf(rdtgrp->schema, CACHE_LEVEL3);
> +	}
> +
> +	return ret;
> +}
> +
> +ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
> +			char *buf, size_t nbytes, loff_t off)
> +{
> +	int ret = 0;
> +	struct rdtgroup *rdtgrp;
> +	char *schema;
> +
> +	rdtgrp = rdtgroup_kn_lock_live(of->kn);
> +	if (!rdtgrp)
> +		return -ENODEV;
> +
> +	schema = kzalloc(sizeof(char) * strlen(buf) + 1, GFP_KERNEL);
> +	if (!schema) {
> +		ret = -ENOMEM;
> +		goto out_unlock;
> +	}
> +
> +	memcpy(schema, buf, strlen(buf) + 1);

Open coding kstrdup() is indeed useful. and reevaluating strlen(buf) three
times in the same function is even more useful.

> +
> +	ret = get_resources(buf, rdtgrp);
> +	if (ret)
> +		goto out;
> +
> +	memcpy(rdtgrp->schema, schema, strlen(schema) + 1);

IIRC then the kernel has even strcpy() and strncpy for that matter.

Btw, what makes sure that strlen(schema) is < 1023 ?????

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]


#1478828 — [PATCH v2 31/33] Documentation/kernel-parameters: Add kernel parameter "resctrl" for CAT

From"Fenghua Yu" <fenghua.yu@intel.com>
Date2016-09-08 09:00 +0200
Subject[PATCH v2 31/33] Documentation/kernel-parameters: Add kernel parameter "resctrl" for CAT
Message-ID<sf1tw-2mB-43@gated-at.bofh.it>
In reply to#1478824
From: Fenghua Yu <fenghua.yu@intel.com>

Add kernel parameter "resctrl" for CAT L3:

resctrl=disable_cat_l3: disable CAT L3
resctrl=simulate_cat_l3: simulate CAT L3

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 Documentation/kernel-parameters.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index a4f4d69..1240a4f 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3692,6 +3692,19 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			Memory area to be used by remote processor image,
 			managed by CMA.
 
+	resctrl=	[X86] Resource control
+			Format:
+			[disable_cat_l3][,simulate_cat_l3]
+
+			disable_cat_l3  - Disable CAT L3. By default, CAT L3
+					  is enabled.
+
+			simulate_cat_l3 - Simulate CAT L3 on a machine that
+					  doesn't have the feature. In the
+					  simulation, max closid is 16 and
+					  max cbm lenghth is 20, and host
+					  machine's cache hierarchy is used.
+
 	rw		[KNL] Mount root device read-write on boot
 
 	S		[KNL] Run init in single mode
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]


#1479564 — Re: [PATCH v2 31/33] Documentation/kernel-parameters: Add kernel parameter "resctrl" for CAT

FromThomas Gleixner <tglx@linutronix.de>
Date2016-09-09 00:30 +0200
SubjectRe: [PATCH v2 31/33] Documentation/kernel-parameters: Add kernel parameter "resctrl" for CAT
Message-ID<sffZx-3f7-49@gated-at.bofh.it>
In reply to#1478828
On Thu, 8 Sep 2016, Fenghua Yu wrote:

> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> Add kernel parameter "resctrl" for CAT L3:

We add the fricking documentation for kernel parameters in the patch which introduces them and not an some random other place.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]


#1478829 — [PATCH v2 28/33] x86/intel_rdt_rdtgroup.c: Read and write cpus

From"Fenghua Yu" <fenghua.yu@intel.com>
Date2016-09-08 09:10 +0200
Subject[PATCH v2 28/33] x86/intel_rdt_rdtgroup.c: Read and write cpus
Message-ID<sf1Db-2F3-3@gated-at.bofh.it>
In reply to#1478824
From: Fenghua Yu <fenghua.yu@intel.com>

Normally each task is associated with one rdtgroup and we use the schema
for that rdtgroup whenever the task is running. The user can designate
some cpus to always use the same schema, regardless of which task is
running. To do that the user write a cpumask bit string to the "cpus"
file.

A cpu can only be listed in one rdtgroup. If the user specifies a cpu
that is currently assigned to a different rdtgroup, it is removed
from that rdtgroup.

See Documentation/x86/intel_rdt_ui.txt

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 92 ++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 71231ba..6c3161a 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -59,6 +59,10 @@ static struct dentry *rdt_mount(struct file_system_type *fs_type,
 			 void *data);
 static void rdt_kill_sb(struct super_block *sb);
 
+static int rdtgroup_cpus_show(struct seq_file *s, void *v);
+static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
+			char *buf, size_t nbytes, loff_t off);
+
 /* rdtgroup core interface files */
 static struct rftype rdtgroup_root_base_files[] = {
 	{
@@ -1189,3 +1193,91 @@ static void rdt_kill_sb(struct super_block *sb)
 
 	mutex_unlock(&rdtgroup_mutex);
 }
+
+static int rdtgroup_cpus_show(struct seq_file *s, void *v)
+{
+	struct kernfs_open_file *of = s->private;
+	struct rdtgroup *rdtgrp;
+
+	rdtgrp = rdtgroup_kn_lock_live(of->kn);
+	if (!rdtgrp)
+		return -ENODEV;
+
+	seq_printf(s, "%*pb\n", cpumask_pr_args(&rdtgrp->cpu_mask));
+	rdtgroup_kn_unlock(of->kn);
+
+	return 0;
+}
+
+static int cpus_validate(struct cpumask *cpumask, struct rdtgroup *rdtgrp)
+{
+	int old_cpumask_bit, new_cpumask_bit;
+	int cpu;
+
+	for_each_online_cpu(cpu) {
+		old_cpumask_bit = cpumask_test_cpu(cpu, &rdtgrp->cpu_mask);
+		new_cpumask_bit = cpumask_test_cpu(cpu, cpumask);
+		/* Cannot clear a "cpus" bit in a rdtgroup. */
+		if (old_cpumask_bit == 1 && new_cpumask_bit == 0)
+			return -EINVAL;
+	}
+
+	/* If a cpu is not online, cannot set it. */
+	for_each_cpu(cpu, cpumask) {
+		if (!cpu_online(cpu))
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
+			char *buf, size_t nbytes, loff_t off)
+{
+	struct rdtgroup *rdtgrp;
+	unsigned long bitmap[BITS_TO_LONGS(NR_CPUS)];
+	struct cpumask *cpumask;
+	int cpu;
+	struct list_head *l;
+	struct rdtgroup *r;
+	int ret = 0;
+
+	if (!buf)
+		return -EINVAL;
+
+	rdtgrp = rdtgroup_kn_lock_live(of->kn);
+	if (!rdtgrp)
+		return -ENODEV;
+
+	if (list_empty(&rdtgroup_lists)) {
+		ret = -EINVAL;
+		goto end;
+	}
+
+	ret = __bitmap_parse(buf, strlen(buf), 0, bitmap, nr_cpu_ids);
+	if (ret)
+		goto end;
+
+	cpumask = to_cpumask(bitmap);
+	ret = cpus_validate(cpumask, rdtgrp);
+	if (ret)
+		goto end;
+
+	list_for_each(l, &rdtgroup_lists) {
+		r = list_entry(l, struct rdtgroup, rdtgroup_list);
+		if (r == rdtgrp)
+			continue;
+
+		for_each_cpu_and(cpu, &r->cpu_mask, cpumask)
+			cpumask_clear_cpu(cpu, &r->cpu_mask);
+	}
+
+	cpumask_copy(&rdtgrp->cpu_mask, cpumask);
+	for_each_cpu(cpu, cpumask)
+		per_cpu(cpu_rdtgroup, cpu) = rdtgrp;
+
+end:
+	rdtgroup_kn_unlock(of->kn);
+
+	return ret ?: nbytes;
+}
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]


#1479490 — Re: [PATCH v2 28/33] x86/intel_rdt_rdtgroup.c: Read and write cpus

FromThomas Gleixner <tglx@linutronix.de>
Date2016-09-08 22:30 +0200
SubjectRe: [PATCH v2 28/33] x86/intel_rdt_rdtgroup.c: Read and write cpus
Message-ID<sfe7o-23T-21@gated-at.bofh.it>
In reply to#1478829
On Thu, 8 Sep 2016, Fenghua Yu wrote:

> Normally each task is associated with one rdtgroup and we use the schema
> for that rdtgroup whenever the task is running. The user can designate
> some cpus to always use the same schema, regardless of which task is
> running. To do that the user write a cpumask bit string to the "cpus"
> file.

Is that just a left over of the previous series or am I completely confused
by now?

> +static int cpus_validate(struct cpumask *cpumask, struct rdtgroup *rdtgrp)
> +{
> +	int old_cpumask_bit, new_cpumask_bit;
> +	int cpu;
> +
> +	for_each_online_cpu(cpu) {
> +		old_cpumask_bit = cpumask_test_cpu(cpu, &rdtgrp->cpu_mask);
> +		new_cpumask_bit = cpumask_test_cpu(cpu, cpumask);
> +		/* Cannot clear a "cpus" bit in a rdtgroup. */
> +		if (old_cpumask_bit == 1 && new_cpumask_bit == 0)
> +			return -EINVAL;
> +	}
> +
> +	/* If a cpu is not online, cannot set it. */
> +	for_each_cpu(cpu, cpumask) {
> +		if (!cpu_online(cpu))
> +			return -EINVAL;
> +	}

cpumask_intersects() exists for a reason. And how is this protected against
cpu hotplug?

> +	list_for_each(l, &rdtgroup_lists) {
> +		r = list_entry(l, struct rdtgroup, rdtgroup_list);
> +		if (r == rdtgrp)
> +			continue;
> +
> +		for_each_cpu_and(cpu, &r->cpu_mask, cpumask)
> +			cpumask_clear_cpu(cpu, &r->cpu_mask);

This code clearly predates the invention of cpumask_andnot()

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]


#1478830 — [PATCH v2 24/33] x86/intel_rdt_rdtgroup.c: Create info directory

From"Fenghua Yu" <fenghua.yu@intel.com>
Date2016-09-08 09:10 +0200
Subject[PATCH v2 24/33] x86/intel_rdt_rdtgroup.c: Create info directory
Message-ID<sf1Db-2F3-1@gated-at.bofh.it>
In reply to#1478824
From: Fenghua Yu <fenghua.yu@intel.com>

During boot time, the "info" directory is set up under resctrl root.
it contains one "info" file and one resource specific directory
if the resource is enabled.

If L3 is enabled, "l3" sub-directory is created under the "info"
directory. There are three l3 specific info files under it:
max_closid, max_cbm_len, and domain_to_cache_id.

The "info" directory is exposed to user after resctrl is mounted.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/intel_rdt_rdtgroup.h |   7 +
 arch/x86/kernel/cpu/intel_rdt.c           |   2 +
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c  | 958 ++++++++++++++++++++++++++++++
 3 files changed, 967 insertions(+)
 create mode 100644 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c

diff --git a/arch/x86/include/asm/intel_rdt_rdtgroup.h b/arch/x86/include/asm/intel_rdt_rdtgroup.h
index 3703964..92208a2 100644
--- a/arch/x86/include/asm/intel_rdt_rdtgroup.h
+++ b/arch/x86/include/asm/intel_rdt_rdtgroup.h
@@ -7,8 +7,15 @@
 #include <linux/kernfs.h>
 #include <asm/intel_rdt.h>
 
+/* Defined in intel_rdt_rdtgroup.c.*/
+extern int __init rdtgroup_init(void);
 extern void rdtgroup_exit(struct task_struct *tsk);
 
+/* Defined in intel_rdt.c. */
+extern struct list_head rdtgroup_lists;
+extern struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn);
+extern void rdtgroup_kn_unlock(struct kernfs_node *kn);
+
 /* cftype->flags */
 enum {
 	RFTYPE_WORLD_WRITABLE = (1 << 4),/* (DON'T USE FOR NEW FILES) S_IWUGO */
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index f7c728b..6c3df9e 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -790,6 +790,8 @@ static int __init intel_rdt_late_init(void)
 	if (ret < 0)
 		return ret;
 
+	rdtgroup_init();
+
 	pr_info("Intel cache allocation enabled\n");
 	if (cpu_has(c, X86_FEATURE_CDP_L3))
 		pr_info("Intel code data prioritization detected\n");
diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
new file mode 100644
index 0000000..7842194
--- /dev/null
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -0,0 +1,958 @@
+/*
+ * Resource Director Technology(RDT)
+ * - User interface for Resource Alloction in RDT.
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * 2016 Written by
+ *    Fenghua Yu <fenghua.yu@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * More information about RDT be found in the Intel (R) x86 Architecture
+ * Software Developer Manual.
+ */
+#include <linux/cred.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/init_task.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/magic.h>
+#include <linux/mm.h>
+#include <linux/mutex.h>
+#include <linux/mount.h>
+#include <linux/pagemap.h>
+#include <linux/proc_fs.h>
+#include <linux/rcupdate.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+#include <linux/pid_namespace.h>
+#include <linux/vmalloc.h>
+#include <linux/atomic.h>
+#include <linux/cpumask.h>
+#include <linux/cacheinfo.h>
+#include <asm/intel_rdt_rdtgroup.h>
+#include <asm/intel_rdt.h>
+
+#define RDTGROUP_FILE_NAME_LEN	(MAX_RDTGROUP_TYPE_NAMELEN +	\
+				 MAX_RFTYPE_NAME + 2)
+
+static int rdt_info_show(struct seq_file *seq, void *v);
+static int rdt_max_closid_show(struct seq_file *seq, void *v);
+static int rdt_max_cbm_len_show(struct seq_file *seq, void *v);
+static int domain_to_cache_id_show(struct seq_file *seq, void *v);
+
+/* rdtgroup core interface files */
+static struct rftype rdtgroup_root_base_files[] = {
+	{
+		.name = "tasks",
+		.seq_show = rdtgroup_tasks_show,
+		.write = rdtgroup_tasks_write,
+	},
+	{
+		.name = "cpus",
+		.write = rdtgroup_cpus_write,
+		.seq_show = rdtgroup_cpus_show,
+	},
+	{
+		.name = "schemata",
+		.write = rdtgroup_schemata_write,
+		.seq_show = rdtgroup_schemata_show,
+	},
+};
+
+static struct rftype info_files[] = {
+	{
+		.name = "info",
+		.seq_show = rdt_info_show,
+	},
+};
+
+/* rdtgroup information files for one cache resource. */
+static struct rftype res_info_files[] = {
+	{
+		.name = "max_closid",
+		.seq_show = rdt_max_closid_show,
+	},
+	{
+		.name = "max_cbm_len",
+		.seq_show = rdt_max_cbm_len_show,
+	},
+	{
+		.name = "domain_to_cache_id",
+		.seq_show = domain_to_cache_id_show,
+	},
+};
+
+static struct rftype rdtgroup_partition_base_files[] = {
+	{
+		.name = "tasks",
+		.seq_show = rdtgroup_tasks_show,
+		.write = rdtgroup_tasks_write,
+	},
+	{
+		.name = "cpus",
+		.write = rdtgroup_cpus_write,
+		.seq_show = rdtgroup_cpus_show,
+	},
+	{
+		.name = "schemata",
+		.write = rdtgroup_schemata_write,
+		.seq_show = rdtgroup_schemata_show,
+	},
+};
+
+struct rdtgroup *root_rdtgrp;
+static struct rftype rdtgroup_partition_base_files[];
+struct cache_domain cache_domains[MAX_CACHE_LEAVES];
+/* The default hierarchy. */
+struct rdtgroup_root rdtgrp_dfl_root;
+static struct list_head rdtgroups;
+
+/*
+ * kernfs_root - find out the kernfs_root a kernfs_node belongs to
+ * @kn: kernfs_node of interest
+ *
+ * Return the kernfs_root @kn belongs to.
+ */
+static inline struct kernfs_root *get_kernfs_root(struct kernfs_node *kn)
+{
+	if (kn->parent)
+		kn = kn->parent;
+	return kn->dir.root;
+}
+
+/*
+ * rdtgroup_file_mode - deduce file mode of a control file
+ * @cft: the control file in question
+ *
+ * S_IRUGO for read, S_IWUSR for write.
+ */
+static umode_t rdtgroup_file_mode(const struct rftype *rft)
+{
+	umode_t mode = 0;
+
+	if (rft->read_u64 || rft->read_s64 || rft->seq_show)
+		mode |= S_IRUGO;
+
+	if (rft->write_u64 || rft->write_s64 || rft->write)
+		mode |= S_IWUSR;
+
+	return mode;
+}
+
+/* set uid and gid of rdtgroup dirs and files to that of the creator */
+static int rdtgroup_kn_set_ugid(struct kernfs_node *kn)
+{
+	struct iattr iattr = { .ia_valid = ATTR_UID | ATTR_GID,
+			       .ia_uid = current_fsuid(),
+			       .ia_gid = current_fsgid(), };
+
+	if (uid_eq(iattr.ia_uid, GLOBAL_ROOT_UID) &&
+	    gid_eq(iattr.ia_gid, GLOBAL_ROOT_GID))
+		return 0;
+
+	return kernfs_setattr(kn, &iattr);
+}
+
+static int rdtgroup_add_file(struct kernfs_node *parent_kn, struct rftype *rft)
+{
+	char name[RDTGROUP_FILE_NAME_LEN];
+	struct kernfs_node *kn;
+	struct lock_class_key *key = NULL;
+	int ret;
+
+	strncpy(name, rft->name, RDTGROUP_FILE_NAME_LEN);
+	kn = __kernfs_create_file(parent_kn, name, rdtgroup_file_mode(rft),
+				  0, rft->kf_ops, rft, NULL, key);
+	if (IS_ERR(kn))
+		return PTR_ERR(kn);
+
+	ret = rdtgroup_kn_set_ugid(kn);
+	if (ret) {
+		kernfs_remove(kn);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void rdtgroup_rm_file(struct kernfs_node *kn, const struct rftype *rft)
+{
+	char name[RDTGROUP_FILE_NAME_LEN];
+
+	strncpy(name, rft->name, RDTGROUP_FILE_NAME_LEN);
+	kernfs_remove_by_name(kn, name);
+}
+
+static void rdtgroup_rm_files(struct kernfs_node *kn, struct rftype *rft,
+			      const struct rftype *end)
+{
+	for (; rft != end; rft++)
+		rdtgroup_rm_file(kn, rft);
+}
+
+static int rdtgroup_add_files(struct kernfs_node *kn, struct rftype *rfts,
+			      const struct rftype *end)
+{
+	struct rftype *rft;
+	int ret;
+
+	lockdep_assert_held(&rdtgroup_mutex);
+
+	for (rft = rfts; rft != end; rft++) {
+		ret = rdtgroup_add_file(kn, rft);
+		if (ret) {
+			pr_warn("%s: failed to add %s, err=%d\n",
+				__func__, rft->name, ret);
+			rdtgroup_rm_files(kn, rft, end);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Get resource type from name in kernfs_node. This can be extended to
+ * multi-resources (e.g. L2). Right now simply return RESOURCE_L3 because
+ * we only have L3 support.
+ */
+static enum resource_type get_kn_res_type(struct kernfs_node *kn)
+{
+	return RESOURCE_L3;
+}
+
+static int rdt_max_closid_show(struct seq_file *seq, void *v)
+{
+	struct kernfs_open_file *of = seq->private;
+
+	switch (get_kn_res_type(of->kn)) {
+	case RESOURCE_L3:
+		seq_printf(seq, "%d\n",
+			boot_cpu_data.x86_l3_max_closid);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int rdt_max_cbm_len_show(struct seq_file *seq, void *v)
+{
+	struct kernfs_open_file *of = seq->private;
+
+	switch (get_kn_res_type(of->kn)) {
+	case RESOURCE_L3:
+		seq_printf(seq, "%d\n",
+			boot_cpu_data.x86_l3_max_cbm_len);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int get_shared_domain(int domain, int level)
+{
+	int sd;
+
+	for_each_cache_domain(sd, 0, shared_domain_num) {
+		if (cat_l3_enabled && level == CACHE_LEVEL3) {
+			if (shared_domain[sd].l3_domain == domain)
+				return sd;
+		}
+	}
+
+	return -1;
+}
+
+static void rdt_info_show_cat(struct seq_file *seq, int level)
+{
+	int domain;
+	int domain_num = get_domain_num(level);
+	int closid;
+	u64 cbm;
+	struct clos_cbm_table **cctable;
+	int maxid;
+	int shared_domain;
+	int cnt;
+
+	if (level == CACHE_LEVEL3)
+		cctable = l3_cctable;
+	else
+		return;
+
+	maxid = cconfig.max_closid;
+	for (domain = 0; domain < domain_num; domain++) {
+		seq_printf(seq, "domain %d:\n", domain);
+		shared_domain = get_shared_domain(domain, level);
+		for (closid = 0; closid < maxid; closid++) {
+			int dindex, iindex;
+
+			if (test_bit(closid,
+			(unsigned long *)cconfig.closmap[shared_domain])) {
+				dindex = get_dcbm_table_index(closid);
+				cbm = cctable[domain][dindex].cbm;
+				cnt = cctable[domain][dindex].clos_refcnt;
+				seq_printf(seq, "cbm[%d]=%lx, refcnt=%d\n",
+					 dindex, (unsigned long)cbm, cnt);
+				if (cdp_enabled) {
+					iindex = get_icbm_table_index(closid);
+					cbm = cctable[domain][iindex].cbm;
+					cnt =
+					   cctable[domain][iindex].clos_refcnt;
+					seq_printf(seq,
+						   "cbm[%d]=%lx, refcnt=%d\n",
+						   iindex, (unsigned long)cbm,
+						   cnt);
+				}
+			} else {
+				cbm = max_cbm(level);
+				cnt = 0;
+				dindex = get_dcbm_table_index(closid);
+				seq_printf(seq, "cbm[%d]=%lx, refcnt=%d\n",
+					 dindex, (unsigned long)cbm, cnt);
+				if (cdp_enabled) {
+					iindex = get_icbm_table_index(closid);
+					seq_printf(seq,
+						 "cbm[%d]=%lx, refcnt=%d\n",
+						 iindex, (unsigned long)cbm,
+						 cnt);
+				}
+			}
+		}
+	}
+}
+
+static void show_shared_domain(struct seq_file *seq)
+{
+	int domain;
+
+	seq_puts(seq, "Shared domains:\n");
+
+	for_each_cache_domain(domain, 0, shared_domain_num) {
+		struct shared_domain *sd;
+
+		sd = &shared_domain[domain];
+		seq_printf(seq, "domain[%d]:", domain);
+		if (cat_enabled(CACHE_LEVEL3))
+			seq_printf(seq, "l3_domain=%d ", sd->l3_domain);
+		seq_printf(seq, "cpumask=%*pb\n",
+			   cpumask_pr_args(&sd->cpumask));
+	}
+}
+
+static int rdt_info_show(struct seq_file *seq, void *v)
+{
+	show_shared_domain(seq);
+
+	if (cat_l3_enabled) {
+		if (rdt_opts.verbose)
+			rdt_info_show_cat(seq, CACHE_LEVEL3);
+	}
+
+	seq_puts(seq, "\n");
+
+	return 0;
+}
+
+static int res_type_to_level(enum resource_type res_type, int *level)
+{
+	int ret = 0;
+
+	switch (res_type) {
+	case RESOURCE_L3:
+		*level = CACHE_LEVEL3;
+		break;
+	case RESOURCE_NUM:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int domain_to_cache_id_show(struct seq_file *seq, void *v)
+{
+	struct kernfs_open_file *of = seq->private;
+	enum resource_type res_type;
+	int domain;
+	int leaf;
+	int level = 0;
+	int ret;
+
+	res_type = (enum resource_type)of->kn->parent->priv;
+
+	ret = res_type_to_level(res_type, &level);
+	if (ret)
+		return 0;
+
+	leaf =	get_cache_leaf(level, 0);
+
+	for (domain = 0; domain < get_domain_num(level); domain++) {
+		unsigned int cid;
+
+		cid = cache_domains[leaf].shared_cache_id[domain];
+		seq_printf(seq, "%d:%d\n", domain, cid);
+	}
+
+	return 0;
+}
+
+static int rdtgroup_procs_write_permission(struct task_struct *task,
+					   struct kernfs_open_file *of)
+{
+	const struct cred *cred = current_cred();
+	const struct cred *tcred = get_task_cred(task);
+	int ret = 0;
+
+	/*
+	 * even if we're attaching all tasks in the thread group, we only
+	 * need to check permissions on one of them.
+	 */
+	if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
+	    !uid_eq(cred->euid, tcred->uid) &&
+	    !uid_eq(cred->euid, tcred->suid))
+		ret = -EPERM;
+
+	put_cred(tcred);
+	return ret;
+}
+
+static int info_populate_dir(struct kernfs_node *kn)
+{
+	struct rftype *rfts;
+
+	rfts = info_files;
+	return rdtgroup_add_files(kn, rfts, rfts + ARRAY_SIZE(info_files));
+}
+
+static int res_info_populate_dir(struct kernfs_node *kn)
+{
+	struct rftype *rfts;
+
+	rfts = res_info_files;
+	return rdtgroup_add_files(kn, rfts, rfts + ARRAY_SIZE(res_info_files));
+}
+
+static int rdtgroup_populate_dir(struct kernfs_node *kn)
+{
+	struct rftype *rfts;
+
+	rfts = rdtgroup_root_base_files;
+	return rdtgroup_add_files(kn, rfts,
+				  rfts + ARRAY_SIZE(rdtgroup_root_base_files));
+}
+
+static int rdtgroup_partition_populate_dir(struct kernfs_node *kn)
+{
+	struct rftype *rfts;
+
+	rfts = rdtgroup_partition_base_files;
+	return rdtgroup_add_files(kn, rfts,
+			rfts + ARRAY_SIZE(rdtgroup_partition_base_files));
+}
+
+LIST_HEAD(rdtgroup_lists);
+static void init_rdtgroup_root(struct rdtgroup_root *root)
+{
+	struct rdtgroup *rdtgrp = &root->rdtgrp;
+
+	INIT_LIST_HEAD(&rdtgrp->rdtgroup_list);
+	list_add_tail(&rdtgrp->rdtgroup_list, &rdtgroup_lists);
+	atomic_set(&root->nr_rdtgrps, 1);
+	rdtgrp->root = root;
+}
+
+static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops;
+struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn)
+{
+	struct rdtgroup *rdtgrp;
+
+	if (kernfs_type(kn) == KERNFS_DIR)
+		rdtgrp = kn->priv;
+	else
+		rdtgrp = kn->parent->priv;
+
+	kernfs_break_active_protection(kn);
+
+	mutex_lock(&rdtgroup_mutex);
+	/* Unlock if rdtgrp is dead. */
+	if (!rdtgrp)
+		rdtgroup_kn_unlock(kn);
+
+	return rdtgrp;
+}
+
+void rdtgroup_kn_unlock(struct kernfs_node *kn)
+{
+	mutex_unlock(&rdtgroup_mutex);
+
+	kernfs_unbreak_active_protection(kn);
+}
+
+static char *res_info_dir_name(enum resource_type res_type, char *name)
+{
+	switch (res_type) {
+	case RESOURCE_L3:
+		strncpy(name, "l3", RDTGROUP_FILE_NAME_LEN);
+		break;
+	default:
+		break;
+	}
+
+	return name;
+}
+
+static int create_res_info(enum resource_type res_type,
+			   struct kernfs_node *parent_kn)
+{
+	struct kernfs_node *kn;
+	char name[RDTGROUP_FILE_NAME_LEN];
+	int ret;
+
+	res_info_dir_name(res_type, name);
+	kn = kernfs_create_dir(parent_kn, name, parent_kn->mode, NULL);
+	if (IS_ERR(kn)) {
+		ret = PTR_ERR(kn);
+		goto out;
+	}
+
+	/*
+	 * This extra ref will be put in kernfs_remove() and guarantees
+	 * that @rdtgrp->kn is always accessible.
+	 */
+	kernfs_get(kn);
+
+	ret = rdtgroup_kn_set_ugid(kn);
+	if (ret)
+		goto out_destroy;
+
+	ret = res_info_populate_dir(kn);
+	if (ret)
+		goto out_destroy;
+
+	kernfs_activate(kn);
+
+	ret = 0;
+	goto out;
+
+out_destroy:
+	kernfs_remove(kn);
+out:
+	return ret;
+
+}
+
+static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn,
+				    const char *name)
+{
+	struct kernfs_node *kn;
+	int ret;
+
+	if (parent_kn != root_rdtgrp->kn)
+		return -EPERM;
+
+	/* create the directory */
+	kn = kernfs_create_dir(parent_kn, "info", parent_kn->mode, root_rdtgrp);
+	if (IS_ERR(kn)) {
+		ret = PTR_ERR(kn);
+		goto out;
+	}
+
+	ret = info_populate_dir(kn);
+	if (ret)
+		goto out_destroy;
+
+	if (cat_enabled(CACHE_LEVEL3))
+		create_res_info(RESOURCE_L3, kn);
+
+	/*
+	 * This extra ref will be put in kernfs_remove() and guarantees
+	 * that @rdtgrp->kn is always accessible.
+	 */
+	kernfs_get(kn);
+
+	ret = rdtgroup_kn_set_ugid(kn);
+	if (ret)
+		goto out_destroy;
+
+	kernfs_activate(kn);
+
+	ret = 0;
+	goto out;
+
+out_destroy:
+	kernfs_remove(kn);
+out:
+	return ret;
+}
+
+static int rdtgroup_setup_root(struct rdtgroup_root *root,
+			       unsigned long ss_mask)
+{
+	int ret;
+
+	root_rdtgrp = &root->rdtgrp;
+
+	lockdep_assert_held(&rdtgroup_mutex);
+
+	root->kf_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
+					   KERNFS_ROOT_CREATE_DEACTIVATED,
+					   root_rdtgrp);
+	if (IS_ERR(root->kf_root)) {
+		ret = PTR_ERR(root->kf_root);
+		goto out;
+	}
+	root_rdtgrp->kn = root->kf_root->kn;
+
+	ret = rdtgroup_populate_dir(root->kf_root->kn);
+	if (ret)
+		goto destroy_root;
+
+	rdtgroup_create_info_dir(root->kf_root->kn, "info_dir");
+
+	/*
+	 * Link the root rdtgroup in this hierarchy into all the css_set
+	 * objects.
+	 */
+	WARN_ON(atomic_read(&root->nr_rdtgrps) != 1);
+
+	kernfs_activate(root_rdtgrp->kn);
+	ret = 0;
+	goto out;
+
+destroy_root:
+	kernfs_destroy_root(root->kf_root);
+	root->kf_root = NULL;
+out:
+	return ret;
+}
+
+static int get_shared_cache_id(int cpu, int level)
+{
+	struct cpuinfo_x86 *c;
+	int index_msb;
+	struct cpu_cacheinfo *this_cpu_ci;
+	struct cacheinfo *this_leaf;
+
+	this_cpu_ci = get_cpu_cacheinfo(cpu);
+
+	this_leaf = this_cpu_ci->info_list + level_to_leaf(level);
+	return this_leaf->id;
+	return c->apicid >> index_msb;
+}
+
+static void init_cache_domain(int cpu, int leaf)
+{
+	struct cpu_cacheinfo *this_cpu_ci;
+	struct cpumask *mask;
+	unsigned int level;
+	struct cacheinfo *this_leaf;
+	int domain;
+
+	this_cpu_ci = get_cpu_cacheinfo(cpu);
+	this_leaf = this_cpu_ci->info_list + leaf;
+	cache_domains[leaf].level = this_leaf->level;
+	mask = &this_leaf->shared_cpu_map;
+	for (domain = 0; domain < MAX_CACHE_DOMAINS; domain++) {
+		if (cpumask_test_cpu(cpu,
+			&cache_domains[leaf].shared_cpu_map[domain]))
+			return;
+	}
+	if (domain == MAX_CACHE_DOMAINS) {
+		domain = cache_domains[leaf].max_cache_domains_num++;
+
+		cache_domains[leaf].shared_cpu_map[domain] = *mask;
+
+		level = cache_domains[leaf].level;
+		cache_domains[leaf].shared_cache_id[domain] =
+			get_shared_cache_id(cpu, level);
+	}
+}
+
+static __init void init_cache_domains(void)
+{
+	int cpu;
+	int leaf;
+
+	for (leaf = 0; leaf < get_cpu_cacheinfo(0)->num_leaves; leaf++) {
+		for_each_online_cpu(cpu)
+			init_cache_domain(cpu, leaf);
+	}
+}
+
+void rdtgroup_exit(struct task_struct *tsk)
+{
+
+	if (!list_empty(&tsk->rg_list)) {
+		struct rdtgroup *rdtgrp = tsk->rdtgroup;
+
+		list_del_init(&tsk->rg_list);
+		tsk->rdtgroup = NULL;
+		atomic_dec(&rdtgrp->refcount);
+	}
+}
+
+static void rdtgroup_destroy_locked(struct rdtgroup *rdtgrp)
+	__releases(&rdtgroup_mutex) __acquires(&rdtgroup_mutex)
+{
+	int shared_domain;
+	int closid;
+
+	lockdep_assert_held(&rdtgroup_mutex);
+
+	/* free closid occupied by this rdtgroup. */
+	for_each_cache_domain(shared_domain, 0, shared_domain_num) {
+		closid = rdtgrp->resource.closid[shared_domain];
+		closid_put(closid, shared_domain);
+	}
+
+	list_del_init(&rdtgrp->rdtgroup_list);
+
+	/*
+	 * Remove @rdtgrp directory along with the base files.  @rdtgrp has an
+	 * extra ref on its kn.
+	 */
+	kernfs_remove(rdtgrp->kn);
+}
+
+static int
+rdtgroup_move_task_all(struct rdtgroup *src_rdtgrp, struct rdtgroup *dst_rdtgrp)
+{
+	struct list_head *tasks;
+
+	tasks = &src_rdtgrp->pset.tasks;
+	while (!list_empty(tasks)) {
+		struct task_struct *tsk;
+		struct list_head *pos;
+		pid_t pid;
+		int ret;
+
+		pos = tasks->next;
+		tsk = list_entry(pos, struct task_struct, rg_list);
+		pid = tsk->pid;
+		ret = rdtgroup_move_task(pid, dst_rdtgrp, false, NULL);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * Forcibly remove all of subdirectories under root.
+ */
+static void rmdir_all_sub(void)
+{
+	struct rdtgroup *rdtgrp;
+	int cpu;
+	struct list_head *l;
+	struct task_struct *p;
+
+	/* Move all tasks from sub rdtgroups to default */
+	rcu_read_lock();
+	for_each_process(p) {
+		if (p->rdtgroup)
+			p->rdtgroup = NULL;
+	}
+	rcu_read_unlock();
+
+	while (!list_is_last(&root_rdtgrp->rdtgroup_list, &rdtgroup_lists)) {
+		l = rdtgroup_lists.next;
+		if (l == &root_rdtgrp->rdtgroup_list)
+			l = l->next;
+
+		rdtgrp = list_entry(l, struct rdtgroup, rdtgroup_list);
+		if (rdtgrp == root_rdtgrp)
+			continue;
+
+		for_each_cpu(cpu, &rdtgrp->cpu_mask)
+			per_cpu(cpu_rdtgroup, cpu) = root_rdtgrp;
+
+		rdtgroup_destroy_locked(rdtgrp);
+	}
+}
+
+static int parse_rdtgroupfs_options(char *data)
+{
+	char *token, *o = data;
+	int nr_opts = 0;
+
+	while ((token = strsep(&o, ",")) != NULL) {
+		nr_opts++;
+
+		if (!*token)
+			return -EINVAL;
+		if (!strcmp(token, "cdp")) {
+			/* Enable CDP */
+			rdt_opts.cdp_enabled = true;
+			continue;
+		}
+		if (!strcmp(token, "verbose")) {
+			rdt_opts.verbose = true;
+			continue;
+		}
+	}
+
+	return 0;
+}
+
+static void release_root_closid(void)
+{
+	int domain;
+	int closid;
+
+	if (!root_rdtgrp->resource.valid)
+		return;
+
+	for_each_cache_domain(domain, 0, shared_domain_num) {
+		/* Put closid in root rdtgrp's domain if valid. */
+		closid = root_rdtgrp->resource.closid[domain];
+		closid_put(closid, domain);
+	}
+}
+
+static ssize_t rdtgroup_file_write(struct kernfs_open_file *of, char *buf,
+				 size_t nbytes, loff_t off)
+{
+	struct rftype *rft = of->kn->priv;
+
+	if (rft->write)
+		return rft->write(of, buf, nbytes, off);
+
+	return -EINVAL;
+}
+
+static void *rdtgroup_seqfile_start(struct seq_file *seq, loff_t *ppos)
+{
+	return seq_rft(seq)->seq_start(seq, ppos);
+}
+
+static void *rdtgroup_seqfile_next(struct seq_file *seq, void *v, loff_t *ppos)
+{
+	return seq_rft(seq)->seq_next(seq, v, ppos);
+}
+
+static void rdtgroup_seqfile_stop(struct seq_file *seq, void *v)
+{
+	seq_rft(seq)->seq_stop(seq, v);
+}
+
+static int rdtgroup_seqfile_show(struct seq_file *m, void *arg)
+{
+	struct rftype *rft = seq_rft(m);
+
+	if (rft->seq_show)
+		return rft->seq_show(m, arg);
+	return 0;
+}
+
+static struct kernfs_ops rdtgroup_kf_ops = {
+	.atomic_write_len	= PAGE_SIZE,
+	.write			= rdtgroup_file_write,
+	.seq_start		= rdtgroup_seqfile_start,
+	.seq_next		= rdtgroup_seqfile_next,
+	.seq_stop		= rdtgroup_seqfile_stop,
+	.seq_show		= rdtgroup_seqfile_show,
+};
+
+static struct kernfs_ops rdtgroup_kf_single_ops = {
+	.atomic_write_len	= PAGE_SIZE,
+	.write			= rdtgroup_file_write,
+	.seq_show		= rdtgroup_seqfile_show,
+};
+
+static void rdtgroup_exit_rftypes(struct rftype *rfts)
+{
+	struct rftype *rft;
+
+	for (rft = rfts; rft->name[0] != '\0'; rft++) {
+		/* free copy for custom atomic_write_len, see init_cftypes() */
+		if (rft->max_write_len && rft->max_write_len != PAGE_SIZE)
+			kfree(rft->kf_ops);
+		rft->kf_ops = NULL;
+
+		/* revert flags set by rdtgroup core while adding @cfts */
+		rft->flags &= ~(__RFTYPE_ONLY_ON_DFL | __RFTYPE_NOT_ON_DFL);
+	}
+}
+
+static int rdtgroup_init_rftypes(struct rftype *rfts)
+{
+	struct rftype *rft;
+
+	for (rft = rfts; rft->name[0] != '\0'; rft++) {
+		struct kernfs_ops *kf_ops;
+
+		if (rft->seq_start)
+			kf_ops = &rdtgroup_kf_ops;
+		else
+			kf_ops = &rdtgroup_kf_single_ops;
+
+		/*
+		 * Ugh... if @cft wants a custom max_write_len, we need to
+		 * make a copy of kf_ops to set its atomic_write_len.
+		 */
+		if (rft->max_write_len && rft->max_write_len != PAGE_SIZE) {
+			kf_ops = kmemdup(kf_ops, sizeof(*kf_ops), GFP_KERNEL);
+			if (!kf_ops) {
+				rdtgroup_exit_rftypes(rfts);
+				return -ENOMEM;
+			}
+			kf_ops->atomic_write_len = rft->max_write_len;
+		}
+
+		rft->kf_ops = kf_ops;
+	}
+
+	return 0;
+}
+
+/*
+ * rdtgroup_init - rdtgroup initialization
+ *
+ * Register rdtgroup filesystem, and initialize any subsystems that didn't
+ * request early init.
+ */
+int __init rdtgroup_init(void)
+{
+	int cpu;
+
+	WARN_ON(rdtgroup_init_rftypes(rdtgroup_root_base_files));
+
+	WARN_ON(rdtgroup_init_rftypes(res_info_files));
+	WARN_ON(rdtgroup_init_rftypes(info_files));
+
+	WARN_ON(rdtgroup_init_rftypes(rdtgroup_partition_base_files));
+	mutex_lock(&rdtgroup_mutex);
+
+	init_rdtgroup_root(&rdtgrp_dfl_root);
+	WARN_ON(rdtgroup_setup_root(&rdtgrp_dfl_root, 0));
+
+	mutex_unlock(&rdtgroup_mutex);
+
+	WARN_ON(sysfs_create_mount_point(fs_kobj, "resctrl"));
+	WARN_ON(register_filesystem(&rdt_fs_type));
+	init_cache_domains();
+
+	INIT_LIST_HEAD(&rdtgroups);
+
+	for_each_online_cpu(cpu)
+		per_cpu(cpu_rdtgroup, cpu) = root_rdtgrp;
+
+	return 0;
+}
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]


#1479356 — Re: [PATCH v2 24/33] x86/intel_rdt_rdtgroup.c: Create info directory

FromThomas Gleixner <tglx@linutronix.de>
Date2016-09-08 18:10 +0200
SubjectRe: [PATCH v2 24/33] x86/intel_rdt_rdtgroup.c: Create info directory
Message-ID<sfa3M-7ZV-15@gated-at.bofh.it>
In reply to#1478830
On Thu, 8 Sep 2016, Fenghua Yu wrote:
> +/*
> + * kernfs_root - find out the kernfs_root a kernfs_node belongs to
> + * @kn: kernfs_node of interest
> + *
> + * Return the kernfs_root @kn belongs to.
> + */
> +static inline struct kernfs_root *get_kernfs_root(struct kernfs_node *kn)
> +{
> +	if (kn->parent)
> +		kn = kn->parent;

So this is guaranteed to have a single nesting?

> +	return kn->dir.root;
> +}
> +
> +/*
> + * rdtgroup_file_mode - deduce file mode of a control file
> + * @cft: the control file in question
> + *
> + * S_IRUGO for read, S_IWUSR for write.
> + */
> +static umode_t rdtgroup_file_mode(const struct rftype *rft)
> +{
> +	umode_t mode = 0;
> +
> +	if (rft->read_u64 || rft->read_s64 || rft->seq_show)
> +		mode |= S_IRUGO;
> +
> +	if (rft->write_u64 || rft->write_s64 || rft->write)
> +		mode |= S_IWUSR;

Why don't you store the mode in rtftype instead of evaluating it at
runtime?

Aside of that [read|write]_[s|u]64 are nowhere used in this whole patch
series, but take plenty of storage and line space for nothing.

> +static int rdtgroup_add_files(struct kernfs_node *kn, struct rftype *rfts,
> +			      const struct rftype *end)
> +{
> +	struct rftype *rft;
> +	int ret;
> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	for (rft = rfts; rft != end; rft++) {
> +		ret = rdtgroup_add_file(kn, rft);
> +		if (ret) {
> +			pr_warn("%s: failed to add %s, err=%d\n",
> +				__func__, rft->name, ret);
> +			rdtgroup_rm_files(kn, rft, end);

So we remove the file which failed to be added along with those which we
have not been added yet.

		    rdtgroup_rm_files(kn, rfts, rft);

Might be more correct, but I might be wrong as usual.

> +/*
> + * Get resource type from name in kernfs_node. This can be extended to
> + * multi-resources (e.g. L2). Right now simply return RESOURCE_L3 because
> + * we only have L3 support.

That's crap. If you know that you have seperate types then spend the time
to implement the storage instead of documenting your lazy/sloppyness.

> + */
> +static enum resource_type get_kn_res_type(struct kernfs_node *kn)
> +{
> +	return RESOURCE_L3;
> +}
> +
> +static int rdt_max_closid_show(struct seq_file *seq, void *v)
> +{
> +	struct kernfs_open_file *of = seq->private;
> +
> +	switch (get_kn_res_type(of->kn)) {
> +	case RESOURCE_L3:
> +		seq_printf(seq, "%d\n",
> +			boot_cpu_data.x86_l3_max_closid);

x86_l3_max_closid is u16 ..... %u ?

And that line break is required because the line is

		seq_printf(seq, "%d\n",	boot_cpu_data.x86_l3_max_closid);

exactly 73 characters long ....

> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rdt_max_cbm_len_show(struct seq_file *seq, void *v)
> +{
> +	struct kernfs_open_file *of = seq->private;
> +
> +	switch (get_kn_res_type(of->kn)) {
> +	case RESOURCE_L3:
> +		seq_printf(seq, "%d\n",
> +			boot_cpu_data.x86_l3_max_cbm_len);

Ditto

> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}

> +static void rdt_info_show_cat(struct seq_file *seq, int level)
> +{
> +	int domain;
> +	int domain_num = get_domain_num(level);
> +	int closid;
> +	u64 cbm;
> +	struct clos_cbm_table **cctable;
> +	int maxid;
> +	int shared_domain;
> +	int cnt;

Soon you occupy half of the screen.

> +	if (level == CACHE_LEVEL3)
> +		cctable = l3_cctable;
> +	else
> +		return;
> +
> +	maxid = cconfig.max_closid;
> +	for (domain = 0; domain < domain_num; domain++) {
> +		seq_printf(seq, "domain %d:\n", domain);
> +		shared_domain = get_shared_domain(domain, level);
> +		for (closid = 0; closid < maxid; closid++) {
> +			int dindex, iindex;
> +
> +			if (test_bit(closid,
> +			(unsigned long *)cconfig.closmap[shared_domain])) {
> +				dindex = get_dcbm_table_index(closid);
> +				cbm = cctable[domain][dindex].cbm;
> +				cnt = cctable[domain][dindex].clos_refcnt;
> +				seq_printf(seq, "cbm[%d]=%lx, refcnt=%d\n",
> +					 dindex, (unsigned long)cbm, cnt);
> +				if (cdp_enabled) {
> +					iindex = get_icbm_table_index(closid);
> +					cbm = cctable[domain][iindex].cbm;
> +					cnt =
> +					   cctable[domain][iindex].clos_refcnt;
> +					seq_printf(seq,
> +						   "cbm[%d]=%lx, refcnt=%d\n",
> +						   iindex, (unsigned long)cbm,
> +						   cnt);
> +				}
> +			} else {
> +				cbm = max_cbm(level);
> +				cnt = 0;
> +				dindex = get_dcbm_table_index(closid);
> +				seq_printf(seq, "cbm[%d]=%lx, refcnt=%d\n",
> +					 dindex, (unsigned long)cbm, cnt);
> +				if (cdp_enabled) {
> +					iindex = get_icbm_table_index(closid);
> +					seq_printf(seq,
> +						 "cbm[%d]=%lx, refcnt=%d\n",
> +						 iindex, (unsigned long)cbm,
> +						 cnt);
> +				}

This is completely unreadable. Split it into static functions ....

> +			}
> +		}
> +	}
> +}
> +
> +static void show_shared_domain(struct seq_file *seq)
> +{
> +	int domain;
> +
> +	seq_puts(seq, "Shared domains:\n");
> +
> +	for_each_cache_domain(domain, 0, shared_domain_num) {
> +		struct shared_domain *sd;
> +
> +		sd = &shared_domain[domain];
> +		seq_printf(seq, "domain[%d]:", domain);
> +		if (cat_enabled(CACHE_LEVEL3))
> +			seq_printf(seq, "l3_domain=%d ", sd->l3_domain);
> +		seq_printf(seq, "cpumask=%*pb\n",
> +			   cpumask_pr_args(&sd->cpumask));

What's the value of printing a cpu mask for something which is not enabled?

> +	}
> +}
> +
> +static int rdt_info_show(struct seq_file *seq, void *v)
> +{
> +	show_shared_domain(seq);
> +
> +	if (cat_l3_enabled) {
> +		if (rdt_opts.verbose)

Concatenate the conditionals into a single line please.

> +			rdt_info_show_cat(seq, CACHE_LEVEL3);
> +	}
> +
> +	seq_puts(seq, "\n");
> +
> +	return 0;
> +}
> +
> +static int res_type_to_level(enum resource_type res_type, int *level)
> +{
> +	int ret = 0;
> +
> +	switch (res_type) {
> +	case RESOURCE_L3:
> +		*level = CACHE_LEVEL3;
> +		break;
> +	case RESOURCE_NUM:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;

Groan. What's wrong with

static int res_type_to_level(type)
{
	switch (type) {
	case RESOURCE_L3: return CACHE_LEVEL3;
	case RESOURCE_NUM: return -EINVAL;
	}
}

and at the callsite you do:


> +}
> +
> +static int domain_to_cache_id_show(struct seq_file *seq, void *v)
> +{
> +	struct kernfs_open_file *of = seq->private;
> +	enum resource_type res_type;
> +	int domain;
> +	int leaf;
> +	int level = 0;
> +	int ret;
> +
> +	res_type = (enum resource_type)of->kn->parent->priv;
> +
> +	ret = res_type_to_level(res_type, &level);
> +	if (ret)
> +		return 0;

  	level = res_type_to_level(res_type);
	if (level < 0)
	   	return 0;

That gets rid of the initialization of level as well and becomes readable
source code. Hmm?

> +
> +	leaf =	get_cache_leaf(level, 0);

  	leafidx = cache_get_leaf_index(...);

I trip over this over and over and I can't get used to this misnomer.

> +
> +	for (domain = 0; domain < get_domain_num(level); domain++) {
> +		unsigned int cid;
> +
> +		cid = cache_domains[leaf].shared_cache_id[domain];
> +		seq_printf(seq, "%d:%d\n", domain, cid);

Proper print qualifiers are overrated....

> +static int info_populate_dir(struct kernfs_node *kn)
> +{
> +	struct rftype *rfts;
> +
> +	rfts = info_files;

	struct rftype *rfts = info_files;

> +	return rdtgroup_add_files(kn, rfts, rfts + ARRAY_SIZE(info_files));
> +}

> +static int rdtgroup_partition_populate_dir(struct kernfs_node *kn)

Has no user.

> +LIST_HEAD(rdtgroup_lists);

I told you before that globals or module static variable don't get defined
in the middle of the code and not being sticked to a function definition
w/o a space.

> +static void init_rdtgroup_root(struct rdtgroup_root *root)
> +{
> +	struct rdtgroup *rdtgrp = &root->rdtgrp;
> +
> +	INIT_LIST_HEAD(&rdtgrp->rdtgroup_list);
> +	list_add_tail(&rdtgrp->rdtgroup_list, &rdtgroup_lists);
> +	atomic_set(&root->nr_rdtgrps, 1);
> +	rdtgrp->root = root;

Yuck.

	grp = root->grp;
	init(grp);
	root->nr_grps = 1;
	grp->root = root;

Confused.

> +}
> +
> +static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops;
> +struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn)
> +{
> +	struct rdtgroup *rdtgrp;
> +
> +	if (kernfs_type(kn) == KERNFS_DIR)
> +		rdtgrp = kn->priv;
> +	else
> +		rdtgrp = kn->parent->priv;

So this again assumes that there is a single level of directories....

> +	kernfs_break_active_protection(kn);
> +
> +	mutex_lock(&rdtgroup_mutex);
> +	/* Unlock if rdtgrp is dead. */
> +	if (!rdtgrp)
> +		rdtgroup_kn_unlock(kn);
> +
> +	return rdtgrp;
> +}
> +
> +void rdtgroup_kn_unlock(struct kernfs_node *kn)
> +{
> +	mutex_unlock(&rdtgroup_mutex);
> +
> +	kernfs_unbreak_active_protection(kn);
> +}
> +
> +static char *res_info_dir_name(enum resource_type res_type, char *name)
> +{
> +	switch (res_type) {
> +	case RESOURCE_L3:
> +		strncpy(name, "l3", RDTGROUP_FILE_NAME_LEN);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return name;

What's the purpose of this return value if its ignored at the call site?

> +}
> +
> +static int create_res_info(enum resource_type res_type,
> +			   struct kernfs_node *parent_kn)
> +{
> +	struct kernfs_node *kn;
> +	char name[RDTGROUP_FILE_NAME_LEN];
> +	int ret;
> +
> +	res_info_dir_name(res_type, name);

So name contains random crap if res_type is not handled in res_info_dir_name().

> +	kn = kernfs_create_dir(parent_kn, name, parent_kn->mode, NULL);
> +	if (IS_ERR(kn)) {
> +		ret = PTR_ERR(kn);
> +		goto out;
> +	}
> +
> +	/*
> +	 * This extra ref will be put in kernfs_remove() and guarantees
> +	 * that @rdtgrp->kn is always accessible.
> +	 */
> +	kernfs_get(kn);
> +
> +	ret = rdtgroup_kn_set_ugid(kn);
> +	if (ret)
> +		goto out_destroy;
> +
> +	ret = res_info_populate_dir(kn);
> +	if (ret)
> +		goto out_destroy;
> +
> +	kernfs_activate(kn);
> +
> +	ret = 0;
> +	goto out;

Hell no.

> +
> +out_destroy:
> +	kernfs_remove(kn);
> +out:
> +	return ret;
> +
> +}
> +
> +static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn,
> +				    const char *name)
> +{
> +	struct kernfs_node *kn;
> +	int ret;
> +
> +	if (parent_kn != root_rdtgrp->kn)
> +		return -EPERM;
> +
> +	/* create the directory */
> +	kn = kernfs_create_dir(parent_kn, "info", parent_kn->mode, root_rdtgrp);
> +	if (IS_ERR(kn)) {
> +		ret = PTR_ERR(kn);
> +		goto out;
> +	}
> +
> +	ret = info_populate_dir(kn);
> +	if (ret)
> +		goto out_destroy;
> +
> +	if (cat_enabled(CACHE_LEVEL3))
> +		create_res_info(RESOURCE_L3, kn);
> +
> +	/*
> +	 * This extra ref will be put in kernfs_remove() and guarantees
> +	 * that @rdtgrp->kn is always accessible.
> +	 */
> +	kernfs_get(kn);
> +
> +	ret = rdtgroup_kn_set_ugid(kn);
> +	if (ret)
> +		goto out_destroy;
> +
> +	kernfs_activate(kn);
> +
> +	ret = 0;
> +	goto out;

Copy and paste .... sucks.

> +out_destroy:
> +	kernfs_remove(kn);
> +out:
> +	return ret;
> +}
> +
> +static int rdtgroup_setup_root(struct rdtgroup_root *root,
> +			       unsigned long ss_mask)
> +{
> +	int ret;
> +
> +	root_rdtgrp = &root->rdtgrp;
> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	root->kf_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
> +					   KERNFS_ROOT_CREATE_DEACTIVATED,
> +					   root_rdtgrp);
> +	if (IS_ERR(root->kf_root)) {
> +		ret = PTR_ERR(root->kf_root);
> +		goto out;
> +	}
> +	root_rdtgrp->kn = root->kf_root->kn;
> +
> +	ret = rdtgroup_populate_dir(root->kf_root->kn);
> +	if (ret)
> +		goto destroy_root;
> +
> +	rdtgroup_create_info_dir(root->kf_root->kn, "info_dir");
> +
> +	/*
> +	 * Link the root rdtgroup in this hierarchy into all the css_set

css_set objects ? Again: Copy and paste sucks, when done without brain
involvement.

> +	 * objects.
> +	 */
> +	WARN_ON(atomic_read(&root->nr_rdtgrps) != 1);
> +
> +	kernfs_activate(root_rdtgrp->kn);
> +	ret = 0;
> +	goto out;
> +
> +destroy_root:
> +	kernfs_destroy_root(root->kf_root);
> +	root->kf_root = NULL;
> +out:
> +	return ret;
> +}

> +static int get_shared_cache_id(int cpu, int level)
> +{
> +	struct cpuinfo_x86 *c;
> +	int index_msb;
> +	struct cpu_cacheinfo *this_cpu_ci;
> +	struct cacheinfo *this_leaf;
> +
> +	this_cpu_ci = get_cpu_cacheinfo(cpu);

Once more. this_cpu_ci is actively misleading.

> +
> +	this_leaf = this_cpu_ci->info_list + level_to_leaf(level);
> +	return this_leaf->id;
> +	return c->apicid >> index_msb;
> +}

> +static void init_cache_domain(int cpu, int leaf)
> +{
> +	struct cpu_cacheinfo *this_cpu_ci;
> +	struct cpumask *mask;
> +	unsigned int level;
> +	struct cacheinfo *this_leaf;
> +	int domain;
> +
> +	this_cpu_ci = get_cpu_cacheinfo(cpu);
> +	this_leaf = this_cpu_ci->info_list + leaf;
> +	cache_domains[leaf].level = this_leaf->level;
> +	mask = &this_leaf->shared_cpu_map;
> +	for (domain = 0; domain < MAX_CACHE_DOMAINS; domain++) {
> +		if (cpumask_test_cpu(cpu,
> +			&cache_domains[leaf].shared_cpu_map[domain]))
> +			return;
> +	}
> +	if (domain == MAX_CACHE_DOMAINS) {
> +		domain = cache_domains[leaf].max_cache_domains_num++;
> +
> +		cache_domains[leaf].shared_cpu_map[domain] = *mask;
> +
> +		level = cache_domains[leaf].level;
> +		cache_domains[leaf].shared_cache_id[domain] =
> +			get_shared_cache_id(cpu, level);

I've seen similar code in the other file. Why do we need two incarnations
of that? Can't we have a shared cache domain information storage where all
info is kept for both the control and the user space interface?

> +	}
> +}
> +
> +static __init void init_cache_domains(void)
> +{
> +	int cpu;
> +	int leaf;
> +
> +	for (leaf = 0; leaf < get_cpu_cacheinfo(0)->num_leaves; leaf++) {
> +		for_each_online_cpu(cpu)
> +			init_cache_domain(cpu, leaf);

What updates this stuff on hotplug?

> +	}
> +}
> +
> +void rdtgroup_exit(struct task_struct *tsk)
> +{
> +
> +	if (!list_empty(&tsk->rg_list)) {

I told you last time that rg_list is a misnomer ....

> +		struct rdtgroup *rdtgrp = tsk->rdtgroup;
> +
> +		list_del_init(&tsk->rg_list);
> +		tsk->rdtgroup = NULL;
> +		atomic_dec(&rdtgrp->refcount);

And there is still no sign of documentation on how that list is used and
protected.

> +	}
> +}
> +
> +static void rdtgroup_destroy_locked(struct rdtgroup *rdtgrp)
> +	__releases(&rdtgroup_mutex) __acquires(&rdtgroup_mutex)

Where?

> +{
> +	int shared_domain;
> +	int closid;
> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	/* free closid occupied by this rdtgroup. */
> +	for_each_cache_domain(shared_domain, 0, shared_domain_num) {
> +		closid = rdtgrp->resource.closid[shared_domain];
> +		closid_put(closid, shared_domain);
> +	}
> +
> +	list_del_init(&rdtgrp->rdtgroup_list);
> +
> +	/*
> +	 * Remove @rdtgrp directory along with the base files.  @rdtgrp has an
> +	 * extra ref on its kn.
> +	 */
> +	kernfs_remove(rdtgrp->kn);
> +}
> +
> +static int
> +rdtgroup_move_task_all(struct rdtgroup *src_rdtgrp, struct rdtgroup *dst_rdtgrp)
> +{
> +	struct list_head *tasks;
> +
> +	tasks = &src_rdtgrp->pset.tasks;
> +	while (!list_empty(tasks)) {

  list_for_each_entry_safe() ???

> +		struct task_struct *tsk;
> +		struct list_head *pos;
> +		pid_t pid;
> +		int ret;
> +
> +		pos = tasks->next;
> +		tsk = list_entry(pos, struct task_struct, rg_list);
> +		pid = tsk->pid;
> +		ret = rdtgroup_move_task(pid, dst_rdtgrp, false, NULL);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Forcibly remove all of subdirectories under root.
> + */
> +static void rmdir_all_sub(void)
> +{
> +	struct rdtgroup *rdtgrp;
> +	int cpu;
> +	struct list_head *l;
> +	struct task_struct *p;
> +
> +	/* Move all tasks from sub rdtgroups to default */
> +	rcu_read_lock();
> +	for_each_process(p) {
> +		if (p->rdtgroup)
> +			p->rdtgroup = NULL;
> +	}
> +	rcu_read_unlock();

And how is that protected against concurrent forks?

> +
> +	while (!list_is_last(&root_rdtgrp->rdtgroup_list, &rdtgroup_lists)) {
> +		l = rdtgroup_lists.next;
> +		if (l == &root_rdtgrp->rdtgroup_list)
> +			l = l->next;
> +
> +		rdtgrp = list_entry(l, struct rdtgroup, rdtgroup_list);
> +		if (rdtgrp == root_rdtgrp)
> +			continue;
> +
> +		for_each_cpu(cpu, &rdtgrp->cpu_mask)
> +			per_cpu(cpu_rdtgroup, cpu) = root_rdtgrp;
> +
> +		rdtgroup_destroy_locked(rdtgrp);
> +	}
> +}

> +static void *rdtgroup_seqfile_start(struct seq_file *seq, loff_t *ppos)
> +{
> +	return seq_rft(seq)->seq_start(seq, ppos);
> +}
> +
> +static void *rdtgroup_seqfile_next(struct seq_file *seq, void *v, loff_t *ppos)
> +{
> +	return seq_rft(seq)->seq_next(seq, v, ppos);
> +}
> +
> +static void rdtgroup_seqfile_stop(struct seq_file *seq, void *v)
> +{
> +	seq_rft(seq)->seq_stop(seq, v);
> +}
> +
> +static int rdtgroup_seqfile_show(struct seq_file *m, void *arg)
> +{
> +	struct rftype *rft = seq_rft(m);
> +
> +	if (rft->seq_show)
> +		return rft->seq_show(m, arg);
> +	return 0;
> +}
> +
> +static struct kernfs_ops rdtgroup_kf_ops = {
> +	.atomic_write_len	= PAGE_SIZE,
> +	.write			= rdtgroup_file_write,
> +	.seq_start		= rdtgroup_seqfile_start,
> +	.seq_next		= rdtgroup_seqfile_next,
> +	.seq_stop		= rdtgroup_seqfile_stop,
> +	.seq_show		= rdtgroup_seqfile_show,
> +};

And once more nothing uses this at all. So why is it there?

> +static struct kernfs_ops rdtgroup_kf_single_ops = {
> +	.atomic_write_len	= PAGE_SIZE,
> +	.write			= rdtgroup_file_write,
> +	.seq_show		= rdtgroup_seqfile_show,
> +};
> +
> +static void rdtgroup_exit_rftypes(struct rftype *rfts)
> +{
> +	struct rftype *rft;
> +
> +	for (rft = rfts; rft->name[0] != '\0'; rft++) {
> +		/* free copy for custom atomic_write_len, see init_cftypes() */
> +		if (rft->max_write_len && rft->max_write_len != PAGE_SIZE)
> +			kfree(rft->kf_ops);
> +		rft->kf_ops = NULL;
> +
> +		/* revert flags set by rdtgroup core while adding @cfts */
> +		rft->flags &= ~(__RFTYPE_ONLY_ON_DFL | __RFTYPE_NOT_ON_DFL);
> +	}
> +}
> +
> +static int rdtgroup_init_rftypes(struct rftype *rfts)
> +{
> +	struct rftype *rft;
> +
> +	for (rft = rfts; rft->name[0] != '\0'; rft++) {
> +		struct kernfs_ops *kf_ops;
> +
> +		if (rft->seq_start)
> +			kf_ops = &rdtgroup_kf_ops;
> +		else
> +			kf_ops = &rdtgroup_kf_single_ops;

Ditto.

> +
> +		/*
> +		 * Ugh... if @cft wants a custom max_write_len, we need to
> +		 * make a copy of kf_ops to set its atomic_write_len.
> +		 */
> +		if (rft->max_write_len && rft->max_write_len != PAGE_SIZE) {
> +			kf_ops = kmemdup(kf_ops, sizeof(*kf_ops), GFP_KERNEL);
> +			if (!kf_ops) {
> +				rdtgroup_exit_rftypes(rfts);
> +				return -ENOMEM;
> +			}
> +			kf_ops->atomic_write_len = rft->max_write_len;

No user either. Copy and paste once more ?

> +		}
> +
> +		rft->kf_ops = kf_ops;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * rdtgroup_init - rdtgroup initialization
> + *
> + * Register rdtgroup filesystem, and initialize any subsystems that didn't
> + * request early init.
> + */
> +int __init rdtgroup_init(void)
> +{
> +	int cpu;
> +
> +	WARN_ON(rdtgroup_init_rftypes(rdtgroup_root_base_files));
> +
> +	WARN_ON(rdtgroup_init_rftypes(res_info_files));
> +	WARN_ON(rdtgroup_init_rftypes(info_files));
> +
> +	WARN_ON(rdtgroup_init_rftypes(rdtgroup_partition_base_files));
> +	mutex_lock(&rdtgroup_mutex);
> +
> +	init_rdtgroup_root(&rdtgrp_dfl_root);
> +	WARN_ON(rdtgroup_setup_root(&rdtgrp_dfl_root, 0));
> +
> +	mutex_unlock(&rdtgroup_mutex);
> +
> +	WARN_ON(sysfs_create_mount_point(fs_kobj, "resctrl"));
> +	WARN_ON(register_filesystem(&rdt_fs_type));
> +	init_cache_domains();
> +
> +	INIT_LIST_HEAD(&rdtgroups);
> +
> +	for_each_online_cpu(cpu)
> +		per_cpu(cpu_rdtgroup, cpu) = root_rdtgrp;

Another more for each cpu loop. Where is the hotplug update happening?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]


#1478831 — [PATCH v2 11/33] x86/intel_rdt: Hot cpu support for Cache Allocation

From"Fenghua Yu" <fenghua.yu@intel.com>
Date2016-09-08 09:10 +0200
Subject[PATCH v2 11/33] x86/intel_rdt: Hot cpu support for Cache Allocation
Message-ID<sf1Db-2F3-5@gated-at.bofh.it>
In reply to#1478824
From: Vikas Shivappa <vikas.shivappa@linux.intel.com>

This patch adds hot plug cpu support for Intel Cache allocation. Support
includes updating the cache bitmask MSRs IA32_L3_QOS_n when a new CPU
package comes online or goes offline. The IA32_L3_QOS_n MSRs are one per
Class of service on each CPU package. The new package's MSRs are
synchronized with the values of existing MSRs. Also the software cache
for IA32_PQR_ASSOC MSRs are reset during hot cpu notifications.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/intel_rdt.c | 85 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index 9f30492..4537658 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -21,6 +21,7 @@
  */
 #include <linux/slab.h>
 #include <linux/err.h>
+#include <linux/cpu.h>
 #include <linux/sched.h>
 #include <asm/pqr_common.h>
 #include <asm/intel_rdt.h>
@@ -130,6 +131,9 @@ static inline void msr_update_all(int msr, u64 val)
 	on_each_cpu_mask(&rdt_cpumask, msr_cpu_update, &info, 1);
 }
 
+/*
+ * Set only one cpu in cpumask in all cpus that share the same cache.
+ */
 static inline bool rdt_cpumask_update(int cpu)
 {
 	cpumask_and(&tmp_cpumask, &rdt_cpumask, topology_core_cpumask(cpu));
@@ -141,6 +145,80 @@ static inline bool rdt_cpumask_update(int cpu)
 	return false;
 }
 
+/*
+ * cbm_update_msrs() - Updates all the existing IA32_L3_MASK_n MSRs
+ * which are one per CLOSid on the current package.
+ */
+static void cbm_update_msrs(void *dummy)
+{
+	int maxid = boot_cpu_data.x86_cache_max_closid;
+	struct rdt_remote_data info;
+	unsigned int i;
+
+	for (i = 0; i < maxid; i++) {
+		if (cctable[i].clos_refcnt) {
+			info.msr = CBM_FROM_INDEX(i);
+			info.val = cctable[i].cbm;
+			msr_cpu_update(&info);
+		}
+	}
+}
+
+static int intel_rdt_online_cpu(unsigned int cpu)
+{
+	struct intel_pqr_state *state = &per_cpu(pqr_state, cpu);
+
+	state->closid = 0;
+	mutex_lock(&rdtgroup_mutex);
+	/* The cpu is set in root rdtgroup after online. */
+	cpumask_set_cpu(cpu, &root_rdtgrp->cpu_mask);
+	per_cpu(cpu_rdtgroup, cpu) = root_rdtgrp;
+	/*
+	 * If the cpu is first time found and set in its siblings that
+	 * share the same cache, update the CBM MSRs for the cache.
+	 */
+	if (rdt_cpumask_update(cpu))
+		smp_call_function_single(cpu, cbm_update_msrs, NULL, 1);
+	mutex_unlock(&rdtgroup_mutex);
+}
+
+static int clear_rdtgroup_cpumask(unsigned int cpu)
+{
+	struct list_head *l;
+	struct rdtgroup *r;
+
+	list_for_each(l, &rdtgroup_lists) {
+		r = list_entry(l, struct rdtgroup, rdtgroup_list);
+		if (cpumask_test_cpu(cpu, &r->cpu_mask)) {
+			cpumask_clear_cpu(cpu, &r->cpu_mask);
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int intel_rdt_offline_cpu(unsigned int cpu)
+{
+	int i;
+
+	mutex_lock(&rdtgroup_mutex);
+	if (!cpumask_test_and_clear_cpu(cpu, &rdt_cpumask)) {
+		mutex_unlock(&rdtgroup_mutex);
+		return;
+	}
+
+	cpumask_and(&tmp_cpumask, topology_core_cpumask(cpu), cpu_online_mask);
+	cpumask_clear_cpu(cpu, &tmp_cpumask);
+	i = cpumask_any(&tmp_cpumask);
+
+	if (i < nr_cpu_ids)
+		cpumask_set_cpu(i, &rdt_cpumask);
+
+	clear_rdtgroup_cpumask(cpu);
+	mutex_unlock(&rdtgroup_mutex);
+}
+
 static int __init intel_rdt_late_init(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
@@ -169,6 +247,13 @@ static int __init intel_rdt_late_init(void)
 
 	for_each_online_cpu(i)
 		rdt_cpumask_update(i);
+
+	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
+				"AP_INTEL_RDT_ONLINE",
+				intel_rdt_online_cpu, intel_rdt_offline_cpu);
+	if (err < 0)
+		goto out_err;
+
 	pr_info("Intel cache allocation enabled\n");
 out_err:
 
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]


#1479025 — Re: [PATCH v2 11/33] x86/intel_rdt: Hot cpu support for Cache Allocation

FromThomas Gleixner <tglx@linutronix.de>
Date2016-09-08 12:10 +0200
SubjectRe: [PATCH v2 11/33] x86/intel_rdt: Hot cpu support for Cache Allocation
Message-ID<sf4ro-4qN-23@gated-at.bofh.it>
In reply to#1478831
On Thu, 8 Sep 2016, Fenghua Yu wrote:
> +/*
> + * cbm_update_msrs() - Updates all the existing IA32_L3_MASK_n MSRs
> + * which are one per CLOSid on the current package.
> + */
> +static void cbm_update_msrs(void *dummy)
> +{
> +	int maxid = boot_cpu_data.x86_cache_max_closid;
> +	struct rdt_remote_data info;
> +	unsigned int i;
> +
> +	for (i = 0; i < maxid; i++) {
> +		if (cctable[i].clos_refcnt) {
> +			info.msr = CBM_FROM_INDEX(i);
> +			info.val = cctable[i].cbm;
> +			msr_cpu_update(&info);
> +		}
> +	}
> +}
> +
> +static int intel_rdt_online_cpu(unsigned int cpu)
> +{
> +	struct intel_pqr_state *state = &per_cpu(pqr_state, cpu);
> +
> +	state->closid = 0;
> +	mutex_lock(&rdtgroup_mutex);
> +	/* The cpu is set in root rdtgroup after online. */
> +	cpumask_set_cpu(cpu, &root_rdtgrp->cpu_mask);
> +	per_cpu(cpu_rdtgroup, cpu) = root_rdtgrp;
> +	/*
> +	 * If the cpu is first time found and set in its siblings that

-ENOPARSE

> +	 * share the same cache, update the CBM MSRs for the cache.
> +	 */
> +	if (rdt_cpumask_update(cpu))
> +		smp_call_function_single(cpu, cbm_update_msrs, NULL, 1);

This smp_call_function() is a pointless exercise. online callbacks are
guaranteed to run on @cpu.

> +	mutex_unlock(&rdtgroup_mutex);
> +}
> +
> +static int clear_rdtgroup_cpumask(unsigned int cpu)
> +{
> +	struct list_head *l;
> +	struct rdtgroup *r;
> +
> +	list_for_each(l, &rdtgroup_lists) {
> +		r = list_entry(l, struct rdtgroup, rdtgroup_list);
> +		if (cpumask_test_cpu(cpu, &r->cpu_mask)) {
> +			cpumask_clear_cpu(cpu, &r->cpu_mask);
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;

What's the point of the return value if it gets ignored anyway.

> +}
> +
> +static int intel_rdt_offline_cpu(unsigned int cpu)
> +{
> +	int i;
> +
> +	mutex_lock(&rdtgroup_mutex);
> +	if (!cpumask_test_and_clear_cpu(cpu, &rdt_cpumask)) {
> +		mutex_unlock(&rdtgroup_mutex);
> +		return;
> +	}
> +
> +	cpumask_and(&tmp_cpumask, topology_core_cpumask(cpu), cpu_online_mask);
> +	cpumask_clear_cpu(cpu, &tmp_cpumask);
> +	i = cpumask_any(&tmp_cpumask);
> +
> +	if (i < nr_cpu_ids)
> +		cpumask_set_cpu(i, &rdt_cpumask);
> +
> +	clear_rdtgroup_cpumask(cpu);
> +	mutex_unlock(&rdtgroup_mutex);
> +}
> +
>  static int __init intel_rdt_late_init(void)
>  {
>  	struct cpuinfo_x86 *c = &boot_cpu_data;
> @@ -169,6 +247,13 @@ static int __init intel_rdt_late_init(void)
>  
>  	for_each_online_cpu(i)
>  		rdt_cpumask_update(i);
> +
> +	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> +				"AP_INTEL_RDT_ONLINE",
> +				intel_rdt_online_cpu, intel_rdt_offline_cpu);

Why are you using nocalls() here? cpuhp_setup_state() will invoke
intel_rdt_online_cpu() on every online cpu.

And you just call rdt_cpumask_update() for each cpu. What is doing the rest
of the cpu initialization (cpu_rdtgroup, root_rtgroup->cpu_mask) ????

> +	if (err < 0)
> +		goto out_err;

Oh well.....

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]


#1478832 — [PATCH v2 13/33] Define CONFIG_INTEL_RDT

From"Fenghua Yu" <fenghua.yu@intel.com>
Date2016-09-08 09:10 +0200
Subject[PATCH v2 13/33] Define CONFIG_INTEL_RDT
Message-ID<sf1Dc-2F3-17@gated-at.bofh.it>
In reply to#1478824
From: Vikas Shivappa <vikas.shivappa@linux.intel.com>

CONFIG_INTEL_RDT is defined. The option provides support for resource
allocation which is a sub-feature of Intel Resource Director Technology
(RDT).

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/Kconfig | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2a1f0ce..6782127 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -406,6 +406,19 @@ config GOLDFISH
        def_bool y
        depends on X86_GOLDFISH
 
+config INTEL_RDT
+	bool "Intel Resource Director Technology support"
+	default n
+	depends on X86_64 && CPU_SUP_INTEL
+	help
+	  This option provides support for resource allocation which is a
+	  sub-feature of Intel Resource Director Technology(RDT).
+	  Current implementation supports L3 cache allocation.
+	  Using this feature a user can specify the amount of L3 cache space
+	  into which an application can fill.
+
+	  Say N if unsure.
+
 if X86_32
 config X86_EXTENDED_PLATFORM
 	bool "Support for extended (non-PC) x86 platforms"
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]


#1479030 — Re: [PATCH v2 13/33] Define CONFIG_INTEL_RDT

FromThomas Gleixner <tglx@linutronix.de>
Date2016-09-08 12:20 +0200
SubjectRe: [PATCH v2 13/33] Define CONFIG_INTEL_RDT
Message-ID<sf4B3-4xM-7@gated-at.bofh.it>
In reply to#1478832
On Thu, 8 Sep 2016, Fenghua Yu wrote:
> From: Vikas Shivappa <vikas.shivappa@linux.intel.com>

This changelog is still utter crap.

> CONFIG_INTEL_RDT is defined.

We know already from $subject that CONFIG_INTEL_RDT is introduced.

>                              The option provides support for resource
> allocation which is a sub-feature of Intel Resource Director Technology
> (RDT).

The config switch says:
 
> +config INTEL_RDT
> +	bool "Intel Resource Director Technology support"

So what? Is this now resource allocation or resource director or does the
actual stuff introduce something completely different?

> +	default n
> +	depends on X86_64 && CPU_SUP_INTEL

Why is this 64bit only? The changelog and/or the help text should tell
that.

> +	help
> +	  This option provides support for resource allocation which is a
> +	  sub-feature of Intel Resource Director Technology(RDT).

Sure and you repeat the changelog nonsense here again.

> +	  Current implementation supports L3 cache allocation.
> +	  Using this feature a user can specify the amount of L3 cache space
> +	  into which an application can fill.

Sigh

	tglx

[toc] | [prev] | [next] | [standalone]


#1478833 — [PATCH v2 08/33] x86/intel_rdt: Add Class of service management

From"Fenghua Yu" <fenghua.yu@intel.com>
Date2016-09-08 09:10 +0200
Subject[PATCH v2 08/33] x86/intel_rdt: Add Class of service management
Message-ID<sf1Db-2F3-7@gated-at.bofh.it>
In reply to#1478824
From: Vikas Shivappa <vikas.shivappa@linux.intel.com>

Adds some data-structures and APIs to support Class of service
management(closid). There is a new clos_cbm table which keeps a 1:1
mapping between closid and capacity bit mask (cbm)
and a count of usage of closid. Each task would be associated with a
Closid at a time and this patch adds a new field closid to task_struct
to keep track of the same.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/intel_rdt.h | 12 ++++++
 arch/x86/kernel/cpu/intel_rdt.c  | 81 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 91 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/include/asm/intel_rdt.h

diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
new file mode 100644
index 0000000..68bab26
--- /dev/null
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -0,0 +1,12 @@
+#ifndef _RDT_H_
+#define _RDT_H_
+
+#ifdef CONFIG_INTEL_RDT
+
+struct clos_cbm_table {
+	unsigned long cbm;
+	unsigned int clos_refcnt;
+};
+
+#endif
+#endif
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index fcd0642..b25940a 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -21,17 +21,94 @@
  */
 #include <linux/slab.h>
 #include <linux/err.h>
+#include <asm/intel_rdt.h>
+
+/*
+ * cctable maintains 1:1 mapping between CLOSid and cache bitmask.
+ */
+static struct clos_cbm_table *cctable;
+/*
+ * closid availability bit map.
+ */
+unsigned long *closmap;
+static DEFINE_MUTEX(rdtgroup_mutex);
+
+static inline void closid_get(u32 closid)
+{
+	struct clos_cbm_table *cct = &cctable[closid];
+
+	lockdep_assert_held(&rdtgroup_mutex);
+
+	cct->clos_refcnt++;
+}
+
+static int closid_alloc(u32 *closid)
+{
+	u32 maxid;
+	u32 id;
+
+	lockdep_assert_held(&rdtgroup_mutex);
+
+	maxid = boot_cpu_data.x86_cache_max_closid;
+	id = find_first_zero_bit(closmap, maxid);
+	if (id == maxid)
+		return -ENOSPC;
+
+	set_bit(id, closmap);
+	closid_get(id);
+	*closid = id;
+
+	return 0;
+}
+
+static inline void closid_free(u32 closid)
+{
+	clear_bit(closid, closmap);
+	cctable[closid].cbm = 0;
+}
+
+static void closid_put(u32 closid)
+{
+	struct clos_cbm_table *cct = &cctable[closid];
+
+	lockdep_assert_held(&rdtgroup_mutex);
+	if (WARN_ON(!cct->clos_refcnt))
+		return;
+
+	if (!--cct->clos_refcnt)
+		closid_free(closid);
+}
 
 static int __init intel_rdt_late_init(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
+	u32 maxid;
+	int err = 0, size;
 
 	if (!cpu_has(c, X86_FEATURE_CAT_L3))
 		return -ENODEV;
 
-	pr_info("Intel cache allocation detected\n");
+	maxid = c->x86_cache_max_closid;
 
-	return 0;
+	size = maxid * sizeof(struct clos_cbm_table);
+	cctable = kzalloc(size, GFP_KERNEL);
+	if (!cctable) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	size = BITS_TO_LONGS(maxid) * sizeof(long);
+	closmap = kzalloc(size, GFP_KERNEL);
+	if (!closmap) {
+		kfree(cctable);
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	pr_info("Intel cache allocation enabled\n");
+out_err:
+
+	return err;
 }
 
 late_initcall(intel_rdt_late_init);
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]


#1478939 — Re: [PATCH v2 08/33] x86/intel_rdt: Add Class of service management

FromThomas Gleixner <tglx@linutronix.de>
Date2016-09-08 11:00 +0200
SubjectRe: [PATCH v2 08/33] x86/intel_rdt: Add Class of service management
Message-ID<sf3lE-3zk-9@gated-at.bofh.it>
In reply to#1478833
On Thu, 8 Sep 2016, Fenghua Yu wrote:
> @@ -0,0 +1,12 @@
> +#ifndef _RDT_H_
> +#define _RDT_H_

The standard guard is

    _ASM_X86_RDT_H

> +
> +#ifdef CONFIG_INTEL_RDT

What's the purpose of sticking a struct definition inside an ifdef?

> +
> +struct clos_cbm_table {
> +	unsigned long cbm;
> +	unsigned int clos_refcnt;

Please proper align the struct members for readability sake

	unsigned long	cbm;
	unsigned int	clos_refcnt;

Adding a kerneldoc comment above the struct explaining it would not hurting
either.

> +/*
> + * cctable maintains 1:1 mapping between CLOSid and cache bitmask.
> + */
> +static struct clos_cbm_table *cctable;
> +/*
> + * closid availability bit map.
> + */
> +unsigned long *closmap;
> +static DEFINE_MUTEX(rdtgroup_mutex);
> +
> +static inline void closid_get(u32 closid)
> +{
> +	struct clos_cbm_table *cct = &cctable[closid];
> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	cct->clos_refcnt++;

So the whole thing can be written with a single line at the single call
site.

	cc_table[closid].clos_refcnt++;

And that already has the lockdep_assert_held() check. While companies might
pay based on line counts, kernel development asks for readable and sensible
code.

> +}
> +
> +static int closid_alloc(u32 *closid)
> +{
> +	u32 maxid;
> +	u32 id;

Please put variables with the same type into one line. There is no value in
wasting screen estate.

> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	maxid = boot_cpu_data.x86_cache_max_closid;
> +	id = find_first_zero_bit(closmap, maxid);
> +	if (id == maxid)
> +		return -ENOSPC;
> +
> +	set_bit(id, closmap);
> +	closid_get(id);
> +	*closid = id;
> +
> +	return 0;
> +}
> +
> +static inline void closid_free(u32 closid)
> +{
> +	clear_bit(closid, closmap);
> +	cctable[closid].cbm = 0;
> +}
> +
> +static void closid_put(u32 closid)
> +{
> +	struct clos_cbm_table *cct = &cctable[closid];
> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +	if (WARN_ON(!cct->clos_refcnt))
> +		return;
> +
> +	if (!--cct->clos_refcnt)
> +		closid_free(closid);
> +}
>  
>  static int __init intel_rdt_late_init(void)
>  {
>  	struct cpuinfo_x86 *c = &boot_cpu_data;
> +	u32 maxid;
> +	int err = 0, size;
>  
>  	if (!cpu_has(c, X86_FEATURE_CAT_L3))
>  		return -ENODEV;
>  
> -	pr_info("Intel cache allocation detected\n");
> +	maxid = c->x86_cache_max_closid;
>  
> -	return 0;
> +	size = maxid * sizeof(struct clos_cbm_table);
> +	cctable = kzalloc(size, GFP_KERNEL);
> +	if (!cctable) {
> +		err = -ENOMEM;
> +		goto out_err;

I told you that before: Using a goto just to return err is pointless and
silly. What the hell is wrong with 

		return -ENOMEM;
???

> +	}
> +
> +	size = BITS_TO_LONGS(maxid) * sizeof(long);
> +	closmap = kzalloc(size, GFP_KERNEL);
> +	if (!closmap) {
> +		kfree(cctable);
> +		err = -ENOMEM;
> +		goto out_err;

Groan.

> +	}
> +
> +	pr_info("Intel cache allocation enabled\n");
> +out_err:

[toc] | [prev] | [next] | [standalone]


#1478834 — [PATCH v2 22/33] x86/intel_rdt.c: Extend RDT to per cache and per resources

From"Fenghua Yu" <fenghua.yu@intel.com>
Date2016-09-08 09:10 +0200
Subject[PATCH v2 22/33] x86/intel_rdt.c: Extend RDT to per cache and per resources
Message-ID<sf1Dc-2F3-9@gated-at.bofh.it>
In reply to#1478824
From: Fenghua Yu <fenghua.yu@intel.com>

QoS mask MSRs array is per cache. We need to allocate CLOSID per cache
instead global CLOSID.

A few different resources can share same QoS mask MSRs array. For
example, one L2 cache can share QoS MSRs with its next level
L3 cache. A domain number represents the L2 cache, the L3 cache, the L2
cache's shared cpumask, and the L3 cache's shared cpumask.

cctable is extended to be index by domain number so that each cache
has its own control table.

shared_domain is introduced to cover multiple resources sharing
CLOSID.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/intel_rdt.h |   1 +
 arch/x86/kernel/cpu/intel_rdt.c  | 647 +++++++++++++++++++++++++++++++++------
 2 files changed, 547 insertions(+), 101 deletions(-)

diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index a4f794b..85beecc 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -74,6 +74,7 @@ extern inline void closid_get(u32 closid, int domain);
 extern void closid_put(u32 closid, int domain);
 extern void closid_free(u32 closid, int domain, int level);
 extern int closid_alloc(u32 *closid, int domain);
+extern struct mutex rdtgroup_mutex;
 extern bool cat_l3_enabled;
 extern unsigned int get_domain_num(int level);
 extern struct shared_domain *shared_domain;
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index 1bcff29..f7c728b 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -17,14 +17,17 @@
  * more details.
  *
  * More information about RDT be found in the Intel (R) x86 Architecture
- * Software Developer Manual June 2015, volume 3, section 17.15.
+ * Software Developer Manual.
  */
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/cpu.h>
 #include <linux/sched.h>
+#include <linux/cpumask.h>
+#include <linux/cacheinfo.h>
 #include <asm/pqr_common.h>
 #include <asm/intel_rdt.h>
+#include <asm/intel_rdt_rdtgroup.h>
 
 /*
  * During cache alloc mode cctable maintains 1:1 mapping between
@@ -35,40 +38,62 @@
  * index of a dcache_cbm for CLOSid 'n' = n << 1.
  * index of a icache_cbm for CLOSid 'n' = n << 1 + 1
  */
-static struct clos_cbm_table *cctable;
+struct clos_cbm_table **l3_cctable;
+
 /*
  * Minimum bits required in Cache bitmask.
  */
 unsigned int min_bitmask_len = 1;
+
 /*
  * Mask of CPUs for writing CBM values. We only need one CPU per-socket.
  */
-static cpumask_t rdt_cpumask;
-/*
- * Temporary cpumask used during hot cpu notificaiton handling. The usage
- * is serialized by hot cpu locks.
- */
-static cpumask_t tmp_cpumask;
-static DEFINE_MUTEX(rdtgroup_mutex);
+cpumask_t rdt_l3_cpumask;
+
+bool cat_l3_enabled;
+
 struct static_key __read_mostly rdt_enable_key = STATIC_KEY_INIT_FALSE;
 struct clos_config cconfig;
 bool cdp_enabled;
+static bool disable_cat_l3 __initdata;
+struct shared_domain *shared_domain;
+int shared_domain_num;
+
+struct rdt_opts rdt_opts = {
+	.cdp_enabled = false,
+	.verbose = false,
+	.simulate_cat_l3 = false,
+};
+
+#define __DCBM_TABLE_INDEX(x) (x << 1)
+#define __ICBM_TABLE_INDEX(x) ((x << 1) + 1)
+#define __ICBM_MSR_INDEX(x)                    \
+	L3_CBM_FROM_INDEX(__ICBM_TABLE_INDEX(x))
+
+#define DCBM_TABLE_INDEX(x)    (x << cdp_enabled)
+#define ICBM_TABLE_INDEX(x)    ((x << cdp_enabled) + cdp_enabled)
 
-#define __DCBM_TABLE_INDEX(x)	(x << 1)
-#define __ICBM_TABLE_INDEX(x)	((x << 1) + 1)
-#define __DCBM_MSR_INDEX(x)			\
-	CBM_FROM_INDEX(__DCBM_TABLE_INDEX(x))
-#define __ICBM_MSR_INDEX(x)			\
-	CBM_FROM_INDEX(__ICBM_TABLE_INDEX(x))
+DEFINE_MUTEX(rdtgroup_mutex);
 
-#define DCBM_TABLE_INDEX(x)	(x << cdp_enabled)
-#define ICBM_TABLE_INDEX(x)	((x << cdp_enabled) + cdp_enabled)
+DEFINE_PER_CPU_READ_MOSTLY(int, cpu_l3_domain) = -1;
+DEFINE_PER_CPU_READ_MOSTLY(int, cpu_shared_domain) = -1;
+DEFINE_PER_CPU_READ_MOSTLY(struct rdtgroup *, cpu_rdtgroup) = 0;
 
 struct rdt_remote_data {
 	int msr;
 	u64 val;
 };
 
+inline int get_dcbm_table_index(int x)
+{
+	return DCBM_TABLE_INDEX(x);
+}
+
+inline int get_icbm_table_index(int x)
+{
+	return ICBM_TABLE_INDEX(x);
+}
+
 /*
  * cache_alloc_hsw_probe() - Have to probe for Intel haswell server CPUs
  * as it does not have CPUID enumeration support for Cache allocation.
@@ -98,41 +123,141 @@ static inline bool cache_alloc_hsw_probe(void)
 
 	wrmsr_safe(MSR_IA32_PQR_ASSOC, l, h_old);
 
-	boot_cpu_data.x86_cache_max_closid = 4;
-	boot_cpu_data.x86_cache_max_cbm_len = 20;
+	boot_cpu_data.x86_l3_max_closid = 4;
+	boot_cpu_data.x86_l3_max_cbm_len = 20;
 	min_bitmask_len = 2;
 
 	return true;
 }
 
+u32 max_cbm_len(int level)
+{
+	switch (level) {
+	case CACHE_LEVEL3:
+		return boot_cpu_data.x86_l3_max_cbm_len;
+	default:
+		break;
+	}
+
+	return (u32)~0;
+}
+
+u64 max_cbm(int level)
+{
+	switch (level) {
+	case CACHE_LEVEL3:
+		return (1ULL << boot_cpu_data.x86_l3_max_cbm_len) - 1;
+	default:
+		break;
+	}
+
+	return (u64)~0;
+}
+
+static u32 hw_max_closid(int level)
+{
+	switch (level) {
+	case CACHE_LEVEL3:
+		return  boot_cpu_data.x86_l3_max_closid;
+	default:
+		break;
+	}
+
+	WARN(1, "invalid level\n");
+	return 0;
+}
+
+static int cbm_from_index(u32 i, int level)
+{
+	switch (level) {
+	case CACHE_LEVEL3:
+		return L3_CBM_FROM_INDEX(i);
+	default:
+		break;
+	}
+
+	WARN(1, "invalid level\n");
+	return 0;
+}
+
+bool cat_enabled(int level)
+{
+	switch (level) {
+	case CACHE_LEVEL3:
+		return cat_l3_enabled;
+	default:
+		break;
+	}
+
+	return false;
+}
+
+static inline bool cat_l3_supported(struct cpuinfo_x86 *c)
+{
+	if (cpu_has(c, X86_FEATURE_CAT_L3))
+		return true;
+
+	/*
+	 * Probe for Haswell server CPUs.
+	 */
+	if (c->x86 == 0x6 && c->x86_model == 0x3f)
+		return cache_alloc_hsw_probe();
+
+	return false;
+}
+
 void __intel_rdt_sched_in(void *dummy)
 {
 	struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
+	struct rdtgroup *rdtgrp;
+	int closid;
+	int domain;
 
 	/*
-	 * Currently closid is always 0. When  user interface is added,
-	 * closid will come from user interface.
+	 * If this task is assigned to an rdtgroup, use it.
+	 * Else use the group assigned to this cpu.
 	 */
-	if (state->closid == 0)
+	rdtgrp = current->rdtgroup;
+	if (!rdtgrp)
+		rdtgrp = this_cpu_read(cpu_rdtgroup);
+
+	domain = this_cpu_read(cpu_shared_domain);
+	closid = rdtgrp->resource.closid[domain];
+
+	if (closid == state->closid)
 		return;
 
-	wrmsr(MSR_IA32_PQR_ASSOC, state->rmid, 0);
-	state->closid = 0;
+	state->closid = closid;
+	/* Don't really write PQR register in simulation mode. */
+	if (unlikely(rdt_opts.simulate_cat_l3))
+		return;
+
+	wrmsr(MSR_IA32_PQR_ASSOC, state->rmid, closid);
 }
 
 /*
  * When cdp mode is enabled, refcnt is maintained in the dcache_cbm entry.
  */
-static inline void closid_get(u32 closid)
+inline void closid_get(u32 closid, int domain)
 {
-	struct clos_cbm_table *cct = &cctable[DCBM_TABLE_INDEX(closid)];
-
 	lockdep_assert_held(&rdtgroup_mutex);
 
-	cct->clos_refcnt++;
+	if (cat_l3_enabled) {
+		int l3_domain;
+		int dindex;
+
+		l3_domain = shared_domain[domain].l3_domain;
+		dindex = DCBM_TABLE_INDEX(closid);
+		l3_cctable[l3_domain][dindex].clos_refcnt++;
+		if (cdp_enabled) {
+			int iindex = ICBM_TABLE_INDEX(closid);
+
+			l3_cctable[l3_domain][iindex].clos_refcnt++;
+		}
+	}
 }
 
-static int closid_alloc(u32 *closid)
+int closid_alloc(u32 *closid, int domain)
 {
 	u32 maxid;
 	u32 id;
@@ -140,105 +265,215 @@ static int closid_alloc(u32 *closid)
 	lockdep_assert_held(&rdtgroup_mutex);
 
 	maxid = cconfig.max_closid;
-	id = find_first_zero_bit(cconfig.closmap, maxid);
+	id = find_first_zero_bit((unsigned long *)cconfig.closmap[domain],
+				 maxid);
+
 	if (id == maxid)
 		return -ENOSPC;
 
-	set_bit(id, cconfig.closmap);
-	closid_get(id);
+	set_bit(id, (unsigned long *)cconfig.closmap[domain]);
+	closid_get(id, domain);
 	*closid = id;
-	cconfig.closids_used++;
 
 	return 0;
 }
 
-static inline void closid_free(u32 closid)
+unsigned int get_domain_num(int level)
 {
-	clear_bit(closid, cconfig.closmap);
-	cctable[DCBM_TABLE_INDEX(closid)].cbm = 0;
-
-	if (WARN_ON(!cconfig.closids_used))
-		return;
+	if (level == CACHE_LEVEL3)
+		return cpumask_weight(&rdt_l3_cpumask);
+	else
+		return -EINVAL;
+}
 
-	cconfig.closids_used--;
+int level_to_leaf(int level)
+{
+	switch (level) {
+	case CACHE_LEVEL3:
+		return 3;
+	default:
+		return -EINVAL;
+	}
 }
 
-static void closid_put(u32 closid)
+void closid_free(u32 closid, int domain, int level)
 {
-	struct clos_cbm_table *cct = &cctable[DCBM_TABLE_INDEX(closid)];
+	struct clos_cbm_table **cctable;
+	int leaf;
+	struct cpumask *mask;
+	int cpu;
+
+	if (level == CACHE_LEVEL3)
+		cctable = l3_cctable;
+
+	clear_bit(closid, (unsigned long *)cconfig.closmap[domain]);
+
+	if (level == CACHE_LEVEL3) {
+		cctable[domain][closid].cbm = max_cbm(level);
+		leaf = level_to_leaf(level);
+		mask = &cache_domains[leaf].shared_cpu_map[domain];
+		cpu = cpumask_first(mask);
+		smp_call_function_single(cpu, cbm_update_l3_msr, &closid, 1);
+	}
+}
 
+static void _closid_put(u32 closid, struct clos_cbm_table *cct,
+			int domain, int level)
+{
 	lockdep_assert_held(&rdtgroup_mutex);
 	if (WARN_ON(!cct->clos_refcnt))
 		return;
 
 	if (!--cct->clos_refcnt)
-		closid_free(closid);
+		closid_free(closid, domain, level);
 }
 
-static void msr_cpu_update(void *arg)
+void closid_put(u32 closid, int domain)
+{
+	struct clos_cbm_table *cct;
+
+	if (cat_l3_enabled) {
+		int l3_domain = shared_domain[domain].l3_domain;
+
+		cct = &l3_cctable[l3_domain][DCBM_TABLE_INDEX(closid)];
+		_closid_put(closid, cct, l3_domain, CACHE_LEVEL3);
+		if (cdp_enabled) {
+			cct = &l3_cctable[l3_domain][ICBM_TABLE_INDEX(closid)];
+			_closid_put(closid, cct, l3_domain, CACHE_LEVEL3);
+		}
+	}
+}
+
+void msr_cpu_update(void *arg)
 {
 	struct rdt_remote_data *info = arg;
 
+	if (unlikely(rdt_opts.verbose))
+		pr_info("Write %lx to msr %x on cpu%d\n",
+			(unsigned long)info->val, info->msr,
+			smp_processor_id());
+
+	if (unlikely(rdt_opts.simulate_cat_l3))
+		return;
+
 	wrmsrl(info->msr, info->val);
 }
 
+static struct cpumask *rdt_cache_cpumask(int level)
+{
+	return &rdt_l3_cpumask;
+}
+
 /*
  * msr_update_all() - Update the msr for all packages.
  */
-static inline void msr_update_all(int msr, u64 val)
+static inline void msr_update_all(int msr, u64 val, int level)
 {
 	struct rdt_remote_data info;
 
 	info.msr = msr;
 	info.val = val;
-	on_each_cpu_mask(&rdt_cpumask, msr_cpu_update, &info, 1);
+	on_each_cpu_mask(rdt_cache_cpumask(level), msr_cpu_update, &info, 1);
 }
 
-static bool code_data_mask_equal(void)
+static void init_qos_msrs(int level)
 {
-	int i, dindex, iindex;
+	if (cat_enabled(level)) {
+		u32 maxcbm;
+		u32 i;
 
-	for (i = 0; i < cconfig.max_closid; i++) {
-		dindex = __DCBM_TABLE_INDEX(i);
-		iindex = __ICBM_TABLE_INDEX(i);
-		if (cctable[dindex].clos_refcnt &&
-		     (cctable[dindex].cbm != cctable[iindex].cbm))
-			return false;
+		maxcbm = max_cbm(level);
+		for (i = 0; i < hw_max_closid(level); i++)
+			msr_update_all(cbm_from_index(i, level), maxcbm, level);
 	}
+}
 
-	return true;
+/*
+ * Initialize QOS_MASK_n registers to all 1's.
+ *
+ * Initialize L3_QOS_CFG register to enable or disable CDP.
+ */
+void init_msrs(bool cdpenabled)
+{
+	if (cat_enabled(CACHE_LEVEL3)) {
+		init_qos_msrs(CACHE_LEVEL3);
+		msr_update_all(MSR_IA32_L3_QOS_CFG, cdpenabled, CACHE_LEVEL3);
+	}
+
+}
+
+int get_cache_leaf(int level, int cpu)
+{
+	int index;
+	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
+	struct cacheinfo *this_leaf;
+	int num_leaves = this_cpu_ci->num_leaves;
+
+	for (index = 0; index < num_leaves; index++) {
+		this_leaf = this_cpu_ci->info_list + index;
+		if (this_leaf->level == level)
+			return index;
+	}
+
+	return -EINVAL;
+}
+
+static struct cpumask *get_shared_cpu_map(int cpu, int level)
+{
+	int index;
+	struct cacheinfo *leaf;
+	struct cpu_cacheinfo *cpu_ci = get_cpu_cacheinfo(cpu);
+
+	index = get_cache_leaf(level, cpu);
+	if (index < 0)
+		return 0;
+
+	leaf = cpu_ci->info_list + index;
+
+	return &leaf->shared_cpu_map;
 }
 
 /*
  * Set only one cpu in cpumask in all cpus that share the same cache.
  */
-static inline bool rdt_cpumask_update(int cpu)
+inline bool rdt_cpumask_update(struct cpumask *cpumask, int cpu, int level)
 {
-	cpumask_and(&tmp_cpumask, &rdt_cpumask, topology_core_cpumask(cpu));
+	struct cpumask *shared_cpu_map;
+	cpumask_t tmp_cpumask;
+
+	shared_cpu_map = get_shared_cpu_map(cpu, level);
+	if (!shared_cpu_map)
+		return false;
+
+	cpumask_and(&tmp_cpumask, cpumask, shared_cpu_map);
 	if (cpumask_empty(&tmp_cpumask)) {
-		cpumask_set_cpu(cpu, &rdt_cpumask);
+		cpumask_set_cpu(cpu, cpumask);
 		return true;
 	}
 
 	return false;
 }
 
-static void cbm_update_msr(u32 index)
+void cbm_update_l3_msr(void *pindex)
 {
 	struct rdt_remote_data info;
+	int index;
 	int dindex;
+	int l3_domain;
+	struct clos_cbm_table *pl3_cctable;
 
+	index = *(int *)pindex;
 	dindex = DCBM_TABLE_INDEX(index);
-	if (cctable[dindex].clos_refcnt) {
-
-		info.msr = CBM_FROM_INDEX(dindex);
-		info.val = cctable[dindex].cbm;
-		msr_cpu_update((void *) &info);
-
+	l3_domain =  per_cpu(cpu_l3_domain, smp_processor_id());
+	pl3_cctable = &l3_cctable[l3_domain][dindex];
+	if (pl3_cctable->clos_refcnt) {
+		info.msr = L3_CBM_FROM_INDEX(dindex);
+		info.val = pl3_cctable->cbm;
+		msr_cpu_update(&info);
 		if (cdp_enabled) {
 			info.msr = __ICBM_MSR_INDEX(index);
-			info.val = cctable[dindex + 1].cbm;
-			msr_cpu_update((void *) &info);
+			info.val = l3_cctable[l3_domain][dindex+1].cbm;
+			msr_cpu_update(&info);
 		}
 	}
 }
@@ -252,8 +487,9 @@ static void cbm_update_msrs(void *dummy)
 	int maxid = cconfig.max_closid;
 	unsigned int i;
 
-	for (i = 0; i < maxid; i++) {
-		cbm_update_msr(i);
+	if (cat_l3_enabled) {
+		for (i = 0; i < maxid; i++)
+			cbm_update_l3_msr(&i);
 	}
 }
 
@@ -270,9 +506,11 @@ static int intel_rdt_online_cpu(unsigned int cpu)
 	 * If the cpu is first time found and set in its siblings that
 	 * share the same cache, update the CBM MSRs for the cache.
 	 */
-	if (rdt_cpumask_update(cpu))
+	if (rdt_cpumask_update(&rdt_l3_cpumask, cpu, CACHE_LEVEL3))
 		smp_call_function_single(cpu, cbm_update_msrs, NULL, 1);
 	mutex_unlock(&rdtgroup_mutex);
+
+	return 0;
 }
 
 static int clear_rdtgroup_cpumask(unsigned int cpu)
@@ -293,63 +531,270 @@ static int clear_rdtgroup_cpumask(unsigned int cpu)
 
 static int intel_rdt_offline_cpu(unsigned int cpu)
 {
-	int i;
+	struct cpumask *shared_cpu_map;
+	int new_cpu;
+	int l3_domain;
+	int level;
+	int leaf;
 
 	mutex_lock(&rdtgroup_mutex);
-	if (!cpumask_test_and_clear_cpu(cpu, &rdt_cpumask)) {
-		mutex_unlock(&rdtgroup_mutex);
-		return;
-	}
 
-	cpumask_and(&tmp_cpumask, topology_core_cpumask(cpu), cpu_online_mask);
-	cpumask_clear_cpu(cpu, &tmp_cpumask);
-	i = cpumask_any(&tmp_cpumask);
+	level = CACHE_LEVEL3;
+
+	l3_domain = per_cpu(cpu_l3_domain, cpu);
+	leaf = level_to_leaf(level);
+	shared_cpu_map = &cache_domains[leaf].shared_cpu_map[l3_domain];
 
-	if (i < nr_cpu_ids)
-		cpumask_set_cpu(i, &rdt_cpumask);
+	cpumask_clear_cpu(cpu, &rdt_l3_cpumask);
+	cpumask_clear_cpu(cpu, shared_cpu_map);
+	if (cpumask_empty(shared_cpu_map))
+		goto out;
+
+	new_cpu = cpumask_first(shared_cpu_map);
+	rdt_cpumask_update(&rdt_l3_cpumask, new_cpu, level);
 
 	clear_rdtgroup_cpumask(cpu);
+out:
 	mutex_unlock(&rdtgroup_mutex);
+	return 0;
+}
+
+/*
+ * Initialize per-cpu cpu_l3_domain.
+ *
+ * cpu_l3_domain numbers are consequtive integer starting from 0.
+ * Sets up 1:1 mapping of cpu id and cpu_l3_domain.
+ */
+static int __init cpu_cache_domain_init(int level)
+{
+	int i, j;
+	int max_cpu_cache_domain = 0;
+	int index;
+	struct cacheinfo *leaf;
+	int *domain;
+	struct cpu_cacheinfo *cpu_ci;
+
+	for_each_online_cpu(i) {
+		domain = &per_cpu(cpu_l3_domain, i);
+		if (*domain == -1) {
+			index = get_cache_leaf(level, i);
+			if (index < 0)
+				return -EINVAL;
+
+			cpu_ci = get_cpu_cacheinfo(i);
+			leaf = cpu_ci->info_list + index;
+			if (cpumask_empty(&leaf->shared_cpu_map)) {
+				WARN(1, "no shared cpu for L2\n");
+				return -EINVAL;
+			}
+
+			for_each_cpu(j, &leaf->shared_cpu_map) {
+				domain = &per_cpu(cpu_l3_domain, j);
+				*domain = max_cpu_cache_domain;
+			}
+			max_cpu_cache_domain++;
+		}
+	}
+
+	return 0;
+}
+
+static int __init rdt_setup(char *str)
+{
+	char *tok;
+
+	while ((tok = strsep(&str, ",")) != NULL) {
+		if (!*tok)
+			return -EINVAL;
+
+		if (strcmp(tok, "simulate_cat_l3") == 0) {
+			pr_info("Simulate CAT L3\n");
+			rdt_opts.simulate_cat_l3 = true;
+		} else if (strcmp(tok, "disable_cat_l3") == 0) {
+			pr_info("CAT L3 is disabled\n");
+			disable_cat_l3 = true;
+		} else {
+			pr_info("Invalid rdt option\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+__setup("resctrl=", rdt_setup);
+
+static inline bool resource_alloc_enabled(void)
+{
+	return cat_l3_enabled;
+}
+
+static int shared_domain_init(void)
+{
+	int l3_domain_num = get_domain_num(CACHE_LEVEL3);
+	int size;
+	int domain;
+	struct cpumask *cpumask;
+	struct cpumask *shared_cpu_map;
+	int cpu;
+
+	if (cat_l3_enabled) {
+		shared_domain_num = l3_domain_num;
+		cpumask = &rdt_l3_cpumask;
+	} else
+		return -EINVAL;
+
+	size = shared_domain_num * sizeof(struct shared_domain);
+	shared_domain = kzalloc(size, GFP_KERNEL);
+	if (!shared_domain)
+		return -EINVAL;
+
+	domain = 0;
+	for_each_cpu(cpu, cpumask) {
+		if (cat_l3_enabled)
+			shared_domain[domain].l3_domain =
+					per_cpu(cpu_l3_domain, cpu);
+		else
+			shared_domain[domain].l3_domain = -1;
+
+		shared_cpu_map = get_shared_cpu_map(cpu, CACHE_LEVEL3);
+
+		cpumask_copy(&shared_domain[domain].cpumask, shared_cpu_map);
+
+		domain++;
+	}
+	for_each_online_cpu(cpu) {
+		if (cat_l3_enabled)
+			per_cpu(cpu_shared_domain, cpu) =
+					per_cpu(cpu_l3_domain, cpu);
+	}
+
+	return 0;
+}
+
+static int cconfig_init(int maxid)
+{
+	int num;
+	int domain;
+	unsigned long *closmap_block;
+	int maxid_size;
+
+	maxid_size = BITS_TO_LONGS(maxid);
+	num = maxid_size * shared_domain_num;
+	cconfig.closmap = kcalloc(maxid, sizeof(unsigned long *), GFP_KERNEL);
+	if (!cconfig.closmap)
+		goto out_free;
+
+	closmap_block = kcalloc(num, sizeof(unsigned long), GFP_KERNEL);
+	if (!closmap_block)
+		goto out_free;
+
+	for (domain = 0; domain < shared_domain_num; domain++)
+		cconfig.closmap[domain] = (unsigned long *)closmap_block +
+					  domain * maxid_size;
+
+	cconfig.max_closid = maxid;
+
+	return 0;
+out_free:
+	kfree(cconfig.closmap);
+	kfree(closmap_block);
+	return -ENOMEM;
+}
+
+static int __init cat_cache_init(int level, int maxid,
+				 struct clos_cbm_table ***cctable)
+{
+	int domain_num;
+	int domain;
+	int size;
+	int ret = 0;
+	struct clos_cbm_table *p;
+
+	domain_num = get_domain_num(level);
+	size = domain_num * sizeof(struct clos_cbm_table *);
+	*cctable = kzalloc(size, GFP_KERNEL);
+	if (!*cctable) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	size = maxid * domain_num * sizeof(struct clos_cbm_table);
+	p = kzalloc(size, GFP_KERNEL);
+	if (!p) {
+		kfree(*cctable);
+		ret = -ENOMEM;
+		goto out;
+	}
+	for (domain = 0; domain < domain_num; domain++)
+		(*cctable)[domain] = p + domain * maxid;
+
+	ret = cpu_cache_domain_init(level);
+	if (ret) {
+		kfree(*cctable);
+		kfree(p);
+	}
+out:
+	return ret;
 }
 
 static int __init intel_rdt_late_init(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
 	u32 maxid;
-	int err = 0, size, i;
-
-	maxid = c->x86_cache_max_closid;
-
-	size = maxid * sizeof(struct clos_cbm_table);
-	cctable = kzalloc(size, GFP_KERNEL);
-	if (!cctable) {
-		err = -ENOMEM;
-		goto out_err;
+	int i;
+	int ret;
+
+	if (unlikely(disable_cat_l3))
+		cat_l3_enabled = false;
+	else if (cat_l3_supported(c))
+		cat_l3_enabled = true;
+	else if (rdt_opts.simulate_cat_l3 &&
+		 get_cache_leaf(CACHE_LEVEL3, 0) >= 0)
+		cat_l3_enabled = true;
+	else
+		cat_l3_enabled = false;
+
+	if (!resource_alloc_enabled())
+		return -ENODEV;
+
+	if (rdt_opts.simulate_cat_l3) {
+		boot_cpu_data.x86_l3_max_closid = 16;
+		boot_cpu_data.x86_l3_max_cbm_len = 20;
+	}
+	for_each_online_cpu(i) {
+		rdt_cpumask_update(&rdt_l3_cpumask, i, CACHE_LEVEL3);
 	}
 
-	size = BITS_TO_LONGS(maxid) * sizeof(long);
-	cconfig.closmap = kzalloc(size, GFP_KERNEL);
-	if (!cconfig.closmap) {
-		kfree(cctable);
-		err = -ENOMEM;
-		goto out_err;
+	maxid = 0;
+	if (cat_l3_enabled) {
+		maxid = boot_cpu_data.x86_l3_max_closid;
+		ret = cat_cache_init(CACHE_LEVEL3, maxid, &l3_cctable);
+		if (ret)
+			cat_l3_enabled = false;
 	}
 
-	for_each_online_cpu(i)
-		rdt_cpumask_update(i);
+	if (!cat_l3_enabled)
+		return -ENOSPC;
+
+	ret = shared_domain_init();
+	if (ret)
+		return -ENODEV;
+
+	ret = cconfig_init(maxid);
+	if (ret)
+		return ret;
 
 	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
 				"AP_INTEL_RDT_ONLINE",
 				intel_rdt_online_cpu, intel_rdt_offline_cpu);
-	if (err < 0)
-		goto out_err;
+	if (ret < 0)
+		return ret;
 
 	pr_info("Intel cache allocation enabled\n");
 	if (cpu_has(c, X86_FEATURE_CDP_L3))
 		pr_info("Intel code data prioritization detected\n");
-out_err:
 
-	return err;
+	return 0;
 }
 
 late_initcall(intel_rdt_late_init);
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]


#1479311 — Re: [PATCH v2 22/33] x86/intel_rdt.c: Extend RDT to per cache and per resources

FromThomas Gleixner <tglx@linutronix.de>
Date2016-09-08 17:10 +0200
SubjectRe: [PATCH v2 22/33] x86/intel_rdt.c: Extend RDT to per cache and per resources
Message-ID<sf97H-7om-21@gated-at.bofh.it>
In reply to#1478834
On Thu, 8 Sep 2016, Fenghua Yu wrote:

> +#define __DCBM_TABLE_INDEX(x) (x << 1)
> +#define __ICBM_TABLE_INDEX(x) ((x << 1) + 1)

This macro mess is completely undocumented.

> +inline int get_dcbm_table_index(int x)

static inline ??? 

> +{
> +	return DCBM_TABLE_INDEX(x);
> +}
> +
> +inline int get_icbm_table_index(int x)
> +{
> +	return ICBM_TABLE_INDEX(x);
> +}

Why are you introducing these when they are not used at all?

>  /*
>   * cache_alloc_hsw_probe() - Have to probe for Intel haswell server CPUs
>   * as it does not have CPUID enumeration support for Cache allocation.
> @@ -98,41 +123,141 @@ static inline bool cache_alloc_hsw_probe(void)
>  
>  	wrmsr_safe(MSR_IA32_PQR_ASSOC, l, h_old);
>  
> -	boot_cpu_data.x86_cache_max_closid = 4;
> -	boot_cpu_data.x86_cache_max_cbm_len = 20;
> +	boot_cpu_data.x86_l3_max_closid = 4;
> +	boot_cpu_data.x86_l3_max_cbm_len = 20;

So if you actually change the name of the cpudata struct member, then this
would make sense to be split out into a seperate patch. But hell, the patch
order of this stuff is an unholy mess anyway.

>  	min_bitmask_len = 2;
>  
>  	return true;
>  }
>  
> +u32 max_cbm_len(int level)
> +{
> +	switch (level) {
> +	case CACHE_LEVEL3:
> +		return boot_cpu_data.x86_l3_max_cbm_len;
> +	default:
> +		break;
> +	}
> +
> +	return (u32)~0;
> +}
> +u64 max_cbm(int level)

More functions without users and of course without a proper prefix for
public consumption. Documentation is not available either.

> +{
> +	switch (level) {
> +	case CACHE_LEVEL3:
> +		return (1ULL << boot_cpu_data.x86_l3_max_cbm_len) - 1;

So the above max_cmb_len() returns:

	cpuid_count(0x00000010, 1, &eax, &ebx, &ecx, &edx);
	c->x86_l3_max_cbm_len = eax + 1;

i.e the content of leaf 10:1 EAX plus 1.

According to the SDM:

    EAX 4:0: Length of the capacity bit mask for the corresponding ResID.

So first of all. Why is there a "+ 1" ? Again, that's not documented in the
cpuid initialization code at all.

So now you take that magic number and return:

       (2 ^ magic) - 1

Cute. To be honest I'm too lazy to read through the SDM and figure it out
myself. This kind of stuff belongs into the code as comments. I seriously
doubt that the function names match the actual meaning.

> +	default:
> +		break;
> +	}
> +
> +	return (u64)~0;

What kind of return code is this ?

> +static inline bool cat_l3_supported(struct cpuinfo_x86 *c)
> +{
> +	if (cpu_has(c, X86_FEATURE_CAT_L3))
> +		return true;
> +
> +	/*
> +	 * Probe for Haswell server CPUs.
> +	 */
> +	if (c->x86 == 0x6 && c->x86_model == 0x3f)
> +		return cache_alloc_hsw_probe();

Ah now we have an actual user for that haswell probe thingy ....

> +	return false;
> +}
> +
>  void __intel_rdt_sched_in(void *dummy)

I still have not found a reasonable explanation for this dummy argument.

>  {
>  	struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
> +	struct rdtgroup *rdtgrp;
> +	int closid;
> +	int domain;

Sigh.
  
>  	/*
> -	 * Currently closid is always 0. When  user interface is added,
> -	 * closid will come from user interface.
> +	 * If this task is assigned to an rdtgroup, use it.
> +	 * Else use the group assigned to this cpu.
>  	 */
> -	if (state->closid == 0)
> +	rdtgrp = current->rdtgroup;
> +	if (!rdtgrp)
> +		rdtgrp = this_cpu_read(cpu_rdtgroup);

This makes actually sense! Thanks for listening!

> +
> +	domain = this_cpu_read(cpu_shared_domain);
> +	closid = rdtgrp->resource.closid[domain];
> +
> +	if (closid == state->closid)
>  		return;
>  
> -	wrmsr(MSR_IA32_PQR_ASSOC, state->rmid, 0);
> -	state->closid = 0;
> +	state->closid = closid;
> +	/* Don't really write PQR register in simulation mode. */
> +	if (unlikely(rdt_opts.simulate_cat_l3))
> +		return;
> +
> +	wrmsr(MSR_IA32_PQR_ASSOC, state->rmid, closid);
>  }
>  
>  /*
>   * When cdp mode is enabled, refcnt is maintained in the dcache_cbm entry.
>   */
> -static inline void closid_get(u32 closid)
> +inline void closid_get(u32 closid, int domain)

s/static inline/inline/ What the heck?

Can you please explain what this is doing?

>  {
> -	struct clos_cbm_table *cct = &cctable[DCBM_TABLE_INDEX(closid)];
> -
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> -	cct->clos_refcnt++;
> +	if (cat_l3_enabled) {
> +		int l3_domain;
> +		int dindex;

  		int l3_domain, dindex;

> +		l3_domain = shared_domain[domain].l3_domain;
> +		dindex = DCBM_TABLE_INDEX(closid);
> +		l3_cctable[l3_domain][dindex].clos_refcnt++;
> +		if (cdp_enabled) {
> +			int iindex = ICBM_TABLE_INDEX(closid);

And if you call that variable 'index' instead of 'dindex' then you don't
need an extra one 'iindex'.

> +
> +			l3_cctable[l3_domain][iindex].clos_refcnt++;

So now you have a seperate refcount for the Icache part, but the comment
above the function still says otherwise.

> +		}
> +	}
>  }
>  
> -static int closid_alloc(u32 *closid)
> +int closid_alloc(u32 *closid, int domain)
>  {
>  	u32 maxid;
>  	u32 id;
> @@ -140,105 +265,215 @@ static int closid_alloc(u32 *closid)
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
>  	maxid = cconfig.max_closid;
> -	id = find_first_zero_bit(cconfig.closmap, maxid);
> +	id = find_first_zero_bit((unsigned long *)cconfig.closmap[domain],

Why do you need a typecast here? Get your damned structs straight.

> +				 maxid);
> +
>  	if (id == maxid)
>  		return -ENOSPC;
>  
> -	set_bit(id, cconfig.closmap);
> -	closid_get(id);
> +	set_bit(id, (unsigned long *)cconfig.closmap[domain]);
> +	closid_get(id, domain);
>  	*closid = id;
> -	cconfig.closids_used++;
>  
>  	return 0;
>  }
>  
> -static inline void closid_free(u32 closid)
> +unsigned int get_domain_num(int level)
>  {
> -	clear_bit(closid, cconfig.closmap);
> -	cctable[DCBM_TABLE_INDEX(closid)].cbm = 0;
> -
> -	if (WARN_ON(!cconfig.closids_used))
> -		return;
> +	if (level == CACHE_LEVEL3)
> +		return cpumask_weight(&rdt_l3_cpumask);

get_domain_num(level) suggests to me that it returns the domain number
corresponding to the level, but it actually returns the number of bits set
in the rdt_l3_cpumask. Very intuitive - NOT!

> +	else
> +		return -EINVAL;

Proper return value for a function returning 'unsigned int' ....

> +}
>  
> -	cconfig.closids_used--;
> +int level_to_leaf(int level)
> +{
> +	switch (level) {
> +	case CACHE_LEVEL3:
> +		return 3;
> +	default:
> +		return -EINVAL;
> +	}
>  }
>  
> -static void closid_put(u32 closid)
> +void closid_free(u32 closid, int domain, int level)
>  {
> -	struct clos_cbm_table *cct = &cctable[DCBM_TABLE_INDEX(closid)];
> +	struct clos_cbm_table **cctable;
> +	int leaf;
> +	struct cpumask *mask;
> +	int cpu;
> +
> +	if (level == CACHE_LEVEL3)
> +		cctable = l3_cctable;

Oh well. Why needs this assignment to happen here?

> +
> +	clear_bit(closid, (unsigned long *)cconfig.closmap[domain]);
> +
> +	if (level == CACHE_LEVEL3) {

And not here where it actually makes sense?

> +		cctable[domain][closid].cbm = max_cbm(level);
> +		leaf = level_to_leaf(level);
> +		mask = &cache_domains[leaf].shared_cpu_map[domain];
> +		cpu = cpumask_first(mask);
> +		smp_call_function_single(cpu, cbm_update_l3_msr, &closid, 1);

A comment explaining that this must be done on one of the cpus in @domain
would be too helpful.

> +	}
> +}
>  
> +static void _closid_put(u32 closid, struct clos_cbm_table *cct,

Please use two underscores so it's obvious.

> +			int domain, int level)
> +{
>  	lockdep_assert_held(&rdtgroup_mutex);
>  	if (WARN_ON(!cct->clos_refcnt))
>  		return;
>  
>  	if (!--cct->clos_refcnt)
> -		closid_free(closid);
> +		closid_free(closid, domain, level);
>  }
>  
> -static void msr_cpu_update(void *arg)
> +void closid_put(u32 closid, int domain)
> +{
> +	struct clos_cbm_table *cct;
> +
> +	if (cat_l3_enabled) {
> +		int l3_domain = shared_domain[domain].l3_domain;
> +
> +		cct = &l3_cctable[l3_domain][DCBM_TABLE_INDEX(closid)];
> +		_closid_put(closid, cct, l3_domain, CACHE_LEVEL3);
> +		if (cdp_enabled) {
> +			cct = &l3_cctable[l3_domain][ICBM_TABLE_INDEX(closid)];
> +			_closid_put(closid, cct, l3_domain, CACHE_LEVEL3);
> +		}
> +	}
> +}
> +
> +void msr_cpu_update(void *arg)
>  {
>  	struct rdt_remote_data *info = arg;
>  
> +	if (unlikely(rdt_opts.verbose))
> +		pr_info("Write %lx to msr %x on cpu%d\n",
> +			(unsigned long)info->val, info->msr,
> +			smp_processor_id());

That's what DYNAMIC_DEBUG is for.

> +
> +	if (unlikely(rdt_opts.simulate_cat_l3))
> +		return;

Do we really need all this simulation stuff?

> +
>  	wrmsrl(info->msr, info->val);
>  }
>  
> +static struct cpumask *rdt_cache_cpumask(int level)
> +{
> +	return &rdt_l3_cpumask;

That's a really useful helper .... Your choice of checking 'level' five
times in a row in various helpers versus returning l3 unconditionally is at
least interesting.

> +int get_cache_leaf(int level, int cpu)
> +{
> +	int index;
> +	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);

this_cpu_ci is a complete misnomer as it suggests that it's actually the
cacheinfo for 'this cpu', i.e. the cpu on which the code is executing.

> +	struct cacheinfo *this_leaf;
> +	int num_leaves = this_cpu_ci->num_leaves;
> +
> +	for (index = 0; index < num_leaves; index++) {
> +		this_leaf = this_cpu_ci->info_list + index;
> +		if (this_leaf->level == level)
> +			return index;

The function is misnomed as well. It does not return the cache leaf, it
returns the leaf index .....

> +	}

Why do you have a cacheinfo related function in this RDT code? 

> +
> +	return -EINVAL;
> +}
> +
> +static struct cpumask *get_shared_cpu_map(int cpu, int level)
> +{
> +	int index;
> +	struct cacheinfo *leaf;
> +	struct cpu_cacheinfo *cpu_ci = get_cpu_cacheinfo(cpu);
> +
> +	index = get_cache_leaf(level, cpu);
> +	if (index < 0)
> +		return 0;
> +
> +	leaf = cpu_ci->info_list + index;

While here you actually get the leaf.

> +
> +	return &leaf->shared_cpu_map;
>  }
  
>  static int clear_rdtgroup_cpumask(unsigned int cpu)
> @@ -293,63 +531,270 @@ static int clear_rdtgroup_cpumask(unsigned int cpu)
>  
>  static int intel_rdt_offline_cpu(unsigned int cpu)
>  {
> -	int i;
> +	struct cpumask *shared_cpu_map;
> +	int new_cpu;
> +	int l3_domain;
> +	int level;
> +	int leaf;

Sigh. 1/3 of the line space is wasted for single variable declarations.
 
>  	mutex_lock(&rdtgroup_mutex);
> -	if (!cpumask_test_and_clear_cpu(cpu, &rdt_cpumask)) {
> -		mutex_unlock(&rdtgroup_mutex);
> -		return;
> -	}
>  
> -	cpumask_and(&tmp_cpumask, topology_core_cpumask(cpu), cpu_online_mask);
> -	cpumask_clear_cpu(cpu, &tmp_cpumask);
> -	i = cpumask_any(&tmp_cpumask);
> +	level = CACHE_LEVEL3;

I have a hard time to understand the value of that 'level' variable, but
well that's the least of my worries with that code.

> +
> +	l3_domain = per_cpu(cpu_l3_domain, cpu);
> +	leaf = level_to_leaf(level);
> +	shared_cpu_map = &cache_domains[leaf].shared_cpu_map[l3_domain];
>  
> -	if (i < nr_cpu_ids)
> -		cpumask_set_cpu(i, &rdt_cpumask);
> +	cpumask_clear_cpu(cpu, &rdt_l3_cpumask);
> +	cpumask_clear_cpu(cpu, shared_cpu_map);
> +	if (cpumask_empty(shared_cpu_map))
> +		goto out;

So what clears @cpu in the rdtgroup cpumask?

> +
> +	new_cpu = cpumask_first(shared_cpu_map);
> +	rdt_cpumask_update(&rdt_l3_cpumask, new_cpu, level);
>  
>  	clear_rdtgroup_cpumask(cpu);

Can you please use a consistent prefix and naming scheme?

    rdt_cpumask_update()
    clear_rdtgroup_cpumask()

WTF?

> +out:
>  	mutex_unlock(&rdtgroup_mutex);
> +	return 0;
> +}
> +
> +/*
> + * Initialize per-cpu cpu_l3_domain.
> + *
> + * cpu_l3_domain numbers are consequtive integer starting from 0.
> + * Sets up 1:1 mapping of cpu id and cpu_l3_domain.
> + */
> +static int __init cpu_cache_domain_init(int level)
> +{
> +	int i, j;
> +	int max_cpu_cache_domain = 0;
> +	int index;
> +	struct cacheinfo *leaf;
> +	int *domain;
> +	struct cpu_cacheinfo *cpu_ci;

Eyes hurt.

> +
> +	for_each_online_cpu(i) {
> +		domain = &per_cpu(cpu_l3_domain, i);

per_cpu_ptr() exists for a reason.

> +		if (*domain == -1) {
> +			index = get_cache_leaf(level, i);
> +			if (index < 0)
> +				return -EINVAL;
> +
> +			cpu_ci = get_cpu_cacheinfo(i);
> +			leaf = cpu_ci->info_list + index;
> +			if (cpumask_empty(&leaf->shared_cpu_map)) {
> +				WARN(1, "no shared cpu for L2\n");

So L2 is always the right thing for every value of @level? And what the
heck is the value of that WARN? Nothing because the callchain is already
known. What's worse is that you don't tell about @level, @index, @i (which
should be named @cpu).

> +				return -EINVAL;
> +			}
> +
> +			for_each_cpu(j, &leaf->shared_cpu_map) {

So again independent of @level you fiddle with cpu_l3_domain. Interesting.

> +				domain = &per_cpu(cpu_l3_domain, j);
> +				*domain = max_cpu_cache_domain;
> +			}
> +			max_cpu_cache_domain++;

what's the actual meaning of max_cpu_cache_domain?

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init rdt_setup(char *str)
> +{
> +	char *tok;
> +
> +	while ((tok = strsep(&str, ",")) != NULL) {
> +		if (!*tok)
> +			return -EINVAL;
> +
> +		if (strcmp(tok, "simulate_cat_l3") == 0) {
> +			pr_info("Simulate CAT L3\n");
> +			rdt_opts.simulate_cat_l3 = true;

So this goes into rdt_opts

> +		} else if (strcmp(tok, "disable_cat_l3") == 0) {
> +			pr_info("CAT L3 is disabled\n");
> +			disable_cat_l3 = true;

While this is a distinct control variable. Very consistent.

> +		} else {
> +			pr_info("Invalid rdt option\n");

Very helpful w/o printing the actual option ....

> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +__setup("resctrl=", rdt_setup);
> +
> +static inline bool resource_alloc_enabled(void)
> +{
> +	return cat_l3_enabled;
> +}

Oh well.

> +
> +static int shared_domain_init(void)
> +{
> +	int l3_domain_num = get_domain_num(CACHE_LEVEL3);
> +	int size;
> +	int domain;
> +	struct cpumask *cpumask;
> +	struct cpumask *shared_cpu_map;
> +	int cpu;

More random variable declarations.

> +	if (cat_l3_enabled) {
> +		shared_domain_num = l3_domain_num;
> +		cpumask = &rdt_l3_cpumask;
> +	} else
> +		return -EINVAL;

Missing curly braces.

> +
> +	size = shared_domain_num * sizeof(struct shared_domain);
> +	shared_domain = kzalloc(size, GFP_KERNEL);
> +	if (!shared_domain)
> +		return -EINVAL;
> +
> +	domain = 0;
> +	for_each_cpu(cpu, cpumask) {
> +		if (cat_l3_enabled)
> +			shared_domain[domain].l3_domain =
> +					per_cpu(cpu_l3_domain, cpu);
> +		else
> +			shared_domain[domain].l3_domain = -1;
> +
> +		shared_cpu_map = get_shared_cpu_map(cpu, CACHE_LEVEL3);
> +
> +		cpumask_copy(&shared_domain[domain].cpumask, shared_cpu_map);

What's the point of updating the cpumask when the thing is disabled? If
there is a reason then this should be documented in a comment.

> +		domain++;
> +	}
> +	for_each_online_cpu(cpu) {
> +		if (cat_l3_enabled)
> +			per_cpu(cpu_shared_domain, cpu) =
> +					per_cpu(cpu_l3_domain, cpu);

More missing curly braces. And using an intermediate variable would remove
this hard to read line breaks.

> +	}
> +
> +	return 0;
> +}
> +
> +static int cconfig_init(int maxid)
> +{
> +	int num;
> +	int domain;
> +	unsigned long *closmap_block;
> +	int maxid_size;
> +
> +	maxid_size = BITS_TO_LONGS(maxid);
> +	num = maxid_size * shared_domain_num;
> +	cconfig.closmap = kcalloc(maxid, sizeof(unsigned long *), GFP_KERNEL);

Really intuitive. You calc num right before allocating the closmap pointers
and then you use it in the next alloc.

> +	if (!cconfig.closmap)
> +		goto out_free;
> +
> +	closmap_block = kcalloc(num, sizeof(unsigned long), GFP_KERNEL);
> +	if (!closmap_block)
> +		goto out_free;
> +
> +	for (domain = 0; domain < shared_domain_num; domain++)
> +		cconfig.closmap[domain] = (unsigned long *)closmap_block +

More random type casting.

> +					  domain * maxid_size;

Why don't you allocate that whole mess in one go?

	unsigned int ptrsize, mapsize, size, d;
    	void *p;

	ptrsize = maxid * sizeof(unsigned long *);
    	mapsize = BITS_TO_LONGS(maxid) * sizeof(unsigned long);
    	size = ptrsize + num_shared_domains * mapsize;

	p = kzalloc(size, GFP_KERNEL);
	if (!p)
		return -ENOMEM;

	cconfig.closmap = p;	
	cconfig.max_closid = maxid;

	p += ptrsize;
	for (d = 0; d < num_shared_domains; d++, p += mapsize)
	       cconfig.closmap[d] = p;
	return 0;

Would be too simple. Once more.

> +
> +	cconfig.max_closid = maxid;
> +
> +	return 0;
> +out_free:
> +	kfree(cconfig.closmap);
> +	kfree(closmap_block);
> +	return -ENOMEM;
> +}
> +
> +static int __init cat_cache_init(int level, int maxid,
> +				 struct clos_cbm_table ***cctable)
> +{
> +	int domain_num;
> +	int domain;
> +	int size;
> +	int ret = 0;
> +	struct clos_cbm_table *p;
> +
> +	domain_num = get_domain_num(level);
> +	size = domain_num * sizeof(struct clos_cbm_table *);
> +	*cctable = kzalloc(size, GFP_KERNEL);
> +	if (!*cctable) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	size = maxid * domain_num * sizeof(struct clos_cbm_table);
> +	p = kzalloc(size, GFP_KERNEL);
> +	if (!p) {
> +		kfree(*cctable);
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	for (domain = 0; domain < domain_num; domain++)
> +		(*cctable)[domain] = p + domain * maxid;

Same crap.

> +
> +	ret = cpu_cache_domain_init(level);
> +	if (ret) {
> +		kfree(*cctable);
> +		kfree(p);
> +	}
> +out:
> +	return ret;
>  }
>  
>  static int __init intel_rdt_late_init(void)
>  {
>  	struct cpuinfo_x86 *c = &boot_cpu_data;
>  	u32 maxid;
> -	int err = 0, size, i;
> -
> -	maxid = c->x86_cache_max_closid;
> -
> -	size = maxid * sizeof(struct clos_cbm_table);
> -	cctable = kzalloc(size, GFP_KERNEL);
> -	if (!cctable) {
> -		err = -ENOMEM;
> -		goto out_err;
> +	int i;
> +	int ret;
> +
> +	if (unlikely(disable_cat_l3))

This inlikely is completely pointless. This is not a hotpath function. It
just makes the code harder to read.

> +		cat_l3_enabled = false;
> +	else if (cat_l3_supported(c))
> +		cat_l3_enabled = true;
> +	else if (rdt_opts.simulate_cat_l3 &&
> +		 get_cache_leaf(CACHE_LEVEL3, 0) >= 0)
> +		cat_l3_enabled = true;
> +	else
> +		cat_l3_enabled = false;

Please move that into resource_alloc_enabled() and make it readable.

> +	if (!resource_alloc_enabled())
> +		return -ENODEV;
> +
> +	if (rdt_opts.simulate_cat_l3) {
> +		boot_cpu_data.x86_l3_max_closid = 16;
> +		boot_cpu_data.x86_l3_max_cbm_len = 20;
> +	}
> +	for_each_online_cpu(i) {
> +		rdt_cpumask_update(&rdt_l3_cpumask, i, CACHE_LEVEL3);
>  	}
>  
> -	size = BITS_TO_LONGS(maxid) * sizeof(long);
> -	cconfig.closmap = kzalloc(size, GFP_KERNEL);
> -	if (!cconfig.closmap) {
> -		kfree(cctable);
> -		err = -ENOMEM;
> -		goto out_err;
> +	maxid = 0;
> +	if (cat_l3_enabled) {
> +		maxid = boot_cpu_data.x86_l3_max_closid;
> +		ret = cat_cache_init(CACHE_LEVEL3, maxid, &l3_cctable);
> +		if (ret)
> +			cat_l3_enabled = false;
>  	}
>  
> -	for_each_online_cpu(i)
> -		rdt_cpumask_update(i);
> +	if (!cat_l3_enabled)
> +		return -ENOSPC;

Huch? How do you get here when cat_l3_enabled is false?

> +
> +	ret = shared_domain_init();
> +	if (ret)
> +		return -ENODEV;

  Leaks closmaps

> +
> +	ret = cconfig_init(maxid);
> +	if (ret)
> +		return ret;

Leaks more stuff.

>  	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>  				"AP_INTEL_RDT_ONLINE",
>  				intel_rdt_online_cpu, intel_rdt_offline_cpu);

I still have not figured out how all that init scheme is working so that
you can use nocalls() for the hotplug registration. 

> -	if (err < 0)
> -		goto out_err;
> +	if (ret < 0)
> +		return ret;

And this as well.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]


#1478835 — [PATCH v2 20/33] x86/intel_rdt.h: Header for inter_rdt.c

From"Fenghua Yu" <fenghua.yu@intel.com>
Date2016-09-08 09:10 +0200
Subject[PATCH v2 20/33] x86/intel_rdt.h: Header for inter_rdt.c
Message-ID<sf1Dc-2F3-13@gated-at.bofh.it>
In reply to#1478824
From: Fenghua Yu <fenghua.yu@intel.com>

The header mainly provides functions to call from the user interface
file intel_rdt_rdtgroup.c.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/intel_rdt.h | 77 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 72 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index 4e05c6e..a4f794b 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -3,27 +3,94 @@
 
 #ifdef CONFIG_INTEL_RDT
 
+#include <linux/seq_file.h>
 #include <linux/jump_label.h>
 
-#define MAX_CBM_LENGTH			32
 #define IA32_L3_CBM_BASE		0xc90
-#define CBM_FROM_INDEX(x)		(IA32_L3_CBM_BASE + x)
-#define MSR_IA32_PQOS_CFG		0xc81
+#define L3_CBM_FROM_INDEX(x)		(IA32_L3_CBM_BASE + x)
+
+#define MSR_IA32_L3_QOS_CFG		0xc81
+
+enum resource_type {
+	RESOURCE_L3  = 0,
+	RESOURCE_NUM = 1,
+};
+
+#define MAX_CACHE_LEAVES        4
+#define MAX_CACHE_DOMAINS       64
+
+DECLARE_PER_CPU_READ_MOSTLY(int, cpu_l3_domain);
+DECLARE_PER_CPU_READ_MOSTLY(struct rdtgroup *, cpu_rdtgroup);
 
 extern struct static_key rdt_enable_key;
 void __intel_rdt_sched_in(void *dummy);
 
+extern bool cdp_enabled;
+
+struct rdt_opts {
+	bool cdp_enabled;
+	bool verbose;
+	bool simulate_cat_l3;
+};
+
+struct cache_domain {
+	cpumask_t shared_cpu_map[MAX_CACHE_DOMAINS];
+	unsigned int max_cache_domains_num;
+	unsigned int level;
+	unsigned int shared_cache_id[MAX_CACHE_DOMAINS];
+};
+
+extern struct rdt_opts rdt_opts;
+
 struct clos_cbm_table {
 	unsigned long cbm;
 	unsigned int clos_refcnt;
 };
 
 struct clos_config {
-	unsigned long *closmap;
+	unsigned long **closmap;
 	u32 max_closid;
-	u32 closids_used;
 };
 
+struct shared_domain {
+	struct cpumask cpumask;
+	int l3_domain;
+};
+
+#define for_each_cache_domain(domain, start_domain, max_domain)	\
+	for (domain = start_domain; domain < max_domain; domain++)
+
+extern struct clos_config cconfig;
+extern struct shared_domain *shared_domain;
+extern int shared_domain_num;
+
+extern struct rdtgroup *root_rdtgrp;
+
+extern struct clos_cbm_table **l3_cctable;
+
+extern unsigned int min_bitmask_len;
+extern void msr_cpu_update(void *arg);
+extern inline void closid_get(u32 closid, int domain);
+extern void closid_put(u32 closid, int domain);
+extern void closid_free(u32 closid, int domain, int level);
+extern int closid_alloc(u32 *closid, int domain);
+extern bool cat_l3_enabled;
+extern unsigned int get_domain_num(int level);
+extern struct shared_domain *shared_domain;
+extern int shared_domain_num;
+extern inline int get_dcbm_table_index(int x);
+extern inline int get_icbm_table_index(int x);
+
+extern int get_cache_leaf(int level, int cpu);
+
+extern void cbm_update_l3_msr(void *pindex);
+extern int level_to_leaf(int level);
+
+extern void init_msrs(bool cdpenabled);
+extern bool cat_enabled(int level);
+extern u64 max_cbm(int level);
+extern u32 max_cbm_len(int level);
+
 /*
  * intel_rdt_sched_in() - Writes the task's CLOSid to IA32_PQR_MSR
  *
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]


#1479170 — Re: [PATCH v2 20/33] x86/intel_rdt.h: Header for inter_rdt.c

FromThomas Gleixner <tglx@linutronix.de>
Date2016-09-08 14:40 +0200
SubjectRe: [PATCH v2 20/33] x86/intel_rdt.h: Header for inter_rdt.c
Message-ID<sf6My-5OD-3@gated-at.bofh.it>
In reply to#1478835
On Thu, 8 Sep 2016, Fenghua Yu wrote:

Subject: x86/intel_rdt.h: Header for inter_rdt.c

inter_rdt? I know about Inter Mailand ....

> The header mainly provides functions to call from the user interface
> file intel_rdt_rdtgroup.c.

What the heck? We do not introduce function prototypes and whatever crap
without an implementation. We add the stuff when we add a function or
implement something which needs a define/struct whatever.
 
> +enum resource_type {
> +	RESOURCE_L3  = 0,
> +	RESOURCE_NUM = 1,

Why does this need an explicit enum initialization?

> +};
> +
> +#define MAX_CACHE_LEAVES        4
> +#define MAX_CACHE_DOMAINS       64
> +
> +DECLARE_PER_CPU_READ_MOSTLY(int, cpu_l3_domain);
> +DECLARE_PER_CPU_READ_MOSTLY(struct rdtgroup *, cpu_rdtgroup);
>  
>  extern struct static_key rdt_enable_key;
>  void __intel_rdt_sched_in(void *dummy);
>  
> +extern bool cdp_enabled;
> +
> +struct rdt_opts {
> +	bool cdp_enabled;
> +	bool verbose;
> +	bool simulate_cat_l3;
> +};
> +
> +struct cache_domain {
> +	cpumask_t shared_cpu_map[MAX_CACHE_DOMAINS];
> +	unsigned int max_cache_domains_num;
> +	unsigned int level;
> +	unsigned int shared_cache_id[MAX_CACHE_DOMAINS];
> +};
> +
> +extern struct rdt_opts rdt_opts;
> +
>  struct clos_cbm_table {
>  	unsigned long cbm;
>  	unsigned int clos_refcnt;
>  };
>  
>  struct clos_config {
> -	unsigned long *closmap;
> +	unsigned long **closmap;
>  	u32 max_closid;
> -	u32 closids_used;
>  };
>  
> +struct shared_domain {
> +	struct cpumask cpumask;
> +	int l3_domain;
> +};
> +
> +#define for_each_cache_domain(domain, start_domain, max_domain)	\
> +	for (domain = start_domain; domain < max_domain; domain++)
> +
> +extern struct clos_config cconfig;
> +extern struct shared_domain *shared_domain;
> +extern int shared_domain_num;
> +
> +extern struct rdtgroup *root_rdtgrp;
> +
> +extern struct clos_cbm_table **l3_cctable;
> +
> +extern unsigned int min_bitmask_len;
> +extern void msr_cpu_update(void *arg);
> +extern inline void closid_get(u32 closid, int domain);

extern inline?

> +extern void closid_put(u32 closid, int domain);

That's declared static in the source, but sure you do not notice because
intel_rdc.c is not hooked up to the Makefile yet.....

> +extern void closid_free(u32 closid, int domain, int level);

is declared static inline ....

> +extern int closid_alloc(u32 *closid, int domain);

amd more of this crap to follow.

I explicitely asked you last time to do:

>> Which is not making the review any simpler. In order to understand the
>> modifications I have to go back and page in the original stuff from last
>> year once again. So I have to read the original patch first to
>> understand the modifications and then get the overall picture of the new
>> stuff. Please fold stuff back to the proper places so I can start
>> reviewing this thing under the new design idea instead of twisting my
>> brain around two designs.
 
And you replied:

> Ok. I will do that.

Actually you did the reverse. You introduced more crap in the original
patches. See 12/32 vs. the previous version 
http://marc.info/?l=linux-kernel&m=146836100821478

What's the value of mechanically split patches which cannot even compile on
their own? Nothing at all except creating the hell for reviewers.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]


Page 1 of 4  [1] 2 3 4  Next page →

Back to top | Article view | linux.kernel


csiph-web