Re: [PATCH v3 09/19] x86/resctrl: Queue mon_event_read() instead of sending an IPI

From: Peter Newman <peternewman@google.com>
To: James Morse <james.morse@arm.com>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org,
	Fenghua Yu <fenghua.yu@intel.com>,
	Reinette Chatre <reinette.chatre@intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	H Peter Anvin <hpa@zytor.com>, Babu Moger <Babu.Moger@amd.com>,
	shameerali.kolothum.thodi@huawei.com,
	D Scott Phillips OS <scott@os.amperecomputing.com>,
	carl@os.amperecomputing.com, lcherian@marvell.com,
	bobo.shaobowang@huawei.com, tan.shaopeng@fujitsu.com,
	xingxin.hx@openanolis.org, baolin.wang@linux.alibaba.com,
	Jamie Iles <quic_jiles@quicinc.com>,
	Xin Hao <xhao@linux.alibaba.com>
Subject: Re: [PATCH v3 09/19] x86/resctrl: Queue mon_event_read() instead of sending an IPI
Date: Wed, 22 Mar 2023 15:07:59 +0100	[thread overview]
Message-ID: <CALPaoCgXYBphe+toVBmF6eGKz8sCHYsaTvvd5ZnrJBf07tjbzg@mail.gmail.com> (raw)
In-Reply-To: <20230320172620.18254-10-james.morse@arm.com>

Hi James,

On Mon, Mar 20, 2023 at 6:27 PM James Morse <james.morse@arm.com> wrote:
>
> x86 is blessed with an abundance of monitors, one per RMID, that can be

As I explained earlier, this is not the case on AMD.

> read from any CPU in the domain. MPAMs monitors reside in the MMIO MSC,
> the number implemented is up to the manufacturer. This means when there are
> fewer monitors than needed, they need to be allocated and freed.
>
> Worse, the domain may be broken up into slices, and the MMIO accesses
> for each slice may need performing from different CPUs.
>
> These two details mean MPAMs monitor code needs to be able to sleep, and
> IPI another CPU in the domain to read from a resource that has been sliced.

This doesn't sound very convincing. Could mon_event_read() IPI all the
CPUs in the domain? (after waiting to allocate and install monitors
when necessary?)

>
> mon_event_read() already invokes mon_event_count() via IPI, which means
> this isn't possible. On systems using nohz-full, some CPUs need to be
> interrupted to run kernel work as they otherwise stay in user-space
> running realtime workloads. Interrupting these CPUs should be avoided,
> and scheduling work on them may never complete.
>
> Change mon_event_read() to pick a housekeeping CPU, (one that is not using
> nohz_full) and schedule mon_event_count() and wait. If all the CPUs
> in a domain are using nohz-full, then an IPI is used as the fallback.
>
> This function is only used in response to a user-space filesystem request
> (not the timing sensitive overflow code).
>
> This allows MPAM to hide the slice behaviour from resctrl, and to keep
> the monitor-allocation in monitor.c.

This goal sounds more likely.

If it makes the initial enablement smoother, then I'm all for it.

Reviewed-By: Peter Newman <peternewman@google.com>

These changes worked fine for me on tip/master, though there were merge
conflicts to resolve.

Tested-By: Peter Newman <peternewman@google.com>

Thanks!

-Peter