2019-07-30 18:12:52

by Reinette Chatre

Subject: [PATCH V2 00/10] x86/CPU and x86/resctrl: Support pseudo-lock regions spanning L2 and L3 cache

Changes since V1:
- Rebase onto v5.3-rc2

Dear Maintainers,

Cache pseudo-locking involves preloading a region of physical memory into a
reserved portion of cache that no task or CPU can subsequently fill into and
from that point on will only serve cache hits. At this time it is only
possible to create cache pseudo-locked regions in either L2 or L3 cache,
supporting systems that support either L2 Cache Allocation Technology (CAT)
or L3 CAT because CAT is the mechanism used to manage reservations of cache
portions.

This series introduces support for cache pseudo-locked regions that can span
L2 and L3 cache in preparation for systems that may support CAT on L2 and
L3 cache. Only systems with L3 inclusive cache is supported at this time
because if the L3 cache is not inclusive then pseudo-locked memory within
the L3 cache would be evicted when migrated to L2. Because of this
constraint the first patch in this series introduces support in cacheinfo.c
for resctrl to discover if the L3 cache is inclusive. All other patches in
this series are to the resctrl subsystem.

In support of cache pseudo-locked regions spanning L2 and L3 cache the term
"cache pseudo-lock portion" is introduced. Each portion of a cache
pseudo-locked region spans one level of cache and a cache pseudo-locked
region can be made up of one or two cache pseudo-lock portions.

On systems supporting L2 and L3 CAT where L3 cache is inclusive it is
possible to create two types of pseudo-locked regions:
1) A pseudo-locked region spanning just L3 cache, consisting out of a
single pseudo-locked portion.
2) A pseudo-locked region spanning L2 and L3 cache, consisting out of two
pseudo-locked portions.

In an L3 inclusive cache system a L2 pseudo-locked portion is required to
be matched with an L3 pseudo-locked portion to prevent a cache line from
being evicted from L2 when it is evicted from L3.

Patches 2 to 8 to the resctrl subsystem are preparing for the new feature
and should result in no functional change, but some comments do refer to
the new feature. Support for pseudo-locked regions spanning L2 and L3 cache
is introduced in patches 9 and 10.

Your feedback will be greatly appreciated.

Regards,

Reinette

Reinette Chatre (10):
x86/CPU: Expose if cache is inclusive of lower level caches
x86/resctrl: Remove unnecessary size compute
x86/resctrl: Constrain C-states during pseudo-lock region init
x86/resctrl: Set cache line size using new utility
x86/resctrl: Associate pseudo-locked region's cache instance by id
x86/resctrl: Introduce utility to return pseudo-locked cache portion
x86/resctrl: Remove unnecessary pointer to pseudo-locked region
x86/resctrl: Support pseudo-lock regions spanning resources
x86/resctrl: Pseudo-lock portions of multiple resources
x86/resctrl: Only pseudo-lock L3 cache when inclusive

arch/x86/kernel/cpu/cacheinfo.c | 42 +-
arch/x86/kernel/cpu/resctrl/core.c | 7 -
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 37 +-
arch/x86/kernel/cpu/resctrl/internal.h | 39 +-
arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 444 +++++++++++++++++++---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 ++-
include/linux/cacheinfo.h | 4 +
7 files changed, 512 insertions(+), 122 deletions(-)

--
2.17.2

2019-07-30 18:13:03

by Reinette Chatre

Subject: [PATCH V2 02/10] x86/resctrl: Remove unnecessary size compute

Information about a cache pseudo-locked region is maintained in its
struct pseudo_lock_region. One of these properties is the size of the
region that is computed before it is created and does not change over
the pseudo-locked region's lifetime.

When displaying the size of the pseudo-locked region to the user it is
thus not necessary to compute the size again from other properties, it
could just be printed directly from its struct.

The code being changed is entered only when the resource group is
pseudo-locked. At this time the pseudo-locked region has already been
created and the size property would thus be valid.

Signed-off-by: Reinette Chatre <[email protected]>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index a46dee8e78db..3985097ce728 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1308,10 +1308,8 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
} else {
seq_printf(s, "%*s:", max_name_width,
rdtgrp->plr->r->name);
- size = rdtgroup_cbm_to_size(rdtgrp->plr->r,
- rdtgrp->plr->d,
- rdtgrp->plr->cbm);
- seq_printf(s, "%d=%u\n", rdtgrp->plr->d->id, size);
+ seq_printf(s, "%d=%u\n", rdtgrp->plr->d->id,
+ rdtgrp->plr->size);
}
goto out;
}
--
2.17.2

2019-07-30 18:13:04

by Reinette Chatre

Subject: [PATCH V2 07/10] x86/resctrl: Remove unnecessary pointer to pseudo-locked region

Each cache domain (struct rdt_domain) contains a pointer to a
pseudo-locked region that (if set) is associated with it. At the
same time each resource group (struct rdtgroup) also contains a
pointer to a pseudo-locked region that (if set) is associated with
it.

If a pointer from a cache domain to its pseudo-locked region is
maintained then multiple cache domains could point to a single
pseudo-locked region when a pseudo-locked region spans multiple
resources. Such an arrangement would make it harder to support the
current mechanism of iterating over cache domains in order to find all
pseudo-locked regions.

In preparation for pseudo-locked regions that could span multiple
resources the pointer from a cache domain to a pseudo-locked region is
removed. The pointer to a pseudo-locked region from a resource
group remains - when needing to process all pseudo-locked regions on the
system an iteration over all resource groups is used instead of an
iteration over all cache domains.

Signed-off-by: Reinette Chatre <[email protected]>
---
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 3 +-
arch/x86/kernel/cpu/resctrl/internal.h | 6 +--
arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 46 +++++++++++------------
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 8 ++--
4 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 072f584cb238..a0383ff80afe 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -217,7 +217,7 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,

if ((rdtgrp->mode == RDT_MODE_EXCLUSIVE ||
rdtgrp->mode == RDT_MODE_SHAREABLE) &&
- rdtgroup_cbm_overlaps_pseudo_locked(d, cbm_val)) {
+ rdtgroup_cbm_overlaps_pseudo_locked(r, d, cbm_val)) {
rdt_last_cmd_puts("CBM overlaps with pseudo-locked region\n");
return -EINVAL;
}
@@ -293,7 +293,6 @@ static int parse_line(char *line, struct rdt_resource *r,
rdtgrp->plr->r = r;
rdtgrp->plr->d_id = d->id;
rdtgrp->plr->cbm = d->new_ctrl;
- d->plr = rdtgrp->plr;
return 0;
}
goto next;
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index f17633cf4776..892f38899dda 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -309,7 +309,6 @@ struct mbm_state {
* @mbps_val: When mba_sc is enabled, this holds the bandwidth in MBps
* @new_ctrl: new ctrl value to be loaded
* @have_new_ctrl: did user provide new_ctrl for this domain
- * @plr: pseudo-locked region (if any) associated with domain
*/
struct rdt_domain {
struct list_head list;
@@ -326,7 +325,6 @@ struct rdt_domain {
u32 *mbps_val;
u32 new_ctrl;
bool have_new_ctrl;
- struct pseudo_lock_region *plr;
};

/**
@@ -567,7 +565,9 @@ enum rdtgrp_mode rdtgroup_mode_by_closid(int closid);
int rdtgroup_tasks_assigned(struct rdtgroup *r);
int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp);
-bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm);
+bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
+ struct rdt_domain *d,
+ unsigned long cbm);
u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d);
bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
int rdt_pseudo_lock_init(void);
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index f16702a076a3..733cb7f34948 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -272,17 +272,10 @@ static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
*/
static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
{
- struct rdt_domain *d;
-
plr->size = 0;
plr->line_size = 0;
kfree(plr->kmem);
plr->kmem = NULL;
- if (plr->r && plr->d_id >= 0) {
- d = rdt_find_domain(plr->r, plr->d_id, NULL);
- if (!IS_ERR_OR_NULL(d))
- d->plr = NULL;
- }
plr->r = NULL;
plr->d_id = -1;
plr->cbm = 0;
@@ -822,6 +815,7 @@ int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp)

/**
* rdtgroup_cbm_overlaps_pseudo_locked - Test if CBM or portion is pseudo-locked
+ * @r: RDT resource to which @d belongs
* @d: RDT domain
* @cbm: CBM to test
*
@@ -835,17 +829,17 @@ int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp)
* Return: true if @cbm overlaps with pseudo-locked region on @d, false
* otherwise.
*/
-bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm)
+bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
+ struct rdt_domain *d,
+ unsigned long cbm)
{
+ unsigned long pseudo_locked;
unsigned int cbm_len;
- unsigned long cbm_b;

- if (d->plr) {
- cbm_len = d->plr->r->cache.cbm_len;
- cbm_b = d->plr->cbm;
- if (bitmap_intersects(&cbm, &cbm_b, cbm_len))
- return true;
- }
+ pseudo_locked = rdtgroup_pseudo_locked_bits(r, d);
+ cbm_len = r->cache.cbm_len;
+ if (bitmap_intersects(&cbm, &pseudo_locked, cbm_len))
+ return true;
return false;
}

