Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <86fc22a2-e779-b7ab-67d6-a3aff975ae56@linux.alibaba.com>
Date:   Fri, 21 Oct 2022 16:22:40 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.3.2
From:   Shawn Wang <shawnwang@linux.alibaba.com>
Subject: Re: [PATCH v2] x86/resctrl: Clear the stale staged config after the
 configuration is completed
To:     Reinette Chatre <reinette.chatre@intel.com>, fenghua.yu@intel.com
References: <1665304608-120466-1-git-send-email-shawnwang@linux.alibaba.com>
 <7fa6ed4e-abae-85fb-4e95-8c73755a4263@intel.com>
 <ad08eee6-cfea-3858-0def-e2e3fef315fb@linux.alibaba.com>
 <ff44b0ff-6adb-3bae-d17e-4c341c09df5d@intel.com>
Cc:     tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com,
        James Morse <james.morse@arm.com>, jamie@nuviainc.com,
        linux-kernel@vger.kernel.org
In-Reply-To: <ff44b0ff-6adb-3bae-d17e-4c341c09df5d@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Reinette,

On 10/21/2022 12:35 AM, Reinette Chatre wrote:

...

>>>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>>> index 1dafbdc5ac31..2c719da5544f 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>>> @@ -338,6 +338,8 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
>>>>                    msr_param.high = max(msr_param.high, idx + 1);
>>>>                }
>>>>            }
>>>> +        /* Clear the stale staged config */
>>>> +        memset(d->staged_config, 0, sizeof(d->staged_config));
>>>>        }
>>>>          if (cpumask_empty(cpu_mask))
>>>
>>> Please also ensure that the temporary storage is cleared if there is an
>>> early exist because of failure. Please do not duplicate the memset() code
>>> but instead move it to a common exit location.
>>>
>>
>> There are two different resctrl_arch_update_domains() function call paths:
>>
>> 1.rdtgroup_mkdir()->rdtgroup_mkdir_ctrl_mon()->rdtgroup_init_alloc()->resctrl_arch_update_domains()
>> 2.rdtgroup_schemata_write()->resctrl_arch_update_domains()
>>
>> Perhaps there is no common exit location if we want to clear staged_config[] after every call of resctrl_arch_update_domains().
> 
> I was referring to a common exit out of resctrl_arch_update_domains().
> 
> Look at how resctrl_arch_update_domains() behaves with this change:
> 
> resctrl_arch_update_domains()
> {
> 	...
> 
> 	if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL))
> 		return -ENOMEM;
> 
> 	...
> 	list_for_each_entry(d, &r->domains, list) {
> 		...
> 		memset(d->staged_config, 0, sizeof(d->staged_config));
> 	}
> 
> 
> 	...
> done:
> 	free_cpumask_var(cpu_mask);
> 	
> 	return 0;
> }
> 
> 
> The goal of this fix is to ensure that staged_config[] is cleared on
> return from resctrl_arch_update_domains() so that there is no stale
> data in staged_config[] when resctrl_arch_update_domains() is called
> again.
> 
> Considering this, I can see two scenarios in the above solution where
> staged_config[] is not cleared on exit from resctrl_arch_update_domains():

It may not be enough to just clear staged_config[] when 
resctrl_arch_update_domains() exits. I think the fix needs to make sure 
staged_config[] can be cleared where it is set.

The modification of staged_config[] comes from two paths:

Path 1:
rdtgroup_schemata_write() {
	...
	rdtgroup_parse_resource() 	// set staged_config[]
	...				
	resctrl_arch_update_domains() 	// clear staged_config[]
	...
}

Path 2:
rdtgroup_init_alloc() {
	...
	rdtgroup_init_mba()/rdtgroup_init_cat()	// set staged_config[]
	...
	resctrl_arch_update_domains()		// clear staged_config[]
	...
}

If we clear staged_config[] in resctrl_arch_update_domains(), goto 
statement for error handling between setting staged_config[] and calling 
resctrl_arch_update_domains() will be ignored. This can still remain the 
stale staged_config[].

I think maybe it is better to put the clearing work where 
rdtgroup_schemata_write() and rdtgroup_init_alloc() exit.

(Sorry, I mistakenly wrote rdtgroup_init_alloc() to 
rdtgroup_mkdir_ctrl_mon() in my last reply.)

Thank you,

Shawn