lkml.org 
[lkml]   [2024]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from update_cpu_closid_rmid()
    On Mon, Apr 15, 2024 at 10:47:55AM -0700, Reinette Chatre wrote:
    > Hi Dave,
    >
    > On 4/12/2024 9:12 AM, Dave Martin wrote:
    > > On Mon, Apr 08, 2024 at 08:16:08PM -0700, Reinette Chatre wrote:
    > >> Hi James,
    > >>
    > >> On 3/21/2024 9:50 AM, James Morse wrote:
    > >>> update_cpu_closid_rmid() takes a struct rdtgroup as an argument, which
    > >>> it uses to update the local CPUs default pqr values. This is a problem
    > >>> once the resctrl parts move out to /fs/, as the arch code cannot
    > >>> poke around inside struct rdtgroup.
    > >>>
    > >>> Rename update_cpu_closid_rmid() as resctrl_arch_sync_cpus_defaults()
    > >>> to be used as the target of an IPI, and pass the effective CLOSID
    > >>> and RMID in a new struct.
    > >>>
    > >>> Signed-off-by: James Morse <james.morse@arm.com>
    > >>> ---
    > >>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 +++++++++++++++----
    > >>> include/linux/resctrl.h | 11 +++++++++++
    > >>> 2 files changed, 26 insertions(+), 4 deletions(-)
    > >>>
    > >>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    > >>> index 5d2c1ce5b6b1..18f097fce51e 100644
    > >>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    > >>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    > >>> @@ -341,13 +341,13 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
    > >>> * from update_closid_rmid() is protected against __switch_to() because
    > >>> * preemption is disabled.
    > >>> */
    > >>> -static void update_cpu_closid_rmid(void *info)
    > >>> +void resctrl_arch_sync_cpu_defaults(void *info)
    > >>> {
    > >>> - struct rdtgroup *r = info;
    > >>> + struct resctrl_cpu_sync *r = info;
    > >>>
    > >>> if (r) {
    > >>> this_cpu_write(pqr_state.default_closid, r->closid);
    > >>> - this_cpu_write(pqr_state.default_rmid, r->mon.rmid);
    > >>> + this_cpu_write(pqr_state.default_rmid, r->rmid);
    > >>> }
    > >>>
    > >>> /*
    > >>> @@ -362,11 +362,22 @@ static void update_cpu_closid_rmid(void *info)
    > >>> * Update the PGR_ASSOC MSR on all cpus in @cpu_mask,
    > >>> *
    > >>> * Per task closids/rmids must have been set up before calling this function.
    > >>> + * @r may be NULL.
    > >>> */
    > >>> static void
    > >>> update_closid_rmid(const struct cpumask *cpu_mask, struct rdtgroup *r)
    > >>> {
    > >>> - on_each_cpu_mask(cpu_mask, update_cpu_closid_rmid, r, 1);
    > >>> + struct resctrl_cpu_sync defaults;
    > >>> + struct resctrl_cpu_sync *defaults_p = NULL;
    > >>
    > >> Please maintain reverse fir order.
    > >
    > > Or, more tersely as follows?
    > >
    > > struct resctrl_cpu_sync defaults, *defaults_p = NULL;
    >
    > Sure.

    [*]

    > >
    > > "Reverse fir order" seems to be documented as a preference rather than a
    > > rule.
    >
    > This does not seem to be a place that warrants an exception to this
    > preference. Note how this function is not consistent with any other
    > in the file.

    Ack (just bikeshedding here TBH).

    >
    > > The declarations can be swapped, but defaults_p is in some sense a weak
    > > pointer to defaults, so it feels a bit strange to declare them backwards.
    > >
    > > Alternatively, could we rename defaults_p to p? Given the size of this
    > > function I don't think that impacts clarity.

    [...]

    > >
    > > I'll wait for your opinion on this.
    > >
    > >
    >
    > Do you imply that this would maintain the order in this patch? It does
    > not look to me that it would but I may be looking wrong.

    I'm not sure without looking again, but since this discussion is not a
    good use of your time I'll just go ahead and implement the change at
    [*] above, while restoring referse FIR order, if that is good for you.

    >
    > sidenote: the "on_each_cpu_mask()" in update_closid_rmid() can be on
    > one line.

    I guess that might have been split to stick to the 80-char limit.

    Due the the small size of this function, shall I just rename defaults_p to p?
    Alternatively, there are already a few non-printk lines over 80 chars, so
    maybe we can tolerate one more here?

    >
    > ..
    >
    > >>> + * struct resctrl_cpu_sync, or NULL.
    > >>> + */
    > >>
    > >> Updating the CPU's defaults is not the primary goal of this function and because
    > >> of that I do not think this should be the focus with the main goal (updating
    > >> RMID and CLOSID on CPU) ignored. Specifically, this function only updates
    > >> the defaults if *info is set but it _always_ ensures CPU is running with
    > >> appropriate CLOSID/RMID (which may or may not be from a CPU default).
    > >>
    > >> I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate
    > >> and the comment needs to elaborate what the function does.
    > >>
    > >>> +void resctrl_arch_sync_cpu_defaults(void *info);
    > >
    > > That seems reasonable, and follows the original naming and what the
    > > code does:
    > >
    > > What about:
    > >
    > > /**
    > > * resctrl_arch_sync_cpu_defaults() - Refresh the CPU's CLOSID and RMID.
    > > * Call via IPI.
    >
    > Did you intend to change function name?

    Er, yes, I meant to use your suggestion here, so:
    resctrl_arch_sync_cpu_closid_rmid().

    Also, Babu Moger's suggestion to rename struct resctrl_cpu_sync
    to resctrl_cpu_defaults seems good, since that accurately describes what
    is specified in the struct (and what is *not* specified if NULL is
    passed).

    >
    > How about "Refresh the CPU's ..." -> "Refresh this CPU's ..." I think it
    > makes it more obvious how this function is called.

    Agreed.

    >
    > > * @info: If non-NULL, a pointer to a struct resctrl_cpu_sync specifying
    > > * the new CLOSID and RMID for tasks in the default resctrl ctrl
    > > * and mon group when running on this CPU. If NULL, the default
    > > * CLOSID and RMID are not changed.
    >
    > "If NULL, this CPU is not re-assigned to a different group." ?

    Agreed.

    > > *
    > > * This is how reassignment of CPUs and/or tasks to different resctrl groups
    > > * is propagated when requested by the resctrl fs core code.
    >
    > Could you please use imperative tone here? For example, "Propagates reassignment
    > of CPUs and/or tasks to different resctrl groups."

    Yes, that's better (and shorter).

    >
    > > *
    > > * This function should typically record the per-cpu defaults specified by
    >
    > "should" sounds like there may be cases when this is not done? Maybe just
    > "Records the per-CPU defaults specified ..."

    I didn't want to pre-judge what implementation-specific cruft the arch
    code needs here, so I was intentionally vague. But the arch would need
    to put the CPU defaults into effect somehow or other, so yes, I think
    your text is better here.

    I'll make a note of those changes.

    [...]

    Cheers
    ---Dave

    \
     
     \ /
      Last update: 2024-04-16 18:17    [W:4.111 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site