Messages in this thread | | | Date | Wed, 17 Apr 2024 15:42:28 +0100 | From | Dave Martin <> | Subject | Re: [PATCH v1 20/31] x86/resctrl: Allow an architecture to disable pseudo lock |
| |
Hi Reinette,
On Thu, Apr 11, 2024 at 10:40:03AM -0700, Reinette Chatre wrote: > Hi Dave, > > On 4/11/2024 7:17 AM, Dave Martin wrote: > > 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? > > Thank you for digging into this. It was not obvious to me that > the changelog referred to the last_cmd_status string. I do > not think this warrants making the stubs more complicated. > > Reinette >
OK, I'll leave this as-is for now.
Cheers ---Dave
| |