lkml.org 
[lkml]   [2022]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v7 05/12] x86/resctrl: Detect and configure Slow Memory Bandwidth allocation
    From
    Hi Babu,

    On 10/27/2022 8:30 AM, Moger, Babu wrote:
    > On 10/26/22 15:23, Reinette Chatre wrote:
    >> On 10/26/2022 12:07 PM, Moger, Babu wrote:
    >>> On 10/25/22 18:43, Reinette Chatre wrote:
    >>>> On 10/17/2022 3:26 PM, Babu Moger wrote:
    >>>>
    >>>> ...
    >>>>
    >>>>> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
    >>>>>
    >>>>> list_for_each_entry(s, &resctrl_schema_all, list) {
    >>>>> r = s->res;
    >>>>> - if (r->rid == RDT_RESOURCE_MBA) {
    >>>>> + if (r->rid == RDT_RESOURCE_MBA ||
    >>>>> + r->rid == RDT_RESOURCE_SMBA) {
    >>>>> rdtgroup_init_mba(r, rdtgrp->closid);
    >>>>> if (is_mba_sc(r))
    >>>>> continue;
    >>>> The above hunk and the ones that follow are unexpected.
    >>> I am thinking the above check is required, It is updating the
    >>> staged_config with default values. Right now, the default value for SMBA
    >>> is same as MBA default value. So, I used this code to initialize.
    >>>
    >>> Did I miss something?
    >> As I described in the following comments my concern is related to all the
    >> software controller code still executing for SMBA. Yes, in the above hunk
    >> SMBA would need (some of) rdtgroup_init_mba() ... but note that it contains
    >> software controller checks and in the above hunk its call is also followed
    >> by another software controller check.
    >>
    >> The software controller is just applicable to MBA and these checks have been
    >> isolated to the MBA resource. Using it for SMBA that does not support
    >> software controller at all is making the code harder to follow and sets this
    >> code up for future mistakes. I think it would make the code easier to understand
    >> if this is made very clear that software controller is not applicable to SMBA at
    >> all instead of repurposing these flows.
    >
    > Yes. Understood.  How about this? I feel this is much more cleaner.
    >
    > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    > index e5a48f05e787..d91a6a513681 100644
    > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    > @@ -2845,16 +2845,18 @@ static int rdtgroup_init_alloc(struct rdtgroup
    > *rdtgrp)
    >  
    >         list_for_each_entry(s, &resctrl_schema_all, list) {
    >                 r = s->res;
    > -               if (r->rid == RDT_RESOURCE_MBA) {
    > +               if (r->rid == RDT_RESOURCE_MBA ||
    > +                   r->rid == RDT_RESOURCE_SMBA) {
    >                         rdtgroup_init_mba(r, rdtgrp->closid);
    > -                       if (is_mba_sc(r))
    > -                               continue;
    >                 } else {
    >                         ret = rdtgroup_init_cat(s, rdtgrp->closid);
    >                         if (ret < 0)
    >                                 return ret;
    >                 }
    >  
    > +               if (is_mba_sc(r))
    > +                       continue;
    > +
    >                 ret = resctrl_arch_update_domains(r, rdtgrp->closid);
    >                 if (ret < 0) {
    >                         rdt_last_cmd_puts("Failed to initialize
    > allocations\n");
    >

    I do not see how that move changes what is run in the SMBA case and it ignores the
    is_mba_sc() call within rdtgroup_init_mba(). How about making is_mba_sc() more
    robust in support of your original snippet?

    Something like:

    bool is_mba_sc(struct rdt_resource *r)
    {
    if (!r)
    return rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl.membw.mba_sc;

    if (r->rid != RDT_RESOURCE_MBA)
    return false;

    return r->membw.mba_sc;
    }

    Reinette

    \
     
     \ /
      Last update: 2022-10-27 20:38    [W:3.653 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site