Messages in this thread | | | Date | Thu, 11 Apr 2024 15:17:34 +0100 | From | Dave Martin <> | Subject | Re: [PATCH v1 20/31] x86/resctrl: Allow an architecture to disable pseudo lock |
| |
On Mon, Apr 08, 2024 at 08:24:12PM -0700, Reinette Chatre wrote: > Hi James, > > On 3/21/2024 9:50 AM, James Morse wrote: > > Pseudo-lock relies on knowledge of the micro-architecture to disable > > prefetchers etc. > > > > On arm64 these controls are typically secure only, meaning linux can't > > access them. Arm's cache-lockdown feature works in a very different > > way. Resctrl's pseudo-lock isn't going to be used on arm64 platforms. > > > > Add a Kconfig symbol that can be selected by the architecture. This > > enables or disables building of the psuedo_lock.c file, and replaces > > pseudo_lock.c
Noted.
> > the functions with stubs. An additional IS_ENABLED() check is needed > > in rdtgroup_mode_write() so that attempting to enable pseudo-lock > > reports an "Unknown or unsupported mode" to user-space. > > > > I am missing something here. It is not obvious to me why the IS_ENABLED() > check is needed. Wouldn't rdtgroup_locksetup_enter() > return -EOPNOTSUPP if CONFIG_RESCTRL_FS_PSEUDO_LOCK is not enabled? > > Reinette >
Hmm, if I've understood all this correctly, then it looks like the existing code in rdtgroup_mode_write() relies on the dispatched function (rdtgroup_locksetup_enter() etc.) to do an appropriate rdt_last_cmd_puts() on failure. If no function is called at all and the requested mode change is not a no-op or otherwise trivially successful, then it looks like we're supposed to fall into the else clause.
I'd guess James' intent here was to use the fallback else {} to write a suitable status string, while keeping the stub functions as trivial as possible.
Just taking the IS_ENABLED() away would result in error return from the write(), but no suitable last_cmd_status string.
For consistency with the existing x86 implementation, I wonder whether we should put a suitable rdt_last_cmd_puts() in the stub for rdtgroup_locksetup_enter().
There might be other ways to refactor or simplify this, though.
Thoughts?
Cheers ---Dave
| |