@@ -859,13 +853,13 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm
* attempts to create new pseudo-locked regions in the same hierarchy.
*
* Return: true if a pseudo-locked region exists in the hierarchy of @d or
- * if it is not possible to test due to memory allocation issue,
- * false otherwise.
+ * if it is not possible to test due to memory allocation or other
+ * failure, false otherwise.
*/
bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
{
cpumask_var_t cpu_with_psl;
- struct rdt_resource *r;
+ struct rdtgroup *rdtgrp;
struct rdt_domain *d_i;
bool ret = false;

@@ -876,11 +870,16 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
* First determine which cpus have pseudo-locked regions
* associated with them.
*/
- for_each_alloc_enabled_rdt_resource(r) {
- list_for_each_entry(d_i, &r->domains, list) {
- if (d_i->plr)
- cpumask_or(cpu_with_psl, cpu_with_psl,
- &d_i->cpu_mask);
+ list_for_each_entry(rdtgrp, &rdt_all_groups, rdtgroup_list) {
+ if (rdtgrp->plr && rdtgrp->plr->d_id >= 0) {
+ d_i = rdt_find_domain(rdtgrp->plr->r, rdtgrp->plr->d_id,
+ NULL);
+ if (IS_ERR_OR_NULL(d_i)) {
+ ret = true;
+ goto out;
+ }
+ cpumask_or(cpu_with_psl, cpu_with_psl,
+ &d_i->cpu_mask);
}
}

@@ -891,6 +890,7 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
if (cpumask_intersects(&d->cpu_mask, cpu_with_psl))
ret = true;

+out:
free_cpumask_var(cpu_with_psl);
return ret;
}
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index c4bf6ed8b031..0c1786f09963 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -845,8 +845,10 @@ static int rdt_bit_usage_show(struct kernfs_open_file *of,
break;
}
}
+
+ pseudo_locked = rdtgroup_pseudo_locked_bits(r, dom);
+
for (i = r->cache.cbm_len - 1; i >= 0; i--) {
- pseudo_locked = dom->plr ? dom->plr->cbm : 0;
hwb = test_bit(i, &hw_shareable);
swb = test_bit(i, &sw_shareable);
excl = test_bit(i, &exclusive);
@@ -2541,8 +2543,8 @@ static int __init_one_rdt_domain(struct rdt_domain *d, struct rdt_resource *r,
d->new_ctrl |= *ctrl | peer_ctl;
}
}
- if (d->plr && d->plr->cbm > 0)
- used_b |= d->plr->cbm;
+
+ used_b |= rdtgroup_pseudo_locked_bits(r, d);
unused_b = used_b ^ (BIT_MASK(r->cache.cbm_len) - 1);
unused_b &= BIT_MASK(r->cache.cbm_len) - 1;
d->new_ctrl |= unused_b;
--
2.17.2

2019-07-30 18:13:19

by Reinette Chatre

Subject: [PATCH V2 05/10] x86/resctrl: Associate pseudo-locked region's cache instance by id

The properties of a cache pseudo-locked region that are maintained in
its struct pseudo_lock_region include a pointer to the cache domain to
which it belongs. A cache domain is a structure that is associated with
a cache instance and when all CPUs associated with the cache instance go
offline the cache domain associated with it is removed. When a cache
domain is removed care is taken to not point to it anymore from the
pseudo-locked region and all possible references to this removed
information are ensured to be safe, often resulting in an error message
to the user.

Replace the cache domain pointer in the properties of the cache
pseudo-locked region with the actual cache ID to eliminate the special
care that needs to be taken when using this data while also making it
possible to keep displaying cache pseudo-locked region information to
the user even when the cache domain structure has been removed.
Associating the cache ID with the pseudo-locked region will also
simplify the restoration of the pseudo-locked region when the
cache domain is restored when the CPUs come back online.

Signed-off-by: Reinette Chatre <[email protected]>
---
arch/x86/kernel/cpu/resctrl/core.c | 7 -----
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 14 ++-------
arch/x86/kernel/cpu/resctrl/internal.h | 4 +--
arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 38 +++++++++++++++++------
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 21 +++++--------
5 files changed, 40 insertions(+), 44 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 03eb90d00af0..e043074a88cc 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -633,13 +633,6 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)
cancel_delayed_work(&d->cqm_limbo);
}

