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