lkml.org 
[lkml]   [2022]   [Aug]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3 06/10] x86/resctrl: Introduce mon_configurable to detect Bandwidth Monitoring Event Configuration
    From
    Hi Babu,

    On 8/25/2022 1:44 PM, Moger, Babu wrote:
    >
    > On 8/25/2022 10:56 AM, Reinette Chatre wrote:
    >> Hi Babu,
    >>
    >> On 8/25/2022 8:11 AM, Moger, Babu wrote:
    >>> On 8/24/22 16:15, Reinette Chatre wrote:
    >>>> On 8/22/2022 6:43 AM, Babu Moger wrote:
    >>>>> Newer AMD processors support the new feature Bandwidth Monitoring Event
    >>>>> Configuration (BMEC). The events mbm_total_bytes and mbm_local_bytes
    >>>>> are configurable when this feature is present.
    >>>>>
    >>>>> Set mon_configurable if the feature is available.
    >>>>>
    >> ...
    >>
    >>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    >>>>> index fc5286067201..855483b297a8 100644
    >>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    >>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    >>>>> @@ -995,6 +995,16 @@ static int rdt_num_rmids_show(struct kernfs_open_file *of,
    >>>>>       return 0;
    >>>>>   }
    >>>>>   +static int rdt_mon_configurable_show(struct kernfs_open_file *of,
    >>>>> +                     struct seq_file *seq, void *v)
    >>>>> +{
    >>>>> +    struct rdt_resource *r = of->kn->parent->priv;
    >>>>> +
    >>>>> +    seq_printf(seq, "%d\n", r->mon_configurable);
    >>>> Why is this file needed? It seems that the next patches also introduce
    >>>> files in support of this new feature that will make the actual configuration
    >>>> data accessible - those files are only created if this feature is supported.
    >>>> Would those files not be sufficient for user space to learn about the feature
    >>>> support?
    >>> This is part of /sys/fs/resctrl/info/L3_MON# directory which basically has
    >>> the information about all the monitoring features. As this is one of the
    >>> mon features, I have added it there. Also, this can be used from the
    >>> utility(like pqos or rdtset) to check if the configuring the monitor is
    >>> supported without looking at individual files. It makes things easier.
    >> I understand the motivation. My concern is that this is a resource wide
    >> file that will display a binary value that, if true, currently means two
    >> events are configurable. We need to consider how things can change in the
    >> future. We should consider that this is only the beginning of monitoring
    >> configuration and need this interface to be ready for future changes. For
    >> example, what if all of the monitoring events are configurable? Let's say,
    >> for example, in future AMD hardware the "llc_occupancy" event also becomes
    >> configurable, how should info/L3_MON/configurable be interpreted? On some
    >> machines it would thus mean that mbm_total_bytes and mbm_local_bytes are
    >> configurable and on some machines it would mean that mbm_total_bytes,
    >> mbm_local_bytes, and llc_occupancy are configurable. This does not make
    >> it easy for utilities.
    >>
    >> So, in this series the info directory on a system that supports BMEC
    >> would look like:
    >>
    >> info/L3_MON/mon_features:llc_occupancy
    >> info/L3_MON/mon_features:mbm_total_bytes
    >> info/L3_MON/mon_features:mbm_local_bytes
    >> info/L3_MON/configurable:1
    >>
    >> Would information like below not be more specific?
    >> info/L3_MON/mon_features:llc_occupancy
    >> info/L3_MON/mon_features:mbm_total_bytes
    >> info/L3_MON/mon_features:mbm_local_bytes
    >> info/L3_MON/mon_features:mbm_total_config
    >> info/L3_MON/mon_features:mbm_local_config
    >
    > Hi Reinette,
    >
    >  Yes. That is more specific.
    >
    > So, basically your idea is to print the information from mon_evt structure if mon_configarable is set in the resource structure.
    >
    > Some thing like ..
    >
    > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    > index 83c8780726ff..46c6bf3f08e3 100644
    > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
    > @@ -1194,8 +1194,11 @@ static int rdt_mon_features_show(struct kernfs_open_file *of,
    >         struct rdt_resource *r = of->kn->parent->priv;
    >         struct mon_evt *mevt;
    >
    > -       list_for_each_entry(mevt, &r->evt_list, list)
    > +       list_for_each_entry(mevt, &r->evt_list, list) {
    >                 seq_printf(seq, "%s\n", mevt->name);
    > +               if (r->mon_configurable)
    > +                       seq_printf(seq, "%s\n", mevt->config);
    > +       }
    >
    >         return 0;
    >  }
    >
    > Is that the idea?


    I do not see why struct rdt_resource->configurable is needed. Again, this
    is a resource wide property with an implicit meaning related to only two
    event counters. Again, what if AMD later makes the llc_occupancy event counter
    configurable? How can resctrl know, using "r->mon_configurable" whether
    it should print mevt->config?

    How about:
    if (mevt->config)
    seq_printf(seq, "%s\n", mevt->config);

    As I mentioned in [1], mevt->config can be set in rdt_get_mon_l3_config()
    based on a check on the BMEC feature instead of hardcoded as it is now.
    Or, if the string manipulation is of concern the hardcoding of mevt->config
    (perhaps then mevt->config_name) could remain and a new mevt->configurable
    could be set from rdt_get_mon_l3_config() and then the above could be:

    if (mevt->configurable)
    seq_printf(seq, "%s\n", mevt->config_name);

    Reinette

    [1] https://lore.kernel.org/lkml/c5777707-746e-edab-2ce2-3405ff55be56@intel.com/

    \
     
     \ /
      Last update: 2022-08-25 23:25    [W:5.461 / U:0.136 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site