lkml.org 
[lkml]   [2024]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2024-05-27 16:43    [W:3.965 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site