[PATCH 0/3] x86/intel_rdt: Fix exclusive mode with CDP resources

[PATCH 0/3] x86/intel_rdt: Fix exclusive mode with CDP resources

[Reinette Chatre]

Dear Maintainers,

CDP resources do not currently behave as expected when there are resource
groups with mode 'exclusive'.

In the example below it was possible to create two resource groups,
p1 and p2, that are both in exclusive mode but their usage of the
underlying L2 cache actually overlaps.

root@glk:/sys/fs/resctrl# ls
cpus  cpus_list  info  mode  p1  p2  schemata  size  tasks
root@glk:/sys/fs/resctrl# cat schemata
L2DATA:0=fff0
L2CODE:0=fff0
root@glk:/sys/fs/resctrl# cat mode
shareable

root@glk:/sys/fs/resctrl# cat p1/schemata
L2DATA:0=0003
L2CODE:0=000c
root@glk:/sys/fs/resctrl# cat p1/mode
exclusive

root@glk:/sys/fs/resctrl# cat p2/schemata
L2DATA:0=000c
L2CODE:0=0003
root@glk:/sys/fs/resctrl# cat p2/mode
exclusive

root@glk:/sys/fs/resctrl# cat info/L2CODE/bit_usage
0=SSSSSSSSSSSSEEEE
root@glk:/sys/fs/resctrl# cat info/L2DATA/bit_usage
0=SSSSSSSSSSSSEEEE
root@glk:/sys/fs/resctrl#

In the above example, the CBM of L2DATA in p1 overlaps with the CBM of
L2CODE in p2 while they are both in exclusive mode. While it may reflect no
overlap among the L2DATA resources specifically it does actually imply
overlap of use of the underlying hardware that is not the intention of
'exclusive' mode. This happens because the current implementation treats
L2CODE and L2DATA as totally independent, when it is actually referring to
the same underlying hardware.

This series fixes the potential for overlap of hardware resource use when
resource groups are in 'exclusive' mode by ensuring that if there is a CDP
peer on the same hardware then any overlap test would consider it also.

Allocations of data and code resources within the same exclusive resource
group are allowed to overlap.

Your feedback will be greatly appreciated.

Reinette

Reinette Chatre (3):
  x86/intel_rdt: Introduce utility to obtain CDP peer
  x86/intel_rdt: CBM overlap should also check for overlap with CDP peer
  x86/intel_rdt: Fix initial allocation to consider CDP

 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 133 ++++++++++++++++++++++-
 1 file changed, 128 insertions(+), 5 deletions(-)

-- 
2.17.0

[PATCH 1/3] x86/intel_rdt: Introduce utility to obtain CDP peer

[Reinette Chatre]

Introduce a utility that, when provided with a RDT resource and an
instance of this RDT resource (a RDT domain), would return pointers to
the RDT resource and RDT domain that share the same hardware. This is
specific to the CDP resources that share the same hardware.

For example, if a pointer to the RDT_RESOURCE_L2DATA resource (struct
rdt_resource) and a pointer to an instance of this resource (struct
rdt_domain) is provided, then it will return a pointer to the
RDT_RESOURCE_L2CODE resource as well as the specific instance that
shares the same hardware as the provided rdt_domain.

This utility is created in support of the "exclusive" resource group
mode where overlap of resource allocation between resource groups need
to be avoided. The overlap test need to consider not just the matching
resources, but also the resources that share the same hardware.

Temporarily mark it as unused in support of patch testing to avoid
compile warnings until it is used.

Fixes: 49f7b4efa110 ("x86/intel_rdt: Enable setting of exclusive mode")
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
Tested-by: Jithu Joseph <jithu.joseph@intel.com>
Acked-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 72 ++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 1b8e86a5d5e1..c8c02c16d072 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -960,6 +960,78 @@ static int rdtgroup_mode_show(struct kernfs_open_file *of,
 	return 0;
 }
 
