Message-ID: <CALPaoCjyAHAkKJAx3FL3Lze2KkWxDWFDaMyNFPRwuh0Y4Vzz2w@mail.gmail.com>
Date: Wed, 6 Nov 2024 10:42:49 +0100
From: Peter Newman <peternewman@...gle.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: "Luck, Tony" <tony.luck@...el.com>, "Yu, Fenghua" <fenghua.yu@...el.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, "x86@...nel.org" <x86@...nel.org>, 
	"H . Peter Anvin" <hpa@...or.com>, Babu Moger <babu.moger@....com>, James Morse <james.morse@....com>, 
	Martin Kletzander <nert.pinx@...il.com>, Shaopeng Tan <tan.shaopeng@...itsu.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Eranian, Stephane" <eranian@...gle.com>
Subject: Re: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

Hi Reinette,

On Wed, Nov 6, 2024 at 1:13 AM Reinette Chatre
<reinette.chatre@...el.com> wrote:
>
> Hi Tony,
>
> On 11/5/24 3:39 PM, Luck, Tony wrote:
> >> I think this change already undoes the motivation for 09909e098113
> >> ("x86/resctrl: Queue mon_event_read() instead of sending an IPI")? As you mention in
> >> changelog the goal of that work was to enable resctrl_arch_rmid_read() to sleep.
> >> This change will call resctrl_arch_rmid_read() with preemption disabled if
> >> it happens to be called on CPU in monitoring domain. Would that not cause
> >> MPAM monitor count reads from CPU in domain to be a bug?
> >>
> >> Could you please try out this patch with CONFIG_DEBUG_ATOMIC_SLEEP=y?
> >
> > How is this all going to look after the split into fs/resctrl and arch/* ?
>
> Unclear to me at this point. Peter exposed an issue with current implementation
> and this needs to be fixed. Since this involves preparatory work that impacts
> systems currently supported we could also consider reverting to original behavior
> and go back to drawing board with the preparatory work.
>
> > Is the file system code going to have implementation choices that prevent
> > performance sensitive users like Peter from optimizing monitor event
> > reads by binding the monitor process to a CPU in the right domain
> > to avoid IPI?
>
> Apologies for not clearly stating it but I do agree that there is an issue
> that needs to be fixed.
>
> My response was not intended to be interpreted as a NACK but instead an attempt
> to engage in discussion by pointing out that the proposed fix may not be ideal.
>
> I tried out my own suggestion and indeed when just trying to mount resctrl
> on x86 with this patch applied results in:
>         BUG: scheduling while atomic
>
> I do not object to optimizing monitor event reads but the proposed fix
> is not appropriate in its current form.

Yes, I mentioned the atomic sleep issues in my reply to Fenghua. I
expect even the new case I added will have problems on
resctrl_arch_rmid_read() implementations which block, since it would
need to be called with preemption disabled to ensure invocation by a
direct function call stays in the right domain. The last proposed MPAM
resctrl_arch_rmid_read() implementation I saw can return an error when
called in an atomic context, so that means my change would cause slow
counters (i.e., MPAM CSU) to be unreadable when read from the local
domain. (Or totally unreadable on a single-domain machine.)

As a refresher, the original issue that led to this situation was how
an MPAM CSU (cache occupancy) monitor can be installed in response to
a read request. The number of monitors is usually small (or just 1),
so they need to be frequently installed, there can be access issues
depending on what CPU wants to read which domain, and installing a
monitor is a slow operation that requires waiting.

https://lore.kernel.org/lkml/670081d0-b4fc-79c5-68f8-5b3c162b74b9@arm.com/

Thanks,
-Peter

Powered by blists - more mailing lists