Re: [PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer
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, _cdp, _cdp) == 0)
+ return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
+closid, exclusive);
>>>
>>> if (rdt_cdp_peer_get(r, d, _cdp, _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, _cdp, _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
Re: [PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer
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, _cdp, _cdp) == 0)
+ return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
+closid, exclusive);
>>>
>>> if (rdt_cdp_peer_get(r, d, _cdp, _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, _cdp, _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
Re: [PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer
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, _cdp, _cdp) == 0)
> >> + return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
> >> +closid, exclusive);
> >
> > if (rdt_cdp_peer_get(r, d, _cdp, _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, _cdp, _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,
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, _cdp, _cdp) == 0)
> >> + return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
> >> +closid, exclusive);
> >
> > if (rdt_cdp_peer_get(r, d, _cdp, _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, _cdp, _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
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, _cdp, _cdp) == 0)
>> +return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
>> + closid, exclusive);
>
> if (rdt_cdp_peer_get(r, d, _cdp, _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, _cdp, _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
Re: [PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer
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, _cdp, _cdp) == 0)
>> +return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
>> + closid, exclusive);
>
> if (rdt_cdp_peer_get(r, d, _cdp, _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, _cdp, _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
Re: [PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer
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, _cdp, _cdp) == 0)
> + return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
> + closid, exclusive);
if (rdt_cdp_peer_get(r, d, _cdp, _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
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, _cdp, _cdp) == 0)
> + return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
> + closid, exclusive);
if (rdt_cdp_peer_get(r, d, _cdp, _cdp) < 0)
return false;
return __rdtgroup_cbm_overlaps(r_cpd, d_cdp, _cbm, closid, exclusive);
Makes the whole thing more obvious.
Thanks,
tglx
[PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer
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
Signed-off-by: Reinette Chatre
Tested-by: Jithu Joseph
Acked-by: Fenghua Yu
---
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, _cdp, _cdp) == 0)
+ return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
+
[PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer
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
Signed-off-by: Reinette Chatre
Tested-by: Jithu Joseph
Acked-by: Fenghua Yu
---
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, _cdp, _cdp) == 0)
+ return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm,
+