+/**
+ * rdt_cdp_peer_get - Retrieve CDP peer if it exists
+ * @r: RDT resource to which RDT domain @d belongs
+ * @d: Cache instance for which a CDP peer is requested
+ * @r_cdp: RDT resource that shares hardware with @r (RDT resource peer)
+ *         Used to return the result.
+ * @d_cdp: RDT domain that shares hardware with @d (RDT domain peer)
+ *         Used to return the result.
+ *
+ * RDT resources are managed independently and by extension the RDT domains
+ * (RDT resource instances) are managed independently also. The Code and
+ * Data Prioritization (CDP) RDT resources, while managed independently,
+ * could refer to the same underlying hardware. For example,
+ * RDT_RESOURCE_L2CODE and RDT_RESOURCE_L2DATA both refer to the L2 cache.
+ *
+ * When provided with an RDT resource @r and an instance of that RDT
+ * resource @d rdt_cdp_peer_get() will return if there is a peer RDT
+ * resource and the exact instance that shares the same hardware.
+ *
+ * Return: 0 if a CDP peer was found, <0 on error or if no CDP peer exists.
+ *         If a CDP peer was found, @r_cdp will point to the peer RDT resource
+ *         and @d_cdp will point to the peer RDT domain.
+ */
+static int __attribute__((unused)) rdt_cdp_peer_get(struct rdt_resource *r,
+						    struct rdt_domain *d,
+						    struct rdt_resource **r_cdp,
+						    struct rdt_domain **d_cdp)
+{
+	struct rdt_resource *_r_cdp = NULL;
+	struct rdt_domain *_d_cdp = NULL;
+	int ret = 0;
+
+	switch (r->rid) {
+	case RDT_RESOURCE_L3DATA:
+		_r_cdp = &rdt_resources_all[RDT_RESOURCE_L3CODE];
+		break;
+	case RDT_RESOURCE_L3CODE:
+		_r_cdp =  &rdt_resources_all[RDT_RESOURCE_L3DATA];
+		break;
+	case RDT_RESOURCE_L2DATA:
+		_r_cdp =  &rdt_resources_all[RDT_RESOURCE_L2CODE];
+		break;
+	case RDT_RESOURCE_L2CODE:
+		_r_cdp =  &rdt_resources_all[RDT_RESOURCE_L2DATA];
+		break;
+	default:
+		ret = -ENOENT;
+		goto out;
+	}
+
+	/*
+	 * When a new CPU comes online and CDP is enabled then the new
+	 * RDT domains (if any) associated with both CDP RDT resources
+	 * are added in the same CPU online routine while the
+	 * rdtgroup_mutex is held. It should thus not happen for one
+	 * RDT domain to exist and be associated with its RDT CDP
+	 * resource but there is no RDT domain associated with the
+	 * peer RDT CDP resource. Hence the WARN.
+	 */
+	_d_cdp = rdt_find_domain(_r_cdp, d->id, NULL);
+	if (WARN_ON(!_d_cdp)) {
+		_r_cdp = NULL;
+		ret = -ENOENT;
+	}
+
+out:
+	*r_cdp = _r_cdp;
+	*d_cdp = _d_cdp;
+
+	return ret;
+}
+
 /**
  * rdtgroup_cbm_overlaps - Does CBM for intended closid overlap with other
  * @r: Resource to which domain instance @d belongs.
-- 
2.17.0

[PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer

[Reinette Chatre]

The CBM overlap test is used to manage the allocations of RDT resources
where overlap is possible between resource groups. When a resource group
is in exclusive mode then there should be no overlap between resource
groups.

The current overlap test only considers overlap between the same
resources, for example, that usage of a RDT_RESOURCE_L2DATA resource
in one resource group does not overlap with usage of a RDT_RESOURCE_L2DATA
resource in another resource group. The problem with this is that it
allows overlap between a RDT_RESOURCE_L2DATA resource in one resource
group with a RDT_RESOURCE_L2CODE resource in another resource group -
even if both resource groups are in exclusive mode. This is a problem
because even though these appear to be different resources they end up
sharing the same underlying hardware and thus does not fulfill the
user's request for exclusive use of hardware resources.

Fix this by including the CDP peer (if there is one) in every CBM
overlap test. This does not impact the overlap between resources
within the same exclusive resource group that is allowed.

Fixes: 49f7b4efa110 ("x86/intel_rdt: Enable setting of exclusive mode")
Reported-by: Jithu Joseph <jithu.joseph@intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
Tested-by: Jithu Joseph <jithu.joseph@intel.com>
Acked-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 51 ++++++++++++++++++++----
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index c8c02c16d072..3f56b4e624ea 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -983,10 +983,9 @@ static int rdtgroup_mode_show(struct kernfs_open_file *of,
  *         If a CDP peer was found, @r_cdp will point to the peer RDT resource
  *         and @d_cdp will point to the peer RDT domain.
  */