- /*
- * rdt_domain "d" is going to be freed below, so clear
- * its pointer from pseudo_lock_region struct.
- */
- if (d->plr)
- d->plr->d = NULL;
-
kfree(d->ctrl_val);
kfree(d->mbps_val);
bitmap_free(d->rmid_busy_llc);
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index efbd54cc4e69..072f584cb238 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -291,7 +291,7 @@ static int parse_line(char *line, struct rdt_resource *r,
* region and return.
*/
rdtgrp->plr->r = r;
- rdtgrp->plr->d = d;
+ rdtgrp->plr->d_id = d->id;
rdtgrp->plr->cbm = d->new_ctrl;
d->plr = rdtgrp->plr;
return 0;
@@ -471,16 +471,8 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
for_each_alloc_enabled_rdt_resource(r)
seq_printf(s, "%s:uninitialized\n", r->name);
} else if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
- if (!rdtgrp->plr->d) {
- rdt_last_cmd_clear();
- rdt_last_cmd_puts("Cache domain offline\n");
- ret = -ENODEV;
- } else {
- seq_printf(s, "%s:%d=%x\n",
- rdtgrp->plr->r->name,
- rdtgrp->plr->d->id,
- rdtgrp->plr->cbm);
- }
+ seq_printf(s, "%s:%d=%x\n", rdtgrp->plr->r->name,
+ rdtgrp->plr->d_id, rdtgrp->plr->cbm);
} else {
closid = rdtgrp->closid;
for_each_alloc_enabled_rdt_resource(r) {
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index e49b77283924..65f558a2e806 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -149,7 +149,7 @@ struct mongroup {
* struct pseudo_lock_region - pseudo-lock region information
* @r: RDT resource to which this pseudo-locked region
* belongs
- * @d: RDT domain to which this pseudo-locked region
+ * @d_id: ID of cache instance to which this pseudo-locked region
* belongs
* @cbm: bitmask of the pseudo-locked region
* @lock_thread_wq: waitqueue used to wait on the pseudo-locking thread
@@ -169,7 +169,7 @@ struct mongroup {
*/
struct pseudo_lock_region {
struct rdt_resource *r;
- struct rdt_domain *d;
+ int d_id;
u32 cbm;
wait_queue_head_t lock_thread_wq;
int thread_done;
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index 884976913326..e7d1fdd76161 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -272,14 +272,19 @@ static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
*/
static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
{
+ struct rdt_domain *d;
+
plr->size = 0;
plr->line_size = 0;
kfree(plr->kmem);
plr->kmem = NULL;
+ if (plr->r && plr->d_id >= 0) {
+ d = rdt_find_domain(plr->r, plr->d_id, NULL);
+ if (!IS_ERR_OR_NULL(d))
+ d->plr = NULL;
+ }
plr->r = NULL;
- if (plr->d)
- plr->d->plr = NULL;
- plr->d = NULL;
+ plr->d_id = -1;
plr->cbm = 0;
pseudo_lock_cstates_relax(plr);
plr->debugfs_dir = NULL;
@@ -305,10 +310,18 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
*/
static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
{
+ struct rdt_domain *d;
int ret;

/* Pick the first cpu we find that is associated with the cache. */
- plr->cpu = cpumask_first(&plr->d->cpu_mask);
+ d = rdt_find_domain(plr->r, plr->d_id, NULL);
+ if (IS_ERR_OR_NULL(d)) {
+ rdt_last_cmd_puts("Cache domain offline\n");
+ ret = -ENODEV;
+ goto out_region;
+ }
+
+ plr->cpu = cpumask_first(&d->cpu_mask);

if (!cpu_online(plr->cpu)) {
rdt_last_cmd_printf("CPU %u associated with cache not online\n",
@@ -324,9 +337,9 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
goto out_region;
}

- plr->size = rdtgroup_cbm_to_size(plr->r, plr->d, plr->cbm);
+ plr->size = rdtgroup_cbm_to_size(plr->r, d, plr->cbm);

- ret = pseudo_lock_cstates_constrain(plr, &plr->d->cpu_mask);
+ ret = pseudo_lock_cstates_constrain(plr, &d->cpu_mask);
if (ret < 0)
goto out_region;

@@ -358,6 +371,7 @@ static int pseudo_lock_init(struct rdtgroup *rdtgrp)

init_waitqueue_head(&plr->lock_thread_wq);
INIT_LIST_HEAD(&plr->pm_reqs);
+ plr->d_id = -1;
rdtgrp->plr = plr;
return 0;
}
@@ -1183,6 +1197,7 @@ static int pseudo_lock_measure_cycles(struct rdtgroup *rdtgrp, int sel)
{
struct pseudo_lock_region *plr = rdtgrp->plr;
struct task_struct *thread;
+ struct rdt_domain *d;
unsigned int cpu;
int ret = -1;

@@ -1194,13 +1209,14 @@ static int pseudo_lock_measure_cycles(struct rdtgroup *rdtgrp, int sel)
goto out;
}

- if (!plr->d) {
+ d = rdt_find_domain(plr->r, plr->d_id, NULL);
+ if (IS_ERR_OR_NULL(d)) {
ret = -ENODEV;
goto out;
}

plr->thread_done = 0;
- cpu = cpumask_first(&plr->d->cpu_mask);
+ cpu = cpumask_first(&d->cpu_mask);
if (!cpu_online(cpu)) {
ret = -ENODEV;
goto out;
@@ -1497,6 +1513,7 @@ static int pseudo_lock_dev_mmap(struct file *filp, struct vm_area_struct *vma)
struct pseudo_lock_region *plr;
struct rdtgroup *rdtgrp;
unsigned long physical;
+ struct rdt_domain *d;
unsigned long psize;

mutex_lock(&rdtgroup_mutex);
@@ -1510,7 +1527,8 @@ static int pseudo_lock_dev_mmap(struct file *filp, struct vm_area_struct *vma)

plr = rdtgrp->plr;

- if (!plr->d) {
+ d = rdt_find_domain(plr->r, plr->d_id, NULL);
+ if (IS_ERR_OR_NULL(d)) {
mutex_unlock(&rdtgroup_mutex);
return -ENODEV;
}
@@ -1521,7 +1539,7 @@ static int pseudo_lock_dev_mmap(struct file *filp, struct vm_area_struct *vma)
* may be scheduled elsewhere and invalidate entries in the
* pseudo-locked region.
*/
- if (!cpumask_subset(current->cpus_ptr, &plr->d->cpu_mask)) {
+ if (!cpumask_subset(current->cpus_ptr, &d->cpu_mask)) {
mutex_unlock(&rdtgroup_mutex);
return -EINVAL;
}
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 3985097ce728..c4bf6ed8b031 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -262,22 +262,23 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
struct seq_file *s, void *v)
{
struct rdtgroup *rdtgrp;
- struct cpumask *mask;
+ struct rdt_domain *d;
int ret = 0;

rdtgrp = rdtgroup_kn_lock_live(of->kn);

if (rdtgrp) {
if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
- if (!rdtgrp->plr->d) {
+ d = rdt_find_domain(rdtgrp->plr->r, rdtgrp->plr->d_id,
+ NULL);
+ if (IS_ERR_OR_NULL(d)) {
rdt_last_cmd_clear();
rdt_last_cmd_puts("Cache domain offline\n");
ret = -ENODEV;
} else {
- mask = &rdtgrp->plr->d->cpu_mask;
seq_printf(s, is_cpu_list(of) ?
"%*pbl\n" : "%*pb\n",
- cpumask_pr_args(mask));
+ cpumask_pr_args(&d->cpu_mask));
}
} else {
seq_printf(s, is_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
@@ -1301,16 +1302,8 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
}

if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
- if (!rdtgrp->plr->d) {
- rdt_last_cmd_clear();
- rdt_last_cmd_puts("Cache domain offline\n");
- ret = -ENODEV;
- } else {
- seq_printf(s, "%*s:", max_name_width,
- rdtgrp->plr->r->name);
- seq_printf(s, "%d=%u\n", rdtgrp->plr->d->id,
- rdtgrp->plr->size);
- }
+ seq_printf(s, "%*s:", max_name_width, rdtgrp->plr->r->name);
+ seq_printf(s, "%d=%u\n", rdtgrp->plr->d_id, rdtgrp->plr->size);
goto out;
}

--
2.17.2

2019-07-30 18:13:22

by Reinette Chatre

Subject: [PATCH V2 06/10] x86/resctrl: Introduce utility to return pseudo-locked cache portion

To prevent eviction of pseudo-locked memory it is required that no
other resource group uses any portion of a cache that is in use by
a cache pseudo-locked region.

Introduce a utility that will return a Capacity BitMask (CBM) indicating
all portions of a provided cache instance being used for cache
pseudo-locking. This CBM can be used in overlap checking as well as
cache usage reporting.

Signed-off-by: Reinette Chatre <[email protected]>
---
arch/x86/kernel/cpu/resctrl/internal.h | 1 +
arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 65f558a2e806..f17633cf4776 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -568,6 +568,7 @@ int rdtgroup_tasks_assigned(struct rdtgroup *r);
int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp);
bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm);
+u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d);
bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
int rdt_pseudo_lock_init(void);
void rdt_pseudo_lock_release(void);
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index e7d1fdd76161..f16702a076a3 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -1626,3 +1626,26 @@ void rdt_pseudo_lock_release(void)
unregister_chrdev(pseudo_lock_major, "pseudo_lock");
pseudo_lock_major = 0;
}
+
+/**
+ * rdt_pseudo_locked_bits - Portions of cache instance used for pseudo-locking
+ * @r: RDT resource to which cache instance belongs
+ * @d: Cache instance
+ *
+ * Return: bits in CBM of @d that are used for cache pseudo-locking
+ */
+u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d)
+{
+ struct rdtgroup *rdtgrp;
+ u32 pseudo_locked = 0;
+
+ list_for_each_entry(rdtgrp, &rdt_all_groups, rdtgroup_list) {
+ if (!rdtgrp->plr)
+ continue;
+ if (rdtgrp->plr->r && rdtgrp->plr->r->rid == r->rid &&
+ rdtgrp->plr->d_id == d->id)
+ pseudo_locked |= rdtgrp->plr->cbm;
+ }
+
+ return pseudo_locked;
+}
--
2.17.2

2019-07-30 18:13:28

by Reinette Chatre

Subject: [PATCH V2 04/10] x86/resctrl: Set cache line size using new utility

In preparation for support of pseudo-locked regions spanning two
cache levels the cache line size computation is moved to a utility.

Setting of the cache line size is moved a few lines earlier, before
the C-states are constrained, to reduce the amount of cleanup needed
on failure.

Signed-off-by: Reinette Chatre <[email protected]>
---
arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 42 +++++++++++++++++------
1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index 110ae4b4f2e4..884976913326 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -101,6 +101,30 @@ static u64 get_prefetch_disable_bits(void)
return 0;
}

+/**
+ * get_cache_line_size - Determine the cache coherency line size
+ * @cpu: CPU with which cache is associated
+ * @level: Cache level
+ *
+ * Context: @cpu has to be online.
+ * Return: The cache coherency line size for cache level @level associated
+ * with CPU @cpu. Zero on failure.
+ */
+static unsigned int get_cache_line_size(unsigned int cpu, int level)
+{
+ struct cpu_cacheinfo *ci;
+ int i;
+
+ ci = get_cpu_cacheinfo(cpu);
+
+ for (i = 0; i < ci->num_leaves; i++) {
+ if (ci->info_list[i].level == level)
+ return ci->info_list[i].coherency_line_size;
+ }
+
+ return 0;
+}
+
/**
* pseudo_lock_minor_get - Obtain available minor number
* @minor: Pointer to where new minor number will be stored
@@ -281,9 +305,7 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
*/
static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
{
- struct cpu_cacheinfo *ci;
int ret;
- int i;

/* Pick the first cpu we find that is associated with the cache. */
plr->cpu = cpumask_first(&plr->d->cpu_mask);
@@ -295,7 +317,12 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
goto out_region;
}

- ci = get_cpu_cacheinfo(plr->cpu);
+ plr->line_size = get_cache_line_size(plr->cpu, plr->r->cache_level);
+ if (plr->line_size == 0) {
+ rdt_last_cmd_puts("Unable to determine cache line size\n");
+ ret = -1;
+ goto out_region;
+ }

plr->size = rdtgroup_cbm_to_size(plr->r, plr->d, plr->cbm);

@@ -303,15 +330,8 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
if (ret < 0)
goto out_region;

- for (i = 0; i < ci->num_leaves; i++) {
- if (ci->info_list[i].level == plr->r->cache_level) {
- plr->line_size = ci->info_list[i].coherency_line_size;
- return 0;
- }
- }
+ return 0;

- ret = -1;
- rdt_last_cmd_puts("Unable to determine cache line size\n");
out_region:
pseudo_lock_region_clear(plr);
return ret;
--
2.17.2

2019-07-30 19:56:05

by Reinette Chatre

Subject: [PATCH V2 08/10] x86/resctrl: Support pseudo-lock regions spanning resources

Currently cache pseudo-locked regions only consider one cache level but
cache pseudo-locked regions may span multiple cache levels.

In preparation for support of pseudo-locked regions spanning multiple
cache levels pseudo-lock 'portions' are introduced. A 'portion' of a
pseudo-locked region is the portion of a pseudo-locked region that
belongs to a specific resource. Each pseudo-locked portion is identified
with the resource (for example, L2 or L3 cache), the domain (the
specific cache instance), and the capacity bitmask that specifies which
region of the cache is used by the pseudo-locked region.

In support of pseudo-locked regions spanning multiple cache levels a
pseudo-locked region could have multiple 'portions' but in this
introduction only single portions are allowed.

Signed-off-by: Reinette Chatre <[email protected]>
---
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 26 +++-
arch/x86/kernel/cpu/resctrl/internal.h | 32 ++--
arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 180 ++++++++++++++++------
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 44 ++++--
4 files changed, 211 insertions(+), 71 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index a0383ff80afe..a60fb38a4d20 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -207,7 +207,7 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
* hierarchy.
*/
if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&
- rdtgroup_pseudo_locked_in_hierarchy(d)) {
+ rdtgroup_pseudo_locked_in_hierarchy(rdtgrp, d)) {
rdt_last_cmd_puts("Pseudo-locked region in hierarchy\n");
return -EINVAL;
}
@@ -282,6 +282,7 @@ static int parse_line(char *line, struct rdt_resource *r,
if (r->parse_ctrlval(&data, r, d))
return -EINVAL;
if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
+ struct pseudo_lock_portion *p;
/*
* In pseudo-locking setup mode and just
* parsed a valid CBM that should be
@@ -290,9 +291,15 @@ static int parse_line(char *line, struct rdt_resource *r,
* the required initialization for single
* region and return.
*/
- rdtgrp->plr->r = r;
- rdtgrp->plr->d_id = d->id;
- rdtgrp->plr->cbm = d->new_ctrl;
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
+ if (!p) {
+ rdt_last_cmd_puts("Unable to allocate memory for pseudo-lock portion\n");
+ return -ENOMEM;
+ }
+ p->r = r;
+ p->d_id = d->id;
+ p->cbm = d->new_ctrl;
+ list_add(&p->list, &rdtgrp->plr->portions);
return 0;
}
goto next;
@@ -410,8 +417,11 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
goto out;
}
ret = rdtgroup_parse_resource(resname, tok, rdtgrp);
- if (ret)
+ if (ret) {
+ if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP)
+ pseudo_lock_region_clear(rdtgrp->plr);
goto out;
+ }
}

for_each_alloc_enabled_rdt_resource(r) {
@@ -459,6 +469,7 @@ static void show_doms(struct seq_file *s, struct rdt_resource *r, int closid)
int rdtgroup_schemata_show(struct kernfs_open_file *of,
struct seq_file *s, void *v)
{
+ struct pseudo_lock_portion *p;
struct rdtgroup *rdtgrp;
struct rdt_resource *r;
int ret = 0;
@@ -470,8 +481,9 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
for_each_alloc_enabled_rdt_resource(r)
seq_printf(s, "%s:uninitialized\n", r->name);
} else if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
- seq_printf(s, "%s:%d=%x\n", rdtgrp->plr->r->name,
- rdtgrp->plr->d_id, rdtgrp->plr->cbm);
+ list_for_each_entry(p, &rdtgrp->plr->portions, list)
+ seq_printf(s, "%s:%d=%x\n", p->r->name, p->d_id,
+ p->cbm);
} else {
closid = rdtgrp->closid;
for_each_alloc_enabled_rdt_resource(r) {
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 892f38899dda..b041029d4de1 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -145,13 +145,27 @@ struct mongroup {
u32 rmid;
};

+/**
+ * struct pseudo_lock_portion - portion of a pseudo-lock region on one resource
+ * @r: RDT resource to which this pseudo-locked portion
+ * belongs
+ * @d_id: ID of cache instance to which this pseudo-locked portion
+ * belongs
+ * @cbm: bitmask of the pseudo-locked portion
+ * @list: Entry in the list of pseudo-locked portion
+ * belonging to the pseudo-locked region
+ */
+struct pseudo_lock_portion {
+ struct rdt_resource *r;
+ int d_id;
+ u32 cbm;
+ struct list_head list;
+};
+
/**
* struct pseudo_lock_region - pseudo-lock region information
- * @r: RDT resource to which this pseudo-locked region
- * belongs
- * @d_id: ID of cache instance to which this pseudo-locked region
- * belongs
- * @cbm: bitmask of the pseudo-locked region
+ * @portions: list of portions across different resources that
+ * are associated with this pseudo-locked region
* @lock_thread_wq: waitqueue used to wait on the pseudo-locking thread
* completion
* @thread_done: variable used by waitqueue to test if pseudo-locking
@@ -168,9 +182,7 @@ struct mongroup {
* @pm_reqs: Power management QoS requests related to this region
*/
struct pseudo_lock_region {
- struct rdt_resource *r;
- int d_id;
- u32 cbm;
+ struct list_head portions;
wait_queue_head_t lock_thread_wq;
int thread_done;
int cpu;
@@ -569,11 +581,13 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
struct rdt_domain *d,
unsigned long cbm);
u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d);
-bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
+bool rdtgroup_pseudo_locked_in_hierarchy(struct rdtgroup *selfgrp,
+ struct rdt_domain *d);
int rdt_pseudo_lock_init(void);
void rdt_pseudo_lock_release(void);
int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp);
void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp);
+void pseudo_lock_region_clear(struct pseudo_lock_region *plr);
struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r);
int update_domains(struct rdt_resource *r, int closid);
int closids_supported(void);
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index 733cb7f34948..717ea26e325b 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -270,28 +270,85 @@ static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
*
* Return: void
*/
-static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
+void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
{
+ struct pseudo_lock_portion *p, *tmp;
+
plr->size = 0;
plr->line_size = 0;
kfree(plr->kmem);
plr->kmem = NULL;
- plr->r = NULL;
- plr->d_id = -1;
- plr->cbm = 0;
pseudo_lock_cstates_relax(plr);
+ if (!list_empty(&plr->portions)) {
+ list_for_each_entry_safe(p, tmp, &plr->portions, list) {
+ list_del(&p->list);
+ kfree(p);
+ }
+ }
plr->debugfs_dir = NULL;
}

