linux-kernel - Re: [PATCH v7 05/12] x86/resctrl: Detect and configure Slow Memory Bandwidth allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fbb32df7-df49-7a51-67d8-a8abfe7e1c3f@amd.com>
Date:   Thu, 27 Oct 2022 10:30:15 -0500
From:   "Moger, Babu" <babu.moger@....com>
To:     Reinette Chatre <reinette.chatre@...el.com>, corbet@....net,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de
Cc:     fenghua.yu@...el.com, dave.hansen@...ux.intel.com, x86@...nel.org,
        hpa@...or.com, paulmck@...nel.org, akpm@...ux-foundation.org,
        quic_neeraju@...cinc.com, rdunlap@...radead.org,
        damien.lemoal@...nsource.wdc.com, songmuchun@...edance.com,
        peterz@...radead.org, jpoimboe@...nel.org, pbonzini@...hat.com,
        chang.seok.bae@...el.com, pawan.kumar.gupta@...ux.intel.com,
        jmattson@...gle.com, daniel.sneddon@...ux.intel.com,
        sandipan.das@....com, tony.luck@...el.com, james.morse@....com,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        bagasdotme@...il.com, eranian@...gle.com
Subject: Re: [PATCH v7 05/12] x86/resctrl: Detect and configure Slow Memory
 Bandwidth allocation

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