-static int __attribute__((unused)) rdt_cdp_peer_get(struct rdt_resource *r,
-						    struct rdt_domain *d,
-						    struct rdt_resource **r_cdp,
-						    struct rdt_domain **d_cdp)
+static int rdt_cdp_peer_get(struct rdt_resource *r, struct rdt_domain *d,
+			    struct rdt_resource **r_cdp,
+			    struct rdt_domain **d_cdp)
 {
 	struct rdt_resource *_r_cdp = NULL;
 	struct rdt_domain *_d_cdp = NULL;
@@ -1033,7 +1032,7 @@ static int __attribute__((unused)) rdt_cdp_peer_get(struct rdt_resource *r,
 }
 
 /**
- * rdtgroup_cbm_overlaps - Does CBM for intended closid overlap with other
+ * _rdtgroup_cbm_overlaps - Does CBM for intended closid overlap with other
  * @r: Resource to which domain instance @d belongs.
  * @d: The domain instance for which @closid is being tested.
  * @cbm: Capacity bitmask being tested.
@@ -1049,8 +1048,8 @@ static int __attribute__((unused)) rdt_cdp_peer_get(struct rdt_resource *r,
  *
  * Return: false if CBM does not overlap, true if it does.
  */
-bool rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
-			   u32 _cbm, int closid, bool exclusive)
+static bool _rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
+				   u32 _cbm, int closid, bool exclusive)
 {
 	unsigned long *cbm = (unsigned long *)&_cbm;
 	unsigned long *ctrl_b;
@@ -1087,6 +1086,44 @@ bool rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
 	return false;
 }
 
