| 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
| ||
|
Date: Fri, 4 Mar 2022 16:27:00 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: James Morse <james.morse@....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>,
Jamie Iles <jamie@...iainc.com>,
"D Scott Phillips OS" <scott@...amperecomputing.com>,
<lcherian@...vell.com>, <bobo.shaobowang@...wei.com>,
<tan.shaopeng@...itsu.com>
Subject: Re: [PATCH v3 12/21] x86/resctrl: Calculate bandwidth from the
previous __mon_event_count() chunks
Hi James,
On 2/17/2022 10:21 AM, James Morse wrote:
> 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 a extra
> calls to mbm_overflow_count() and get_corrected_mbm_count().
"removes a extra calls" -> "removes an extra call" ?
__mon_event_count() ends with "rr->val += get_corrected_mbm_count()" and
it is called twice by mbm_update(). The intention in this change is for
mbm_bw_count() to benefit from the rmid read done just before ...
but would using rr->val within mbm_bw_count() not result in it getting
data from both rmid reads due to the increment?
mbm_update() {
rr.val = 0;
if (is_mbm_total_enabled()) {
...
__mon_event_count() /* rr->val += ... */
}
if (is_mbm_local_enabled()) {
...
__mon_event_count() /* rr->val += ... */
if (is_mba_sc(NULL))
mbm_bw_count(rmid, &rr); /* Use rr-> val */
}
Should rr.val perhaps be reset before each __mon_event_count() call instead of
just at the beginning of mbm_update()?
> Calculating bandwidth like this means mbm_bw_count() no longer operates
> on hardware register values directly.
>
> Signed-off-by: James Morse <james.morse@....com>
> ---
> Changes since v2:
> * Expanded commit message
>
> Changes since v1:
> * This patch was rewritten
> ---
> arch/x86/kernel/cpu/resctrl/internal.h | 4 ++--
> arch/x86/kernel/cpu/resctrl/monitor.c | 24 +++++++++++++++---------
> 2 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 3b9e43ba7590..c50c8911ef59 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -289,7 +289,7 @@ struct rftype {
> * struct mbm_state - status for each MBM counter in each domain
> * @chunks: Total data moved (multiply by rdt_group.mon_scale to get bytes)
> * @prev_msr: Value of IA32_QM_CTR for this RMID last time we read it
> - * @prev_bw_msr:Value of previous IA32_QM_CTR for bandwidth counting
> + * @prev_bw_chunks: Previous chunks value read when for bandwidth calculation
"value read when for" -> "value read for"?
> * @prev_bw: The most recent bandwidth in MBps
> * @delta_bw: Difference between the current and previous bandwidth
> * @delta_comp: Indicates whether to compute the delta_bw
> @@ -297,7 +297,7 @@ struct rftype {
> struct mbm_state {
> u64 chunks;
> u64 prev_msr;
> - u64 prev_bw_msr;
> + u64 prev_bw_chunks;
> u32 prev_bw;
> u32 delta_bw;
> bool delta_comp;
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index ac1a2e8998bb..8ae375e29256 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -315,7 +315,7 @@ static u64 __mon_event_count(u32 rmid, struct rmid_read *rr)
>
> if (rr->first) {
> memset(m, 0, sizeof(struct mbm_state));
> - m->prev_bw_msr = m->prev_msr = tval;
> + m->prev_msr = tval;
> return 0;
> }
>
> @@ -329,27 +329,32 @@ static u64 __mon_event_count(u32 rmid, struct rmid_read *rr)
> }
>
> /*
> + * mbm_bw_count() - Update bw count from values previously read by
> + * __mon_event_count().
> + * @rmid: The rmid used to identify the cached mbm_state.
> + * @rr: The struct rmid_read populated by __mon_event_count().
> + *
> * Supporting function to calculate the memory bandwidth
> - * and delta bandwidth in MBps.
> + * and delta bandwidth in MBps. The chunks value previously read by
> + * __mon_event_count() is compared with the chunks value from the previous
> + * invocation. This must be called oncer per second to maintain values in MBps.
"must be called oncer" -> "must be called once"
> */
> static void mbm_bw_count(u32 rmid, struct rmid_read *rr)
> {
> struct rdt_hw_resource *hw_res = resctrl_to_arch_res(rr->r);
> struct mbm_state *m = &rr->d->mbm_local[rmid];
> - u64 tval, cur_bw, chunks;
> + u64 cur_bw, chunks, cur_chunks;
>
> - tval = __rmid_read(rmid, rr->evtid);
> - if (tval & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
> - return;
> + cur_chunks = rr->val;
> + chunks = cur_chunks - m->prev_bw_chunks;
> + m->prev_bw_chunks = cur_chunks;
>
> - chunks = mbm_overflow_count(m->prev_bw_msr, tval, hw_res->mbm_width);
> - cur_bw = (get_corrected_mbm_count(rmid, chunks) * hw_res->mon_scale) >> 20;
> + cur_bw = (chunks * hw_res->mon_scale) >> 20;
>
> if (m->delta_comp)
> m->delta_bw = abs(cur_bw - m->prev_bw);
> m->delta_comp = false;
> m->prev_bw = cur_bw;
> - m->prev_bw_msr = tval;
> }
>
> /*
> @@ -509,6 +514,7 @@ static void mbm_update(struct rdt_resource *r, struct rdt_domain *d, int rmid)
> rr.first = false;
> rr.r = r;
> rr.d = d;
> + rr.val = 0;
>
> /*
> * This is protected from concurrent reads from user
Reinette
Powered by blists - more mailing lists