+/**
+ * pseudo_lock_single_portion_valid - Verify properties of pseudo-lock region
+ * @plr: the main pseudo-lock region
+ * @p: the single portion that makes up the pseudo-locked region
+ *
+ * Verify and initialize properties of the pseudo-locked region.
+ *
+ * Return: -1 if portion of cache unable to be used for pseudo-locking
+ * 0 if portion of cache can be used for pseudo-locking, in
+ * addition the CPU on which pseudo-locking will be performed will
+ * be initialized as well as the size and cache line size of the region
+ */
+static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
+ struct pseudo_lock_portion *p)
+{
+ struct rdt_domain *d;
+
+ d = rdt_find_domain(p->r, p->d_id, NULL);
+ if (IS_ERR_OR_NULL(d)) {
+ rdt_last_cmd_puts("Cannot find cache domain\n");
+ return -1;
+ }
+
+ plr->cpu = cpumask_first(&d->cpu_mask);
+ if (!cpu_online(plr->cpu)) {
+ rdt_last_cmd_printf("CPU %u not online\n", plr->cpu);
+ goto err_cpu;
+ }
+
+ plr->line_size = get_cache_line_size(plr->cpu, p->r->cache_level);
+ if (plr->line_size == 0) {
+ rdt_last_cmd_puts("Unable to compute cache line length\n");
+ goto err_cpu;
+ }
+
+ if (pseudo_lock_cstates_constrain(plr, &d->cpu_mask)) {
+ rdt_last_cmd_puts("Cannot limit C-states\n");
+ goto err_line;
+ }
+
+ plr->size = rdtgroup_cbm_to_size(p->r, d, p->cbm);
+
+ return 0;
+
+err_line:
+ plr->line_size = 0;
+err_cpu:
+ plr->cpu = 0;
+ return -1;
+}
+
/**
* pseudo_lock_region_init - Initialize pseudo-lock region information
* @plr: pseudo-lock region
*
* Called after user provided a schemata to be pseudo-locked. From the
* schemata the &struct pseudo_lock_region is on entry already initialized
- * with the resource, domain, and capacity bitmask. Here the information
- * required for pseudo-locking is deduced from this data and &struct
- * pseudo_lock_region initialized further. This information includes:
+ * with the resource, domain, and capacity bitmask. Here the
+ * provided data is validated and information required for pseudo-locking
+ * deduced, and &struct pseudo_lock_region initialized further. This
+ * information includes:
* - size in bytes of the region to be pseudo-locked
* - cache line size to know the stride with which data needs to be accessed
* to be pseudo-locked
@@ -303,44 +360,50 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
*/
static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
{
- struct rdt_domain *d;
+ struct rdt_resource *l3_resource = &rdt_resources_all[RDT_RESOURCE_L3];
+ struct pseudo_lock_portion *p;
int ret;

- /* Pick the first cpu we find that is associated with the cache. */
- d = rdt_find_domain(plr->r, plr->d_id, NULL);
- if (IS_ERR_OR_NULL(d)) {
- rdt_last_cmd_puts("Cache domain offline\n");
- ret = -ENODEV;
+ if (list_empty(&plr->portions)) {
+ rdt_last_cmd_puts("No pseudo-lock portions provided\n");
goto out_region;
}

- plr->cpu = cpumask_first(&d->cpu_mask);
-
- if (!cpu_online(plr->cpu)) {
- rdt_last_cmd_printf("CPU %u associated with cache not online\n",
- plr->cpu);
- ret = -ENODEV;
- goto out_region;
+ /* Cache Pseudo-Locking only supported on L2 and L3 resources */
+ list_for_each_entry(p, &plr->portions, list) {
+ if (p->r->rid != RDT_RESOURCE_L2 &&
+ p->r->rid != RDT_RESOURCE_L3) {
+ rdt_last_cmd_puts("Unsupported resource\n");
+ goto out_region;
+ }
}

- plr->line_size = get_cache_line_size(plr->cpu, plr->r->cache_level);
- if (plr->line_size == 0) {
- rdt_last_cmd_puts("Unable to determine cache line size\n");
- ret = -1;
- goto out_region;
+ /*
+ * If only one resource requested to be pseudo-locked then:
+ * - Just a L3 cache portion is valid
+ * - Just a L2 cache portion on system without L3 cache is valid
+ */
+ if (list_is_singular(&plr->portions)) {
+ p = list_first_entry(&plr->portions, struct pseudo_lock_portion,
+ list);
+ if (p->r->rid == RDT_RESOURCE_L3 ||
+ (p->r->rid == RDT_RESOURCE_L2 &&
+ !l3_resource->alloc_capable)) {
+ ret = pseudo_lock_single_portion_valid(plr, p);
+ if (ret < 0)
+ goto out_region;
+ return 0;
+ } else {
+ rdt_last_cmd_puts("Invalid resource or just L2 provided when L3 is required\n");
+ goto out_region;
+ }
+ } else {
+ rdt_last_cmd_puts("Multiple pseudo-lock portions unsupported\n");
}

- plr->size = rdtgroup_cbm_to_size(plr->r, d, plr->cbm);
-
- ret = pseudo_lock_cstates_constrain(plr, &d->cpu_mask);
- if (ret < 0)
- goto out_region;
-
- return 0;
-
out_region:
pseudo_lock_region_clear(plr);
- return ret;
+ return -1;
}

/**
@@ -362,9 +425,9 @@ static int pseudo_lock_init(struct rdtgroup *rdtgrp)
if (!plr)
return -ENOMEM;

+ INIT_LIST_HEAD(&plr->portions);
init_waitqueue_head(&plr->lock_thread_wq);
INIT_LIST_HEAD(&plr->pm_reqs);
- plr->d_id = -1;
rdtgrp->plr = plr;
return 0;
}
@@ -845,6 +908,7 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,

/**
* rdtgroup_pseudo_locked_in_hierarchy - Pseudo-locked region in cache hierarchy
+ * @selfgrp: current resource group testing for overlap
* @d: RDT domain under test
*
* The setup of a pseudo-locked region affects all cache instances within
@@ -856,8 +920,10 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
* if it is not possible to test due to memory allocation or other
* failure, false otherwise.
*/
-bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
+bool rdtgroup_pseudo_locked_in_hierarchy(struct rdtgroup *selfgrp,
+ struct rdt_domain *d)
{
+ struct pseudo_lock_portion *p;
cpumask_var_t cpu_with_psl;
struct rdtgroup *rdtgrp;
struct rdt_domain *d_i;
@@ -871,15 +937,15 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
* associated with them.
*/
list_for_each_entry(rdtgrp, &rdt_all_groups, rdtgroup_list) {
- if (rdtgrp->plr && rdtgrp->plr->d_id >= 0) {
- d_i = rdt_find_domain(rdtgrp->plr->r, rdtgrp->plr->d_id,
- NULL);
+ if (!rdtgrp->plr || rdtgrp == selfgrp)
+ continue;
+ list_for_each_entry(p, &rdtgrp->plr->portions, list) {
+ d_i = rdt_find_domain(p->r, p->d_id, NULL);
if (IS_ERR_OR_NULL(d_i)) {
ret = true;
goto out;
}
- cpumask_or(cpu_with_psl, cpu_with_psl,
- &d_i->cpu_mask);
+ cpumask_or(cpu_with_psl, cpu_with_psl, &d_i->cpu_mask);
}
}

@@ -1196,6 +1262,7 @@ static int measure_l3_residency(void *_plr)
static int pseudo_lock_measure_cycles(struct rdtgroup *rdtgrp, int sel)
{
struct pseudo_lock_region *plr = rdtgrp->plr;
+ struct pseudo_lock_portion *p;
struct task_struct *thread;
struct rdt_domain *d;
unsigned int cpu;
@@ -1209,7 +1276,16 @@ static int pseudo_lock_measure_cycles(struct rdtgroup *rdtgrp, int sel)
goto out;
}

- d = rdt_find_domain(plr->r, plr->d_id, NULL);
+ /*
+ * Ensure test is run on CPU associated with the pseudo-locked
+ * region. If pseudo-locked region spans L2 and L3, L2 portion
+ * would be first in the list and all CPUs associated with the
+ * L2 cache instance would be associated with pseudo-locked
+ * region.
+ */
+ p = list_first_entry(&plr->portions, struct pseudo_lock_portion, list);
+
+ d = rdt_find_domain(p->r, p->d_id, NULL);
if (IS_ERR_OR_NULL(d)) {
ret = -ENODEV;
goto out;
@@ -1511,6 +1587,7 @@ static int pseudo_lock_dev_mmap(struct file *filp, struct vm_area_struct *vma)
unsigned long vsize = vma->vm_end - vma->vm_start;
unsigned long off = vma->vm_pgoff << PAGE_SHIFT;
struct pseudo_lock_region *plr;
+ struct pseudo_lock_portion *p;
struct rdtgroup *rdtgrp;
unsigned long physical;
struct rdt_domain *d;
@@ -1527,7 +1604,16 @@ static int pseudo_lock_dev_mmap(struct file *filp, struct vm_area_struct *vma)

plr = rdtgrp->plr;

- d = rdt_find_domain(plr->r, plr->d_id, NULL);
+ /*
+ * If pseudo-locked region spans one cache level, there will be
+ * only one portion to consider. If pseudo-locked region spans two
+ * cache levels then the L2 cache portion will be the first entry
+ * and a CPU associated with it is what a task is required to be run
+ * on.
+ */
+ p = list_first_entry(&plr->portions, struct pseudo_lock_portion, list);
+
+ d = rdt_find_domain(p->r, p->d_id, NULL);
if (IS_ERR_OR_NULL(d)) {
mutex_unlock(&rdtgroup_mutex);
return -ENODEV;
@@ -1636,15 +1722,17 @@ void rdt_pseudo_lock_release(void)
*/
u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d)
{
+ struct pseudo_lock_portion *p;
struct rdtgroup *rdtgrp;
u32 pseudo_locked = 0;

list_for_each_entry(rdtgrp, &rdt_all_groups, rdtgroup_list) {
if (!rdtgrp->plr)
continue;
- if (rdtgrp->plr->r && rdtgrp->plr->r->rid == r->rid &&
- rdtgrp->plr->d_id == d->id)
- pseudo_locked |= rdtgrp->plr->cbm;
+ list_for_each_entry(p, &rdtgrp->plr->portions, list) {
+ if (p->r->rid == r->rid && p->d_id == d->id)
+ pseudo_locked |= p->cbm;
+ }
}

return pseudo_locked;
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 0c1786f09963..0039c8087548 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -269,17 +269,31 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,

if (rdtgrp) {
if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
- d = rdt_find_domain(rdtgrp->plr->r, rdtgrp->plr->d_id,
- NULL);
+ struct pseudo_lock_portion *p;
+
+ /*
+ * User space needs to know all CPUs associated
+ * with the pseudo-locked region. When the
+ * pseudo-locked region spans multiple resources it
+ * is possible that not all CPUs are associated with
+ * all portions of the pseudo-locked region.
+ * Only display CPUs that are associated with _all_
+ * portions of the region. The first portion in the
+ * list will be the L2 cache if the region spans
+ * multiple resource, it is thus only needed to
+ * print CPUs associated with the first portion.
+ */
+ p = list_first_entry(&rdtgrp->plr->portions,
+ struct pseudo_lock_portion, list);
+ d = rdt_find_domain(p->r, p->d_id, NULL);
if (IS_ERR_OR_NULL(d)) {
rdt_last_cmd_clear();
rdt_last_cmd_puts("Cache domain offline\n");
ret = -ENODEV;
- } else {
- seq_printf(s, is_cpu_list(of) ?
- "%*pbl\n" : "%*pb\n",
- cpumask_pr_args(&d->cpu_mask));
+ goto out;
}
+ seq_printf(s, is_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
+ cpumask_pr_args(&d->cpu_mask));
} else {
seq_printf(s, is_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
cpumask_pr_args(&rdtgrp->cpu_mask));
@@ -287,8 +301,9 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
} else {
ret = -ENOENT;
}
- rdtgroup_kn_unlock(of->kn);

+out:
+ rdtgroup_kn_unlock(of->kn);
return ret;
}

@@ -1304,8 +1319,19 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
}

if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
- seq_printf(s, "%*s:", max_name_width, rdtgrp->plr->r->name);
- seq_printf(s, "%d=%u\n", rdtgrp->plr->d_id, rdtgrp->plr->size);
+ struct pseudo_lock_portion *portion;
+
+ /*
+ * While the portions of the L2 and L3 caches allocated for a
+ * pseudo-locked region may be different, the size used for
+ * the pseudo-locked region is the same.
+ */
+ list_for_each_entry(portion, &rdtgrp->plr->portions, list) {
+ seq_printf(s, "%*s:", max_name_width,
+ portion->r->name);
+ seq_printf(s, "%d=%u\n", portion->d_id,
+ rdtgrp->plr->size);
+ }
goto out;
}

--
2.17.2

2019-07-30 20:16:02

by Reinette Chatre

Subject: [PATCH V2 03/10] x86/resctrl: Constrain C-states during pseudo-lock region init

CPUs associated with a pseudo-locked cache region are prevented
from entering C6 and deeper C-states to ensure that the
power savings associated with those C-states cannot impact
the pseudo-locked region by forcing the pseudo-locked memory to
be evicted.

When supporting pseudo-locked regions that span L2 and L3 cache
levels it is not necessary to prevent all CPUs associated with both
cache levels from entering deeper C-states. Instead, only the
CPUs associated with the L2 cache need to be limited. This would
potentially result in more power savings since another L2 cache
that does not have a pseudo-locked region but share the L3 cache
would be able to enter power savings.

In preparation for limiting the C-states only where required the code to
do so is moved to earlier in the pseudo-lock region initialization.
Moving this code has the consequence that its actions need to be undone
in more error paths. This is accommodated by moving the C-state cleanup
code to the generic cleanup code (pseudo_lock_region_clear()) and
ensuring that the C-state cleanup code can handle the case when C-states
have not yet been constrained.

Also in preparation for limiting the C-states only on required CPUs the
function now accepts a parameter that specifies which CPUs should have
their C-states constrained - at this time the parameter is still used
for all CPUs associated with the pseudo-locked region.

Signed-off-by: Reinette Chatre <[email protected]>
---
arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 30 ++++++++++++-----------
1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index d7623e1b927d..110ae4b4f2e4 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -175,6 +175,9 @@ static void pseudo_lock_cstates_relax(struct pseudo_lock_region *plr)
{
struct pseudo_lock_pm_req *pm_req, *next;

+ if (list_empty(&plr->pm_reqs))
+ return;
+
list_for_each_entry_safe(pm_req, next, &plr->pm_reqs, list) {
dev_pm_qos_remove_request(&pm_req->req);
list_del(&pm_req->list);
@@ -184,6 +187,8 @@ static void pseudo_lock_cstates_relax(struct pseudo_lock_region *plr)

/**
* pseudo_lock_cstates_constrain - Restrict cores from entering C6
+ * @plr: pseudo-lock region requiring the C-states to be restricted
+ * @cpu_mask: the CPUs that should have their C-states restricted
*
* To prevent the cache from being affected by power management entering
* C6 has to be avoided. This is accomplished by requesting a latency
@@ -197,13 +202,14 @@ static void pseudo_lock_cstates_relax(struct pseudo_lock_region *plr)
* may be set to map to deeper sleep states. In this case the latency
* requirement needs to prevent entering C2 also.
*/
-static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr)
+static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
+ struct cpumask *cpu_mask)
{
struct pseudo_lock_pm_req *pm_req;
int cpu;
int ret;

- for_each_cpu(cpu, &plr->d->cpu_mask) {
+ for_each_cpu(cpu, cpu_mask) {
pm_req = kzalloc(sizeof(*pm_req), GFP_KERNEL);
if (!pm_req) {
rdt_last_cmd_puts("Failure to allocate memory for PM QoS\n");
@@ -251,6 +257,7 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
plr->d->plr = NULL;
plr->d = NULL;
plr->cbm = 0;
+ pseudo_lock_cstates_relax(plr);
plr->debugfs_dir = NULL;
}

@@ -292,6 +299,10 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)

plr->size = rdtgroup_cbm_to_size(plr->r, plr->d, plr->cbm);

+ ret = pseudo_lock_cstates_constrain(plr, &plr->d->cpu_mask);
+ if (ret < 0)
+ goto out_region;
+
for (i = 0; i < ci->num_leaves; i++) {
if (ci->info_list[i].level == plr->r->cache_level) {
plr->line_size = ci->info_list[i].coherency_line_size;
@@ -1280,12 +1291,6 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
if (ret < 0)
return ret;

- ret = pseudo_lock_cstates_constrain(plr);
- if (ret < 0) {
- ret = -EINVAL;
- goto out_region;
- }
-
plr->thread_done = 0;

thread = kthread_create_on_node(pseudo_lock_fn, rdtgrp,
@@ -1294,7 +1299,7 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
if (IS_ERR(thread)) {
ret = PTR_ERR(thread);
rdt_last_cmd_printf("Locking thread returned error %d\n", ret);
- goto out_cstates;
+ goto out_region;
}

kthread_bind(thread, plr->cpu);
@@ -1312,13 +1317,13 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
* empty pseudo-locking loop.
*/
rdt_last_cmd_puts("Locking thread interrupted\n");
- goto out_cstates;
+ goto out_region;
}

ret = pseudo_lock_minor_get(&new_minor);
if (ret < 0) {
rdt_last_cmd_puts("Unable to obtain a new minor number\n");
- goto out_cstates;
+ goto out_region;
}

/*
@@ -1375,8 +1380,6 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
out_debugfs:
debugfs_remove_recursive(plr->debugfs_dir);
pseudo_lock_minor_release(new_minor);
-out_cstates:
- pseudo_lock_cstates_relax(plr);
out_region:
pseudo_lock_region_clear(plr);
out:
@@ -1410,7 +1413,6 @@ void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp)
goto free;
}

- pseudo_lock_cstates_relax(plr);
debugfs_remove_recursive(rdtgrp->plr->debugfs_dir);
device_destroy(pseudo_lock_class, MKDEV(pseudo_lock_major, plr->minor));
pseudo_lock_minor_release(plr->minor);
--
2.17.2

2019-07-30 23:13:18

by Reinette Chatre

Subject: Re: [PATCH V2 00/10] x86/CPU and x86/resctrl: Support pseudo-lock regions spanning L2 and L3 cache

Hi Thomas,

On 7/30/2019 1:00 PM, Thomas Gleixner wrote:
> On Tue, 30 Jul 2019, Reinette Chatre wrote:
>> Patches 2 to 8 to the resctrl subsystem are preparing for the new feature
>> and should result in no functional change, but some comments do refer to
>> the new feature. Support for pseudo-locked regions spanning L2 and L3 cache
>> is introduced in patches 9 and 10.
>>
>> Your feedback will be greatly appreciated.
>
> I've already skimmed V1 and did not find something horrible, but I want to
> hand the deeper review off to Borislav who should return from his well
> earned vacation soon.

Thank you very much. I look forward to working with Borislav on his return.

Reinette

2019-07-31 01:28:17

by Thomas Gleixner

Subject: Re: [PATCH V2 00/10] x86/CPU and x86/resctrl: Support pseudo-lock regions spanning L2 and L3 cache

Reinette,

On Tue, 30 Jul 2019, Reinette Chatre wrote:
> Patches 2 to 8 to the resctrl subsystem are preparing for the new feature
> and should result in no functional change, but some comments do refer to
> the new feature. Support for pseudo-locked regions spanning L2 and L3 cache
> is introduced in patches 9 and 10.
>
> Your feedback will be greatly appreciated.

I've already skimmed V1 and did not find something horrible, but I want to
hand the deeper review off to Borislav who should return from his well
earned vacation soon.

Thanks,

tglx

2019-08-05 15:58:42

by Borislav Petkov

Subject: Re: [PATCH V2 04/10] x86/resctrl: Set cache line size using new utility

On Tue, Jul 30, 2019 at 10:29:38AM -0700, Reinette Chatre wrote:
> In preparation for support of pseudo-locked regions spanning two
> cache levels the cache line size computation is moved to a utility.

Please write this in active voice: "Move the cache line size computation
to a utility function in preparation... "

And yes, "a utility" solely sounds like you're adding a tool which does
that. But it is simply a separate function. :-)

> Setting of the cache line size is moved a few lines earlier, before
> the C-states are constrained, to reduce the amount of cleanup needed
> on failure.

And in general, that passive voice is kinda hard to read. To quote from
Documentation/process/submitting-patches.rst:

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour."

Please check all your commit messages.

> Signed-off-by: Reinette Chatre <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 42 +++++++++++++++++------
> 1 file changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> index 110ae4b4f2e4..884976913326 100644
> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> @@ -101,6 +101,30 @@ static u64 get_prefetch_disable_bits(void)
> return 0;
> }
>
> +/**
> + * get_cache_line_size - Determine the cache coherency line size
> + * @cpu: CPU with which cache is associated
> + * @level: Cache level
> + *
> + * Context: @cpu has to be online.
> + * Return: The cache coherency line size for cache level @level associated
> + * with CPU @cpu. Zero on failure.
> + */
> +static unsigned int get_cache_line_size(unsigned int cpu, int level)
> +{
> + struct cpu_cacheinfo *ci;
> + int i;
> +
> + ci = get_cpu_cacheinfo(cpu);
> +
> + for (i = 0; i < ci->num_leaves; i++) {
> + if (ci->info_list[i].level == level)
> + return ci->info_list[i].coherency_line_size;
> + }
> +
> + return 0;
> +}
> +
> /**
> * pseudo_lock_minor_get - Obtain available minor number
> * @minor: Pointer to where new minor number will be stored
> @@ -281,9 +305,7 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
> */
> static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
> {
> - struct cpu_cacheinfo *ci;
> int ret;
> - int i;
>
> /* Pick the first cpu we find that is associated with the cache. */
> plr->cpu = cpumask_first(&plr->d->cpu_mask);
> @@ -295,7 +317,12 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
> goto out_region;
> }
>
> - ci = get_cpu_cacheinfo(plr->cpu);
> + plr->line_size = get_cache_line_size(plr->cpu, plr->r->cache_level);
> + if (plr->line_size == 0) {

if (!plr->...)

> + rdt_last_cmd_puts("Unable to determine cache line size\n");
> + ret = -1;
> + goto out_region;
> + }
>
> plr->size = rdtgroup_cbm_to_size(plr->r, plr->d, plr->cbm);
>
> @@ -303,15 +330,8 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
> if (ret < 0)
> goto out_region;
>
> - for (i = 0; i < ci->num_leaves; i++) {
> - if (ci->info_list[i].level == plr->r->cache_level) {
> - plr->line_size = ci->info_list[i].coherency_line_size;
> - return 0;
> - }
> - }
> + return 0;
>
> - ret = -1;
> - rdt_last_cmd_puts("Unable to determine cache line size\n");
> out_region:
> pseudo_lock_region_clear(plr);
> return ret;
> --
> 2.17.2
>

--
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

2019-08-05 16:09:26

by Borislav Petkov

Subject: Re: [PATCH V2 06/10] x86/resctrl: Introduce utility to return pseudo-locked cache portion

On Tue, Jul 30, 2019 at 10:29:40AM -0700, Reinette Chatre wrote:
> To prevent eviction of pseudo-locked memory it is required that no
> other resource group uses any portion of a cache that is in use by
> a cache pseudo-locked region.
>
> Introduce a utility that will return a Capacity BitMask (CBM) indicating
> all portions of a provided cache instance being used for cache
> pseudo-locking. This CBM can be used in overlap checking as well as
> cache usage reporting.
>
> Signed-off-by: Reinette Chatre <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
> arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 23 +++++++++++++++++++++++
> 2 files changed, 24 insertions(+)

s/utility/utility function/g

so that there's no ambiguity...

Thx.

--
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

2019-08-07 09:19:41

by Borislav Petkov

Subject: Re: [PATCH V2 08/10] x86/resctrl: Support pseudo-lock regions spanning resources

On Tue, Jul 30, 2019 at 10:29:42AM -0700, Reinette Chatre wrote:
> Currently cache pseudo-locked regions only consider one cache level but
> cache pseudo-locked regions may span multiple cache levels.
>
> In preparation for support of pseudo-locked regions spanning multiple
> cache levels pseudo-lock 'portions' are introduced. A 'portion' of a
> pseudo-locked region is the portion of a pseudo-locked region that
> belongs to a specific resource. Each pseudo-locked portion is identified
> with the resource (for example, L2 or L3 cache), the domain (the
> specific cache instance), and the capacity bitmask that specifies which
> region of the cache is used by the pseudo-locked region.
>
> In support of pseudo-locked regions spanning multiple cache levels a
> pseudo-locked region could have multiple 'portions' but in this
> introduction only single portions are allowed.
>
> Signed-off-by: Reinette Chatre <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 26 +++-
> arch/x86/kernel/cpu/resctrl/internal.h | 32 ++--
> arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 180 ++++++++++++++++------
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 44 ++++--
> 4 files changed, 211 insertions(+), 71 deletions(-)

This patch kinda got pretty big and is hard to review. Can
you split it pls? The addition of pseudo_lock_portion and
pseudo_lock_single_portion_valid() look like a separate patch to me.

> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index a0383ff80afe..a60fb38a4d20 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -207,7 +207,7 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
> * hierarchy.
> */
> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&
> - rdtgroup_pseudo_locked_in_hierarchy(d)) {
> + rdtgroup_pseudo_locked_in_hierarchy(rdtgrp, d)) {
> rdt_last_cmd_puts("Pseudo-locked region in hierarchy\n");
> return -EINVAL;
> }
> @@ -282,6 +282,7 @@ static int parse_line(char *line, struct rdt_resource *r,
> if (r->parse_ctrlval(&data, r, d))
> return -EINVAL;
> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> + struct pseudo_lock_portion *p;
> /*
> * In pseudo-locking setup mode and just
> * parsed a valid CBM that should be
> @@ -290,9 +291,15 @@ static int parse_line(char *line, struct rdt_resource *r,
> * the required initialization for single
> * region and return.
> */
> - rdtgrp->plr->r = r;
> - rdtgrp->plr->d_id = d->id;
> - rdtgrp->plr->cbm = d->new_ctrl;
> + p = kzalloc(sizeof(*p), GFP_KERNEL);
> + if (!p) {
> + rdt_last_cmd_puts("Unable to allocate memory for pseudo-lock portion\n");
> + return -ENOMEM;
> + }
> + p->r = r;
> + p->d_id = d->id;
> + p->cbm = d->new_ctrl;
> + list_add(&p->list, &rdtgrp->plr->portions);
> return 0;
> }

Looking at the indentation level of this, it is basically begging to
become a separate, helper function...

> goto next;
> @@ -410,8 +417,11 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
> goto out;
> }
> ret = rdtgroup_parse_resource(resname, tok, rdtgrp);
> - if (ret)
> + if (ret) {
> + if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP)
> + pseudo_lock_region_clear(rdtgrp->plr);
> goto out;
> + }
> }
>
> for_each_alloc_enabled_rdt_resource(r) {
> @@ -459,6 +469,7 @@ static void show_doms(struct seq_file *s, struct rdt_resource *r, int closid)
> int rdtgroup_schemata_show(struct kernfs_open_file *of,
> struct seq_file *s, void *v)
> {
> + struct pseudo_lock_portion *p;
> struct rdtgroup *rdtgrp;
> struct rdt_resource *r;
> int ret = 0;
> @@ -470,8 +481,9 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
> for_each_alloc_enabled_rdt_resource(r)
> seq_printf(s, "%s:uninitialized\n", r->name);
> } else if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
> - seq_printf(s, "%s:%d=%x\n", rdtgrp->plr->r->name,
> - rdtgrp->plr->d_id, rdtgrp->plr->cbm);
> + list_for_each_entry(p, &rdtgrp->plr->portions, list)
> + seq_printf(s, "%s:%d=%x\n", p->r->name, p->d_id,
> + p->cbm);

Shouldn't this say that those are portions now?

> } else {
> closid = rdtgrp->closid;
> for_each_alloc_enabled_rdt_resource(r) {
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 892f38899dda..b041029d4de1 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -145,13 +145,27 @@ struct mongroup {
> u32 rmid;
> };
>
> +/**
> + * struct pseudo_lock_portion - portion of a pseudo-lock region on one resource
> + * @r: RDT resource to which this pseudo-locked portion
> + * belongs
> + * @d_id: ID of cache instance to which this pseudo-locked portion
> + * belongs
> + * @cbm: bitmask of the pseudo-locked portion
> + * @list: Entry in the list of pseudo-locked portion
> + * belonging to the pseudo-locked region
> + */
> +struct pseudo_lock_portion {
> + struct rdt_resource *r;
> + int d_id;
> + u32 cbm;
> + struct list_head list;
> +};
> +
> /**
> * struct pseudo_lock_region - pseudo-lock region information
> - * @r: RDT resource to which this pseudo-locked region
> - * belongs
> - * @d_id: ID of cache instance to which this pseudo-locked region
> - * belongs
> - * @cbm: bitmask of the pseudo-locked region
> + * @portions: list of portions across different resources that
> + * are associated with this pseudo-locked region
> * @lock_thread_wq: waitqueue used to wait on the pseudo-locking thread
> * completion
> * @thread_done: variable used by waitqueue to test if pseudo-locking
> @@ -168,9 +182,7 @@ struct mongroup {
> * @pm_reqs: Power management QoS requests related to this region
> */
> struct pseudo_lock_region {
> - struct rdt_resource *r;
> - int d_id;
> - u32 cbm;
> + struct list_head portions;
> wait_queue_head_t lock_thread_wq;
> int thread_done;
> int cpu;
> @@ -569,11 +581,13 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
> struct rdt_domain *d,
> unsigned long cbm);
> u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d);
> -bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
> +bool rdtgroup_pseudo_locked_in_hierarchy(struct rdtgroup *selfgrp,
> + struct rdt_domain *d);
> int rdt_pseudo_lock_init(void);
> void rdt_pseudo_lock_release(void);
> int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp);
> void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp);
> +void pseudo_lock_region_clear(struct pseudo_lock_region *plr);
> struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r);
> int update_domains(struct rdt_resource *r, int closid);
> int closids_supported(void);
> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> index 733cb7f34948..717ea26e325b 100644
> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> @@ -270,28 +270,85 @@ static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
> *
> * Return: void
> */
> -static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
> +void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
> {
> + struct pseudo_lock_portion *p, *tmp;
> +
> plr->size = 0;
> plr->line_size = 0;
> kfree(plr->kmem);
> plr->kmem = NULL;
> - plr->r = NULL;
> - plr->d_id = -1;
> - plr->cbm = 0;
> pseudo_lock_cstates_relax(plr);
> + if (!list_empty(&plr->portions)) {
> + list_for_each_entry_safe(p, tmp, &plr->portions, list) {
> + list_del(&p->list);
> + kfree(p);
> + }
> + }
> plr->debugfs_dir = NULL;
> }
>
> +/**
> + * pseudo_lock_single_portion_valid - Verify properties of pseudo-lock region
> + * @plr: the main pseudo-lock region
> + * @p: the single portion that makes up the pseudo-locked region
> + *
> + * Verify and initialize properties of the pseudo-locked region.
> + *
> + * Return: -1 if portion of cache unable to be used for pseudo-locking
> + * 0 if portion of cache can be used for pseudo-locking, in
> + * addition the CPU on which pseudo-locking will be performed will
> + * be initialized as well as the size and cache line size of the region
> + */
> +static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
> + struct pseudo_lock_portion *p)
> +{
> + struct rdt_domain *d;
> +
> + d = rdt_find_domain(p->r, p->d_id, NULL);
> + if (IS_ERR_OR_NULL(d)) {
> + rdt_last_cmd_puts("Cannot find cache domain\n");
> + return -1;
> + }
> +
> + plr->cpu = cpumask_first(&d->cpu_mask);
> + if (!cpu_online(plr->cpu)) {
> + rdt_last_cmd_printf("CPU %u not online\n", plr->cpu);
> + goto err_cpu;
> + }
> +
> + plr->line_size = get_cache_line_size(plr->cpu, p->r->cache_level);
> + if (plr->line_size == 0) {
> + rdt_last_cmd_puts("Unable to compute cache line length\n");
> + goto err_cpu;
> + }
> +
> + if (pseudo_lock_cstates_constrain(plr, &d->cpu_mask)) {
> + rdt_last_cmd_puts("Cannot limit C-states\n");
> + goto err_line;
> + }
> +
> + plr->size = rdtgroup_cbm_to_size(p->r, d, p->cbm);
> +
> + return 0;
> +
> +err_line:
> + plr->line_size = 0;
> +err_cpu:
> + plr->cpu = 0;
> + return -1;
> +}
> +
> /**
> * pseudo_lock_region_init - Initialize pseudo-lock region information
> * @plr: pseudo-lock region
> *
> * Called after user provided a schemata to be pseudo-locked. From the
> * schemata the &struct pseudo_lock_region is on entry already initialized
> - * with the resource, domain, and capacity bitmask. Here the information
> - * required for pseudo-locking is deduced from this data and &struct
> - * pseudo_lock_region initialized further. This information includes:
> + * with the resource, domain, and capacity bitmask. Here the
> + * provided data is validated and information required for pseudo-locking
> + * deduced, and &struct pseudo_lock_region initialized further. This
> + * information includes:
> * - size in bytes of the region to be pseudo-locked
> * - cache line size to know the stride with which data needs to be accessed
> * to be pseudo-locked
> @@ -303,44 +360,50 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
> */
> static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
> {
> - struct rdt_domain *d;
> + struct rdt_resource *l3_resource = &rdt_resources_all[RDT_RESOURCE_L3];
> + struct pseudo_lock_portion *p;
> int ret;
>
> - /* Pick the first cpu we find that is associated with the cache. */
> - d = rdt_find_domain(plr->r, plr->d_id, NULL);
> - if (IS_ERR_OR_NULL(d)) {
> - rdt_last_cmd_puts("Cache domain offline\n");
> - ret = -ENODEV;
> + if (list_empty(&plr->portions)) {
> + rdt_last_cmd_puts("No pseudo-lock portions provided\n");
> goto out_region;

Not return?

Do you need to clear anything in an not even initialized plr?

> }
>
> - plr->cpu = cpumask_first(&d->cpu_mask);
> -
> - if (!cpu_online(plr->cpu)) {
> - rdt_last_cmd_printf("CPU %u associated with cache not online\n",
> - plr->cpu);
> - ret = -ENODEV;
> - goto out_region;
> + /* Cache Pseudo-Locking only supported on L2 and L3 resources */
> + list_for_each_entry(p, &plr->portions, list) {
> + if (p->r->rid != RDT_RESOURCE_L2 &&
> + p->r->rid != RDT_RESOURCE_L3) {
> + rdt_last_cmd_puts("Unsupported resource\n");
> + goto out_region;
> + }
> }
>
> - plr->line_size = get_cache_line_size(plr->cpu, plr->r->cache_level);
> - if (plr->line_size == 0) {
> - rdt_last_cmd_puts("Unable to determine cache line size\n");
> - ret = -1;
> - goto out_region;
> + /*
> + * If only one resource requested to be pseudo-locked then:
> + * - Just a L3 cache portion is valid
> + * - Just a L2 cache portion on system without L3 cache is valid
> + */
> + if (list_is_singular(&plr->portions)) {
> + p = list_first_entry(&plr->portions, struct pseudo_lock_portion,
> + list);

Let that line stick out.

> + if (p->r->rid == RDT_RESOURCE_L3 ||
> + (p->r->rid == RDT_RESOURCE_L2 &&
> + !l3_resource->alloc_capable)) {
> + ret = pseudo_lock_single_portion_valid(plr, p);
> + if (ret < 0)
> + goto out_region;
> + return 0;
> + } else {
> + rdt_last_cmd_puts("Invalid resource or just L2 provided when L3 is required\n");
> + goto out_region;
> + }
> + } else {
> + rdt_last_cmd_puts("Multiple pseudo-lock portions unsupported\n");
> }
>
> - plr->size = rdtgroup_cbm_to_size(plr->r, d, plr->cbm);
> -
> - ret = pseudo_lock_cstates_constrain(plr, &d->cpu_mask);
> - if (ret < 0)
> - goto out_region;
> -
> - return 0;
> -
> out_region:
> pseudo_lock_region_clear(plr);
> - return ret;
> + return -1;
> }
>

Yap, this patch is doing too many things at once and splitting it would help.

Thx.

--
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

2019-08-07 20:42:46

by Reinette Chatre

Subject: Re: [PATCH V2 08/10] x86/resctrl: Support pseudo-lock regions spanning resources

Hi Borislav,

On 8/7/2019 2:18 AM, Borislav Petkov wrote:
> On Tue, Jul 30, 2019 at 10:29:42AM -0700, Reinette Chatre wrote:
>> Currently cache pseudo-locked regions only consider one cache level but
>> cache pseudo-locked regions may span multiple cache levels.
>>
>> In preparation for support of pseudo-locked regions spanning multiple
>> cache levels pseudo-lock 'portions' are introduced. A 'portion' of a
>> pseudo-locked region is the portion of a pseudo-locked region that
>> belongs to a specific resource. Each pseudo-locked portion is identified
>> with the resource (for example, L2 or L3 cache), the domain (the
>> specific cache instance), and the capacity bitmask that specifies which
>> region of the cache is used by the pseudo-locked region.
>>
>> In support of pseudo-locked regions spanning multiple cache levels a
>> pseudo-locked region could have multiple 'portions' but in this
>> introduction only single portions are allowed.
>>

(Will fix to active voice)

>> Signed-off-by: Reinette Chatre <[email protected]>
>> ---
>> arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 26 +++-
>> arch/x86/kernel/cpu/resctrl/internal.h | 32 ++--
>> arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 180 ++++++++++++++++------
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 44 ++++--
>> 4 files changed, 211 insertions(+), 71 deletions(-)
>
> This patch kinda got pretty big and is hard to review. Can
> you split it pls? The addition of pseudo_lock_portion and
> pseudo_lock_single_portion_valid() look like a separate patch to me.

Sorry about the hard review. I'll split it up.

>
>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> index a0383ff80afe..a60fb38a4d20 100644
>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> @@ -207,7 +207,7 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
>> * hierarchy.
>> */
>> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&
>> - rdtgroup_pseudo_locked_in_hierarchy(d)) {
>> + rdtgroup_pseudo_locked_in_hierarchy(rdtgrp, d)) {
>> rdt_last_cmd_puts("Pseudo-locked region in hierarchy\n");
>> return -EINVAL;
>> }
>> @@ -282,6 +282,7 @@ static int parse_line(char *line, struct rdt_resource *r,
>> if (r->parse_ctrlval(&data, r, d))
>> return -EINVAL;
>> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>> + struct pseudo_lock_portion *p;
>> /*
>> * In pseudo-locking setup mode and just
>> * parsed a valid CBM that should be
>> @@ -290,9 +291,15 @@ static int parse_line(char *line, struct rdt_resource *r,
>> * the required initialization for single
>> * region and return.
>> */
>> - rdtgrp->plr->r = r;
>> - rdtgrp->plr->d_id = d->id;
>> - rdtgrp->plr->cbm = d->new_ctrl;
>> + p = kzalloc(sizeof(*p), GFP_KERNEL);
>> + if (!p) {
>> + rdt_last_cmd_puts("Unable to allocate memory for pseudo-lock portion\n");
>> + return -ENOMEM;
>> + }
>> + p->r = r;
>> + p->d_id = d->id;
>> + p->cbm = d->new_ctrl;
>> + list_add(&p->list, &rdtgrp->plr->portions);
>> return 0;
>> }
>
> Looking at the indentation level of this, it is basically begging to
> become a separate, helper function...

Will do.

>
>> goto next;
>> @@ -410,8 +417,11 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>> goto out;
>> }
>> ret = rdtgroup_parse_resource(resname, tok, rdtgrp);
>> - if (ret)
>> + if (ret) {
>> + if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP)
>> + pseudo_lock_region_clear(rdtgrp->plr);
>> goto out;
>> + }
>> }
>>
>> for_each_alloc_enabled_rdt_resource(r) {
>> @@ -459,6 +469,7 @@ static void show_doms(struct seq_file *s, struct rdt_resource *r, int closid)
>> int rdtgroup_schemata_show(struct kernfs_open_file *of,
>> struct seq_file *s, void *v)
>> {
>> + struct pseudo_lock_portion *p;
>> struct rdtgroup *rdtgrp;
>> struct rdt_resource *r;
>> int ret = 0;
>> @@ -470,8 +481,9 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
>> for_each_alloc_enabled_rdt_resource(r)
>> seq_printf(s, "%s:uninitialized\n", r->name);
>> } else if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
>> - seq_printf(s, "%s:%d=%x\n", rdtgrp->plr->r->name,
>> - rdtgrp->plr->d_id, rdtgrp->plr->cbm);
>> + list_for_each_entry(p, &rdtgrp->plr->portions, list)
>> + seq_printf(s, "%s:%d=%x\n", p->r->name, p->d_id,
>> + p->cbm);
>
> Shouldn't this say that those are portions now?

Are you suggesting new user output text? This is not needed. With this
change pseudo-locked resource groups can span more than one resource
(previously limited either L2 or L3). This change thus brings
pseudo-locked resource groups closer to regular resource groups that can
have allocations from multiple resources (and is supported by the
schemata file). A display of a non pseudo-lock resource group includes
all the resources supported, now the display of a pseudo-lock resource
group can include more than one resource similar to non pseudo-lock
resource groups.

Perhaps the naming ("portions") is causing confusing? Each "portion" is
the allocation of a resource that forms part of the pseudo-locked region.

>
>> } else {
>> closid = rdtgrp->closid;
>> for_each_alloc_enabled_rdt_resource(r) {
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 892f38899dda..b041029d4de1 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -145,13 +145,27 @@ struct mongroup {
>> u32 rmid;
>> };
>>
>> +/**
>> + * struct pseudo_lock_portion - portion of a pseudo-lock region on one resource
>> + * @r: RDT resource to which this pseudo-locked portion
>> + * belongs
>> + * @d_id: ID of cache instance to which this pseudo-locked portion
>> + * belongs
>> + * @cbm: bitmask of the pseudo-locked portion
>> + * @list: Entry in the list of pseudo-locked portion
>> + * belonging to the pseudo-locked region
>> + */
>> +struct pseudo_lock_portion {
>> + struct rdt_resource *r;
>> + int d_id;
>> + u32 cbm;
>> + struct list_head list;
>> +};
>> +
>> /**
>> * struct pseudo_lock_region - pseudo-lock region information
>> - * @r: RDT resource to which this pseudo-locked region
>> - * belongs
>> - * @d_id: ID of cache instance to which this pseudo-locked region
>> - * belongs
>> - * @cbm: bitmask of the pseudo-locked region
>> + * @portions: list of portions across different resources that
>> + * are associated with this pseudo-locked region
>> * @lock_thread_wq: waitqueue used to wait on the pseudo-locking thread
>> * completion
>> * @thread_done: variable used by waitqueue to test if pseudo-locking
>> @@ -168,9 +182,7 @@ struct mongroup {
>> * @pm_reqs: Power management QoS requests related to this region
>> */
>> struct pseudo_lock_region {
>> - struct rdt_resource *r;
>> - int d_id;
>> - u32 cbm;
>> + struct list_head portions;
>> wait_queue_head_t lock_thread_wq;
>> int thread_done;
>> int cpu;
>> @@ -569,11 +581,13 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
>> struct rdt_domain *d,
>> unsigned long cbm);
>> u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d);
>> -bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
>> +bool rdtgroup_pseudo_locked_in_hierarchy(struct rdtgroup *selfgrp,
>> + struct rdt_domain *d);
>> int rdt_pseudo_lock_init(void);
>> void rdt_pseudo_lock_release(void);
>> int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp);
>> void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp);
>> +void pseudo_lock_region_clear(struct pseudo_lock_region *plr);
>> struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r);
>> int update_domains(struct rdt_resource *r, int closid);
>> int closids_supported(void);
>> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> index 733cb7f34948..717ea26e325b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> @@ -270,28 +270,85 @@ static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
>> *
>> * Return: void
>> */
>> -static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
>> +void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
>> {
>> + struct pseudo_lock_portion *p, *tmp;
>> +
>> plr->size = 0;
>> plr->line_size = 0;
>> kfree(plr->kmem);
>> plr->kmem = NULL;
>> - plr->r = NULL;
>> - plr->d_id = -1;
>> - plr->cbm = 0;
>> pseudo_lock_cstates_relax(plr);
>> + if (!list_empty(&plr->portions)) {
>> + list_for_each_entry_safe(p, tmp, &plr->portions, list) {
>> + list_del(&p->list);
>> + kfree(p);
>> + }
>> + }
>> plr->debugfs_dir = NULL;
>> }
>>
>> +/**
>> + * pseudo_lock_single_portion_valid - Verify properties of pseudo-lock region
>> + * @plr: the main pseudo-lock region
>> + * @p: the single portion that makes up the pseudo-locked region
>> + *
>> + * Verify and initialize properties of the pseudo-locked region.
>> + *
>> + * Return: -1 if portion of cache unable to be used for pseudo-locking
>> + * 0 if portion of cache can be used for pseudo-locking, in
>> + * addition the CPU on which pseudo-locking will be performed will
>> + * be initialized as well as the size and cache line size of the region
>> + */
>> +static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
>> + struct pseudo_lock_portion *p)
>> +{
>> + struct rdt_domain *d;
>> +
>> + d = rdt_find_domain(p->r, p->d_id, NULL);
>> + if (IS_ERR_OR_NULL(d)) {
>> + rdt_last_cmd_puts("Cannot find cache domain\n");
>> + return -1;
>> + }
>> +
>> + plr->cpu = cpumask_first(&d->cpu_mask);
>> + if (!cpu_online(plr->cpu)) {
>> + rdt_last_cmd_printf("CPU %u not online\n", plr->cpu);
>> + goto err_cpu;
>> + }
>> +
>> + plr->line_size = get_cache_line_size(plr->cpu, p->r->cache_level);
>> + if (plr->line_size == 0) {
>> + rdt_last_cmd_puts("Unable to compute cache line length\n");
>> + goto err_cpu;
>> + }
>> +
>> + if (pseudo_lock_cstates_constrain(plr, &d->cpu_mask)) {
>> + rdt_last_cmd_puts("Cannot limit C-states\n");
>> + goto err_line;
>> + }
>> +
>> + plr->size = rdtgroup_cbm_to_size(p->r, d, p->cbm);
>> +
>> + return 0;
>> +
>> +err_line:
>> + plr->line_size = 0;
>> +err_cpu:
>> + plr->cpu = 0;
>> + return -1;
>> +}
>> +
>> /**
>> * pseudo_lock_region_init - Initialize pseudo-lock region information
>> * @plr: pseudo-lock region
>> *
>> * Called after user provided a schemata to be pseudo-locked. From the
>> * schemata the &struct pseudo_lock_region is on entry already initialized
>> - * with the resource, domain, and capacity bitmask. Here the information
>> - * required for pseudo-locking is deduced from this data and &struct
>> - * pseudo_lock_region initialized further. This information includes:
>> + * with the resource, domain, and capacity bitmask. Here the
>> + * provided data is validated and information required for pseudo-locking
>> + * deduced, and &struct pseudo_lock_region initialized further. This
>> + * information includes:
>> * - size in bytes of the region to be pseudo-locked
>> * - cache line size to know the stride with which data needs to be accessed
>> * to be pseudo-locked
>> @@ -303,44 +360,50 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
>> */
>> static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
>> {
>> - struct rdt_domain *d;
>> + struct rdt_resource *l3_resource = &rdt_resources_all[RDT_RESOURCE_L3];
>> + struct pseudo_lock_portion *p;
>> int ret;
>>
>> - /* Pick the first cpu we find that is associated with the cache. */
>> - d = rdt_find_domain(plr->r, plr->d_id, NULL);
>> - if (IS_ERR_OR_NULL(d)) {
>> - rdt_last_cmd_puts("Cache domain offline\n");
>> - ret = -ENODEV;
>> + if (list_empty(&plr->portions)) {
>> + rdt_last_cmd_puts("No pseudo-lock portions provided\n");
>> goto out_region;
>
> Not return?
>
> Do you need to clear anything in an not even initialized plr?

At this point the plr has been initialized with the parameters of the
region requested by the user within rdtgroup_schemata_write() - at this
time the call that triggered this function from here
(rdtgroup_schemata_write()->rdtgroup_pseudo_lock_create()) does not do
cleanup of this data on failure and relies on the cleanup done earlier.

I agree that this is confusing by not being symmetrical. I'll rework this.

>
>> }
>>
>> - plr->cpu = cpumask_first(&d->cpu_mask);
>> -
>> - if (!cpu_online(plr->cpu)) {
>> - rdt_last_cmd_printf("CPU %u associated with cache not online\n",
>> - plr->cpu);
>> - ret = -ENODEV;
>> - goto out_region;
>> + /* Cache Pseudo-Locking only supported on L2 and L3 resources */
>> + list_for_each_entry(p, &plr->portions, list) {
>> + if (p->r->rid != RDT_RESOURCE_L2 &&
>> + p->r->rid != RDT_RESOURCE_L3) {
>> + rdt_last_cmd_puts("Unsupported resource\n");
>> + goto out_region;
>> + }
>> }
>>
>> - plr->line_size = get_cache_line_size(plr->cpu, plr->r->cache_level);
>> - if (plr->line_size == 0) {
>> - rdt_last_cmd_puts("Unable to determine cache line size\n");
>> - ret = -1;
>> - goto out_region;
>> + /*
>> + * If only one resource requested to be pseudo-locked then:
>> + * - Just a L3 cache portion is valid
>> + * - Just a L2 cache portion on system without L3 cache is valid
>> + */
>> + if (list_is_singular(&plr->portions)) {
>> + p = list_first_entry(&plr->portions, struct pseudo_lock_portion,
>> + list);
>
> Let that line stick out.
>
>> + if (p->r->rid == RDT_RESOURCE_L3 ||
>> + (p->r->rid == RDT_RESOURCE_L2 &&
>> + !l3_resource->alloc_capable)) {
>> + ret = pseudo_lock_single_portion_valid(plr, p);
>> + if (ret < 0)
>> + goto out_region;
>> + return 0;
>> + } else {
>> + rdt_last_cmd_puts("Invalid resource or just L2 provided when L3 is required\n");
>> + goto out_region;
>> + }
>> + } else {
>> + rdt_last_cmd_puts("Multiple pseudo-lock portions unsupported\n");
>> }
>>
>> - plr->size = rdtgroup_cbm_to_size(plr->r, d, plr->cbm);
>> -
>> - ret = pseudo_lock_cstates_constrain(plr, &d->cpu_mask);
>> - if (ret < 0)
>> - goto out_region;
>> -
>> - return 0;
>> -
>> out_region:
>> pseudo_lock_region_clear(plr);
>> - return ret;
>> + return -1;
>> }
>>
>
> Yap, this patch is doing too many things at once and splitting it would help.

Will do. Thank you very much.

Reinette