+/**
+ * rdtgroup_cbm_overlaps - Does CBM overlap with other use of hardware
+ * @r: Resource to which domain instance @d belongs.
+ * @d: The domain instance for which @closid is being tested.
+ * @cbm: Capacity bitmask being tested.
+ * @closid: Intended closid for @cbm.
+ * @exclusive: Only check if overlaps with exclusive resource groups
+ *
+ * Resources that can be allocated using a CBM can use the CBM to control
+ * the overlap of these allocations. rdtgroup_cmb_overlaps() is the test
+ * for overlap. Overlap test is not limited to the specific resource for
+ * which the CBM is intended though - when dealing with CDP resources that
+ * share the underlying hardware the overlap check should be performed on
+ * the CDP resource sharing the hardware also.
+ *
+ * Refer to description of _rdtgroup_cbm_overlaps() for the details of the
+ * overlap test.
+ *
+ * Return: true if CBM overlap detected, false if there is no overlap
+ */
+bool rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
+			   u32 _cbm, int closid, bool exclusive)
+{
+	struct rdt_resource *r_cdp;
+	struct rdt_domain *d_cdp;
+	bool ret;
+
+	ret = _rdtgroup_cbm_overlaps(r, d, _cbm, closid, exclusive);
+	if (ret)
+		return ret;
+
+	if (rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp) == 0)
+		return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
+					      closid, exclusive);
+
+	return ret;
+}
+
 /**
  * rdtgroup_mode_test_exclusive - Test if this resource group can be exclusive
  *
-- 
2.17.0

[PATCH 3/3] x86/intel_rdt: Fix initial allocation to consider CDP

[Reinette Chatre]

When a new resource group is created it is initialized with a default
allocation that considers which portions of cache are currently
available for sharing across all resource groups or which portions of
cache are currently unused.

If a CDP allocation forms part of a resource group that is in exclusive
mode then it should be ensured that no new allocation overlaps with any
resource that shares the underlying hardware. The current initial
allocation does not take this sharing of hardware into account and
a new allocation in a resource that shares the same
hardware would affect the exclusive resource group.

Fix this by considering the allocation of a peer RDT domain - a RDT
domain sharing the same hardware - as part of the test to determine
which portion of cache is in use and available for use.

Fixes: 95f0b77efa57 ("x86/intel_rdt: Initialize new resource group with sane defaults")
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
Acked-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 3f56b4e624ea..5b7932af522a 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -2459,11 +2459,14 @@ static void cbm_ensure_valid(u32 *_val, struct rdt_resource *r)
  */
 static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
 {
+	struct rdt_resource *r_cdp = NULL;
+	struct rdt_domain *d_cdp = NULL;
 	u32 used_b = 0, unused_b = 0;
 	u32 closid = rdtgrp->closid;
 	struct rdt_resource *r;
 	enum rdtgrp_mode mode;
 	struct rdt_domain *d;
+	u32 peer_ctl;
 	int i, ret;
 	u32 *ctrl;
 
@@ -2475,6 +2478,7 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
 		if (r->rid == RDT_RESOURCE_MBA)
 			continue;
 		list_for_each_entry(d, &r->domains, list) {
+			rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp);
 			d->have_new_ctrl = false;
 			d->new_ctrl = r->cache.shareable_bits;
 			used_b = r->cache.shareable_bits;
@@ -2484,9 +2488,19 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
 					mode = rdtgroup_mode_by_closid(i);
 					if (mode == RDT_MODE_PSEUDO_LOCKSETUP)
 						break;
-					used_b |= *ctrl;
+					/*
+					 * If CDP is active include peer
+					 * domain's usage to ensure there
+					 * is no overlap with an exclusive
+					 * group.
+					 */
+					if (d_cdp)
+						peer_ctl = d_cdp->ctrl_val[i];
+					else
+						peer_ctl = 0;
+					used_b |= *ctrl | peer_ctl;
 					if (mode == RDT_MODE_SHAREABLE)
-						d->new_ctrl |= *ctrl;
+						d->new_ctrl |= *ctrl | peer_ctl;
 				}
 			}
 			if (d->plr && d->plr->cbm > 0)
-- 
2.17.0

Re: [PATCH 1/3] x86/intel_rdt: Introduce utility to obtain CDP peer

[Thomas Gleixner]

