linux-kernel - Re: [PATCH v6 12/21] x86/resctrl: Calculate bandwidth from the previous __mon_event

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: <53238b86-5df4-a9cc-c60b-89085b7ab558@intel.com>
Date:   Thu, 8 Sep 2022 14:13:13 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     James Morse <james.morse@....com>, haoxin <xhao@...ux.alibaba.com>,
        <x86@...nel.org>, <linux-kernel@...r.kernel.org>
CC:     Fenghua Yu <fenghua.yu@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Babu Moger <Babu.Moger@....com>,
        <shameerali.kolothum.thodi@...wei.com>,
        D Scott Phillips OS <scott@...amperecomputing.com>,
        <lcherian@...vell.com>, <bobo.shaobowang@...wei.com>,
        <tan.shaopeng@...itsu.com>, Jamie Iles <quic_jiles@...cinc.com>,
        Cristian Marussi <cristian.marussi@....com>,
        <xingxin.hx@...nanolis.org>, <baolin.wang@...ux.alibaba.com>
Subject: Re: [PATCH v6 12/21] x86/resctrl: Calculate bandwidth from the
 previous __mon_event_count() chunks



On 9/8/2022 10:00 AM, James Morse wrote:
> Hi Hao Xin,
> 
> On 07/09/2022 07:47, haoxin wrote:
>> 在 2022/9/2 下午11:48, James Morse 写道:
>>> mbm_bw_count() is only called by the mbm_handle_overflow() worker once a
>>> second. It reads the hardware register, calculates the bandwidth and
>>> updates m->prev_bw_msr which is used to hold the previous hardware register
>>> value.
>>>
>>> Operating directly on hardware register values makes it difficult to make
>>> this code architecture independent, so that it can be moved to /fs/,
>>> making the mba_sc feature something resctrl supports with no additional
>>> support from the architecture.
>>> Prior to calling mbm_bw_count(), mbm_update() reads from the same hardware
>>> register using __mon_event_count().
>>>
>>> Change mbm_bw_count() to use the current chunks value most recently saved
>>> by __mon_event_count(). This removes an extra call to __rmid_read().
>>> Instead of using m->prev_msr to calculate the number of chunks seen,
>>> use the rr->val that was updated by __mon_event_count(). This removes an
>>> extra call to mbm_overflow_count() and get_corrected_mbm_count().
>>> Calculating bandwidth like this means mbm_bw_count() no longer operates
>>> on hardware register values directly.
> 
>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> index 3e69386cfe00..2d81b6cd9632 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> 
>>> @@ -516,10 +521,12 @@ static void mbm_update(struct rdt_resource *r, struct rdt_domain
>>> *d, int rmid)
>>>        */
>>>       if (is_mbm_total_enabled()) {
>>>           rr.evtid = QOS_L3_MBM_TOTAL_EVENT_ID;>> +        rr.val = 0;
> 
>> In mbm_update,  there no use the rr.val, so there no need to initialize ？

This patch introduces using rr.val into mbm_bw_count() that is called
from mbm_update(). This patch thus introduces the requirement that rr.val needs
to be initialized. 

> 
>>>           __mon_event_count(rmid, &rr);
>>>       }
>>>       if (is_mbm_local_enabled()) {
>>>           rr.evtid = QOS_L3_MBM_LOCAL_EVENT_ID;
>>> +        rr.val = 0;
> 
>> ditto.
> 
>>>           __mon_event_count(rmid, &rr);
>>>             /*
> 
> No, but this just leaves that problem for someone else to discover the hard way! I think
> its fair for the compiler to complain that addition on an uninitialised field is a bug.
> 
> I'd prefer to keep this as it is on the principle of 'least surprise'.

Yes, please do keep this. If rr.val is not initialized then the software controller will
use wrong data to compute the delta bandwidth.

Reinette