Hi Reinette,
On 10/26/22 15:23, Reinette Chatre wrote:
> Hi Babu,
>
> On 10/26/2022 12:07 PM, Moger, Babu wrote:
>> On 10/25/22 18:43, Reinette Chatre wrote:
>>> On 10/17/2022 3:26 PM, Babu Moger wrote:
>>>
>>> ...
>>>
>>>> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
>>>>
>>>> list_for_each_entry(s, &resctrl_schema_all, list) {
>>>> r = s->res;
>>>> - if (r->rid == RDT_RESOURCE_MBA) {
>>>> + if (r->rid == RDT_RESOURCE_MBA ||
>>>> + r->rid == RDT_RESOURCE_SMBA) {
>>>> rdtgroup_init_mba(r, rdtgrp->closid);
>>>> if (is_mba_sc(r))
>>>> continue;
>>> The above hunk and the ones that follow are unexpected.
>> I am thinking the above check is required, It is updating the
>> staged_config with default values. Right now, the default value for SMBA
>> is same as MBA default value. So, I used this code to initialize.
>>
>> Did I miss something?
> As I described in the following comments my concern is related to all the
> software controller code still executing for SMBA. Yes, in the above hunk
> SMBA would need (some of) rdtgroup_init_mba() ... but note that it contains
> software controller checks and in the above hunk its call is also followed
> by another software controller check.
>
> The software controller is just applicable to MBA and these checks have been
> isolated to the MBA resource. Using it for SMBA that does not support
> software controller at all is making the code harder to follow and sets this
> code up for future mistakes. I think it would make the code easier to understand
> if this is made very clear that software controller is not applicable to SMBA at
> all instead of repurposing these flows.
Yes. Understood. How about this? I feel this is much more cleaner.
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e5a48f05e787..d91a6a513681 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2845,16 +2845,18 @@ static int rdtgroup_init_alloc(struct rdtgroup
*rdtgrp)
list_for_each_entry(s, &resctrl_schema_all, list) {
r = s->res;
- if (r->rid == RDT_RESOURCE_MBA) {
+ if (r->rid == RDT_RESOURCE_MBA ||
+ r->rid == RDT_RESOURCE_SMBA) {
rdtgroup_init_mba(r, rdtgrp->closid);
- if (is_mba_sc(r))
- continue;
} else {
ret = rdtgroup_init_cat(s, rdtgrp->closid);
if (ret < 0)
return ret;
}
+ if (is_mba_sc(r))
+ continue;
+
ret = resctrl_arch_update_domains(r, rdtgrp->closid);
if (ret < 0) {
rdt_last_cmd_puts("Failed to initialize
allocations\n");
Thanks
Babu
>
>>> Note that the software controller, when resctrl is mounted with "mba_MBps", is
>>> only supported by RDT_RESOURCE_MBA. At this time this really is hard coded all
>>> over the place, for example:
>>>
>>> static int set_mba_sc(bool mba_sc)
>>> {
>>> struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
>>> ...
>>>
>>> }
>>>
>>> Since SMBA hardcodes "delay_linear = false" I do not expect it to support the software
>>> controller ... but these hunks appear to treat SMBA as though it does. It is the "MBA software
>>> controller", not "SMBA software controller". Why does it check above if the MBA software
>>> controller is enabled on SMBA?
>> There is no plan to support SMBA software controller. Yes, I think below
>> checks are not required.
> Thank you for clarifying. Having the code reflect that clearly would make everything
> easier to understand and maintain.
>
> Reinette
>
--
Thanks
Babu Moger