On Wed, 26 Sep 2018, Reinette Chatre wrote:
> + * Return: 0 if a CDP peer was found, <0 on error or if no CDP peer exists.
> + *         If a CDP peer was found, @r_cdp will point to the peer RDT resource
> + *         and @d_cdp will point to the peer RDT domain.
> + */
> +static int __attribute__((unused)) rdt_cdp_peer_get(struct rdt_resource *r,
> +						    struct rdt_domain *d,
> +						    struct rdt_resource **r_cdp,
> +						    struct rdt_domain **d_cdp)
> +{
> +	struct rdt_resource *_r_cdp = NULL;
> +	struct rdt_domain *_d_cdp = NULL;
> +	int ret = 0;
> +
> +	switch (r->rid) {
> +	case RDT_RESOURCE_L3DATA:
> +		_r_cdp = &rdt_resources_all[RDT_RESOURCE_L3CODE];
> +		break;
> +	case RDT_RESOURCE_L3CODE:
> +		_r_cdp =  &rdt_resources_all[RDT_RESOURCE_L3DATA];
> +		break;
> +	case RDT_RESOURCE_L2DATA:
> +		_r_cdp =  &rdt_resources_all[RDT_RESOURCE_L2CODE];
> +		break;
> +	case RDT_RESOURCE_L2CODE:
> +		_r_cdp =  &rdt_resources_all[RDT_RESOURCE_L2DATA];
> +		break;
> +	default:
> +		ret = -ENOENT;
> +		goto out;
> +	}
> +
> +	/*
> +	 * When a new CPU comes online and CDP is enabled then the new
> +	 * RDT domains (if any) associated with both CDP RDT resources
> +	 * are added in the same CPU online routine while the
> +	 * rdtgroup_mutex is held. It should thus not happen for one
> +	 * RDT domain to exist and be associated with its RDT CDP
> +	 * resource but there is no RDT domain associated with the
> +	 * peer RDT CDP resource. Hence the WARN.
> +	 */
> +	_d_cdp = rdt_find_domain(_r_cdp, d->id, NULL);
> +	if (WARN_ON(!_d_cdp)) {
> +		_r_cdp = NULL;
> +		ret = -ENOENT;

While this should never happen, the return value is ambiguous. I'd rather
use EINVAL or such and propagate it further down at the call site.

Thanks,

	tglx

Re: [PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer

[Thomas Gleixner]

On Wed, 26 Sep 2018, Reinette Chatre wrote:
>  /**
> - * rdtgroup_cbm_overlaps - Does CBM for intended closid overlap with other
> + * _rdtgroup_cbm_overlaps - Does CBM for intended closid overlap with other
>   * @r: Resource to which domain instance @d belongs.
>   * @d: The domain instance for which @closid is being tested.
>   * @cbm: Capacity bitmask being tested.
> @@ -1049,8 +1048,8 @@ static int __attribute__((unused)) rdt_cdp_peer_get(struct rdt_resource *r,
>   *
>   * Return: false if CBM does not overlap, true if it does.
>   */
> -bool rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
> -			   u32 _cbm, int closid, bool exclusive)
> +static bool _rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
> +				   u32 _cbm, int closid, bool exclusive)

Existing issue. The documentation uses @cbm, but the argument is _cbm.

Also please make this __rdtgroup_cbm_overlaps(). Double underscores are
standing more out.

>  {
>  	unsigned long *cbm = (unsigned long *)&_cbm;
>  	unsigned long *ctrl_b;
> @@ -1087,6 +1086,44 @@ bool rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
>  	return false;
>  }
>  
> +/**
> + * rdtgroup_cbm_overlaps - Does CBM overlap with other use of hardware
> + * @r: Resource to which domain instance @d belongs.
> + * @d: The domain instance for which @closid is being tested.
> + * @cbm: Capacity bitmask being tested.
> + * @closid: Intended closid for @cbm.
> + * @exclusive: Only check if overlaps with exclusive resource groups
> + *
> + * Resources that can be allocated using a CBM can use the CBM to control
> + * the overlap of these allocations. rdtgroup_cmb_overlaps() is the test
> + * for overlap. Overlap test is not limited to the specific resource for
> + * which the CBM is intended though - when dealing with CDP resources that
> + * share the underlying hardware the overlap check should be performed on
> + * the CDP resource sharing the hardware also.
> + *
> + * Refer to description of _rdtgroup_cbm_overlaps() for the details of the
> + * overlap test.
> + *
> + * Return: true if CBM overlap detected, false if there is no overlap
> + */
> +bool rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
> +			   u32 _cbm, int closid, bool exclusive)

Ditto. And here is no reason for using _cbm.

> +{
> +	struct rdt_resource *r_cdp;
> +	struct rdt_domain *d_cdp;
> +	bool ret;
> +
> +	ret = _rdtgroup_cbm_overlaps(r, d, _cbm, closid, exclusive);
> +	if (ret)
> +		return ret;

  	if (__rdtgroup_cbm_overlaps(r, d, _cbm, closid, exclusive))
		return true;

> +
> +	if (rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp) == 0)
> +		return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
> +					      closid, exclusive);

	if (rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp) < 0)
		return false;

	return __rdtgroup_cbm_overlaps(r_cpd, d_cdp, _cbm, closid, exclusive);

Makes the whole thing more obvious.

Thanks,

	tglx

Re: [PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer

[Reinette Chatre]

Hi Thomas,

On 10/3/2018 12:02 AM, Thomas Gleixner wrote:
> On Wed, 26 Sep 2018, Reinette Chatre wrote:
>>  /**
>> - * rdtgroup_cbm_overlaps - Does CBM for intended closid overlap with other
>> + * _rdtgroup_cbm_overlaps - Does CBM for intended closid overlap with other
>>   * @r: Resource to which domain instance @d belongs.
>>   * @d: The domain instance for which @closid is being tested.
>>   * @cbm: Capacity bitmask being tested.
>> @@ -1049,8 +1048,8 @@ static int __attribute__((unused)) rdt_cdp_peer_get(struct rdt_resource *r,
>>   *
>>   * Return: false if CBM does not overlap, true if it does.
>>   */
>> -bool rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
>> -			   u32 _cbm, int closid, bool exclusive)
>> +static bool _rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
>> +				   u32 _cbm, int closid, bool exclusive)
> 
> Existing issue. The documentation uses @cbm, but the argument is _cbm.

Thanks for spotting this.

> 
> Also please make this __rdtgroup_cbm_overlaps(). Double underscores are
> standing more out.

Will do.

> 
>>  {
>>  	unsigned long *cbm = (unsigned long *)&_cbm;
>>  	unsigned long *ctrl_b;
>> @@ -1087,6 +1086,44 @@ bool rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
>>  	return false;
>>  }
>>  
>> +/**
>> + * rdtgroup_cbm_overlaps - Does CBM overlap with other use of hardware
>> + * @r: Resource to which domain instance @d belongs.
>> + * @d: The domain instance for which @closid is being tested.
>> + * @cbm: Capacity bitmask being tested.
>> + * @closid: Intended closid for @cbm.
>> + * @exclusive: Only check if overlaps with exclusive resource groups
>> + *
>> + * Resources that can be allocated using a CBM can use the CBM to control
>> + * the overlap of these allocations. rdtgroup_cmb_overlaps() is the test
>> + * for overlap. Overlap test is not limited to the specific resource for
>> + * which the CBM is intended though - when dealing with CDP resources that
>> + * share the underlying hardware the overlap check should be performed on
>> + * the CDP resource sharing the hardware also.
>> + *
>> + * Refer to description of _rdtgroup_cbm_overlaps() for the details of the
>> + * overlap test.
>> + *
>> + * Return: true if CBM overlap detected, false if there is no overlap
>> + */
>> +bool rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
>> +			   u32 _cbm, int closid, bool exclusive)
> 
> Ditto. And here is no reason for using _cbm.

Thanks for spotting this also, will do.

> 
>> +{
>> +	struct rdt_resource *r_cdp;
>> +	struct rdt_domain *d_cdp;
>> +	bool ret;
>> +
>> +	ret = _rdtgroup_cbm_overlaps(r, d, _cbm, closid, exclusive);
>> +	if (ret)
>> +		return ret;
> 
>   	if (__rdtgroup_cbm_overlaps(r, d, _cbm, closid, exclusive))
> 		return true;
> 
>> +
>> +	if (rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp) == 0)
>> +		return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
>> +					      closid, exclusive);
> 
> 	if (rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp) < 0)
> 		return false;
> 
> 	return __rdtgroup_cbm_overlaps(r_cpd, d_cdp, _cbm, closid, exclusive);
> 
> Makes the whole thing more obvious.

I think a different change is needed to support the request from your
review of the first patch to propagate that unthinkable error where only
one of the CDP peers could have an rdt_domain associated with it.

In the above that error in question from rdt_cdp_peer_get() will be lost.

I could do the following in support of propagating that error (note that
in support of the code below __rdtgroup_cbm_overlaps() also changes to
return int instead of bool):

int rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
                          u32 cbm, int closid, bool exclusive)
{
        struct rdt_resource *r_cdp;
        struct rdt_domain *d_cdp;
        int ret;

        if (__rdtgroup_cbm_overlaps(r, d, cbm, closid, exclusive))
                return 1;

        ret = rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp);
        if (ret == -ENOENT) {
                return 0;
        } else if (ret == -EINVAL) {
                rdt_last_cmd_puts("Error finding CDP peer\n");
                return ret;
        } else {
                return  __rdtgroup_cbm_overlaps(r_cdp, d_cdp, cbm,
                                                closid, exclusive);
        }

        return -EINVAL;
}

With the above change in rdtgroup_cbm_overlaps() the call sites then
change to for example:

        ret = rdtgroup_cbm_overlaps(r, d, cbm_val, rdtgrp->closid, true);
        if (ret < 0) {
                /* last_cmd_status already populated with error */
                return -EINVAL;
        } else if (ret == 1) {
                rdt_last_cmd_puts("overlaps with exclusive group\n");
                return -EINVAL;
        }
        /* fall through when no overlap detected */

Would this be acceptable?

Thank you very much

Reinette

Re: [PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer

[Thomas Gleixner]

Reinette,

On Wed, 3 Oct 2018, Reinette Chatre wrote:
> On 10/3/2018 12:02 AM, Thomas Gleixner wrote:
> >> +{
> >> +	struct rdt_resource *r_cdp;
> >> +	struct rdt_domain *d_cdp;
> >> +	bool ret;
> >> +
> >> +	ret = _rdtgroup_cbm_overlaps(r, d, _cbm, closid, exclusive);
> >> +	if (ret)
> >> +		return ret;
> > 
> >   	if (__rdtgroup_cbm_overlaps(r, d, _cbm, closid, exclusive))
> > 		return true;
> > 
> >> +
> >> +	if (rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp) == 0)
> >> +		return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
> >> +					      closid, exclusive);
> > 
> > 	if (rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp) < 0)
> > 		return false;
> > 
> > 	return __rdtgroup_cbm_overlaps(r_cpd, d_cdp, _cbm, closid, exclusive);
> > 
> > Makes the whole thing more obvious.
> 
> I think a different change is needed to support the request from your
> review of the first patch to propagate that unthinkable error where only
> one of the CDP peers could have an rdt_domain associated with it.
> 
> In the above that error in question from rdt_cdp_peer_get() will be lost.
> 
> I could do the following in support of propagating that error (note that
> in support of the code below __rdtgroup_cbm_overlaps() also changes to
> return int instead of bool):
> 
> int rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
>                           u32 cbm, int closid, bool exclusive)
> {
>         struct rdt_resource *r_cdp;
>         struct rdt_domain *d_cdp;
>         int ret;
> 
>         if (__rdtgroup_cbm_overlaps(r, d, cbm, closid, exclusive))
>                 return 1;
> 
>         ret = rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp);
>         if (ret == -ENOENT) {
>                 return 0;
>         } else if (ret == -EINVAL) {
>                 rdt_last_cmd_puts("Error finding CDP peer\n");
>                 return ret;
>         } else {
>                 return  __rdtgroup_cbm_overlaps(r_cdp, d_cdp, cbm,
>                                                 closid, exclusive);
>         }
> 
>         return -EINVAL;
> }
> 
> With the above change in rdtgroup_cbm_overlaps() the call sites then
> change to for example:
> 
>         ret = rdtgroup_cbm_overlaps(r, d, cbm_val, rdtgrp->closid, true);
>         if (ret < 0) {
>                 /* last_cmd_status already populated with error */
>                 return -EINVAL;
>         } else if (ret == 1) {
>                 rdt_last_cmd_puts("overlaps with exclusive group\n");
>                 return -EINVAL;
>         }
>         /* fall through when no overlap detected */
> 
> Would this be acceptable?

We really have to think about that whether it's worth it. Looking at the
resulting code I doubt it. Then I'd rather prefer the warnon and the
simpler code. But either way works for me.

Thanks,

	tglx

Re: [PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer

[Reinette Chatre]

Hi Thomas,

On 10/3/2018 12:43 PM, Thomas Gleixner wrote:
> On Wed, 3 Oct 2018, Reinette Chatre wrote:
>> On 10/3/2018 12:02 AM, Thomas Gleixner wrote:
>>>> +{
>>>> +	struct rdt_resource *r_cdp;
>>>> +	struct rdt_domain *d_cdp;
>>>> +	bool ret;
>>>> +
>>>> +	ret = _rdtgroup_cbm_overlaps(r, d, _cbm, closid, exclusive);
>>>> +	if (ret)
>>>> +		return ret;
>>>
>>>   	if (__rdtgroup_cbm_overlaps(r, d, _cbm, closid, exclusive))
>>> 		return true;
>>>
>>>> +
>>>> +	if (rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp) == 0)
>>>> +		return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
>>>> +					      closid, exclusive);
>>>
>>> 	if (rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp) < 0)
>>> 		return false;
>>>
>>> 	return __rdtgroup_cbm_overlaps(r_cpd, d_cdp, _cbm, closid, exclusive);
>>>
>>> Makes the whole thing more obvious.
>>
>> I think a different change is needed to support the request from your
>> review of the first patch to propagate that unthinkable error where only
>> one of the CDP peers could have an rdt_domain associated with it.
>>
>> In the above that error in question from rdt_cdp_peer_get() will be lost.
>>
>> I could do the following in support of propagating that error (note that
>> in support of the code below __rdtgroup_cbm_overlaps() also changes to
>> return int instead of bool):
>>
>> int rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d,
>>                           u32 cbm, int closid, bool exclusive)
>> {
>>         struct rdt_resource *r_cdp;
>>         struct rdt_domain *d_cdp;
>>         int ret;
>>
>>         if (__rdtgroup_cbm_overlaps(r, d, cbm, closid, exclusive))
>>                 return 1;
>>
>>         ret = rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp);
>>         if (ret == -ENOENT) {
>>                 return 0;
>>         } else if (ret == -EINVAL) {
>>                 rdt_last_cmd_puts("Error finding CDP peer\n");
>>                 return ret;
>>         } else {
>>                 return  __rdtgroup_cbm_overlaps(r_cdp, d_cdp, cbm,
>>                                                 closid, exclusive);
>>         }
>>
>>         return -EINVAL;
>> }
>>
>> With the above change in rdtgroup_cbm_overlaps() the call sites then
>> change to for example:
>>
>>         ret = rdtgroup_cbm_overlaps(r, d, cbm_val, rdtgrp->closid, true);
>>         if (ret < 0) {
>>                 /* last_cmd_status already populated with error */
>>                 return -EINVAL;
>>         } else if (ret == 1) {
>>                 rdt_last_cmd_puts("overlaps with exclusive group\n");
>>                 return -EINVAL;
>>         }
>>         /* fall through when no overlap detected */
>>
>> Would this be acceptable?
> 
> We really have to think about that whether it's worth it. Looking at the
> resulting code I doubt it. Then I'd rather prefer the warnon and the
> simpler code. But either way works for me.

Thank you very much. I'll resubmit with the changes you prefer.

Reinette