lkml.org 
[lkml]   [2022]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3 07/10] x86/resctrl: Add sysfs interface files to read/write event configuration
    From

    On 8/26/22 11:35, Reinette Chatre wrote:
    > Hi Babu,
    >
    > On 8/26/2022 9:07 AM, Moger, Babu wrote:
    >> On 8/24/22 16:15, Reinette Chatre wrote:
    >>> On 8/22/2022 6:43 AM, Babu Moger wrote:
    > ...
    >
    >>>> static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
    >>>> struct rdt_domain *d,
    >>>> struct rdt_resource *r, struct rdtgroup *prgrp)
    >>>> @@ -2568,6 +2591,15 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
    >>>> if (ret)
    >>>> goto out_destroy;
    >>>>
    >>>> + /* Create the sysfs event configuration files */
    >>>> + if (r->mon_configurable &&
    >>>> + (mevt->evtid == QOS_L3_MBM_TOTAL_EVENT_ID ||
    >>>> + mevt->evtid == QOS_L3_MBM_LOCAL_EVENT_ID)) {
    >>>> + ret = mon_config_addfile(kn, mevt->config, priv.priv);
    >>>> + if (ret)
    >>>> + goto out_destroy;
    >>>> + }
    >>>> +
    >>> This seems complex to have event features embedded in the code in this way. Could
    >>> the events not be configured during system enumeration? For example, instead
    >>> of hardcoding the config like above to always set:
    >>>
    >>> static struct mon_evt mbm_local_event = {
    >>> .name = "mbm_local_bytes",
    >>> .evtid = QOS_L3_MBM_LOCAL_EVENT_ID,
    >>> + .config = "mbm_local_config",
    >>>
    >>>
    >>> What if instead this information is dynamically set in rdt_get_mon_l3_config()? To
    >>> make things simpler struct mon_evt could get a new member "configurable" and the
    >>> events that actually support configuration will have this set only
    >>> if system has X86_FEATURE_BMEC (struct rdt_resource->configurable then
    >>> becomes unnecessary?). Being configurable thus becomes an event property, not
    >>> a resource property. The "config" member introduced here could then be "config_name".
    >>>
    >>> I think doing so will also make this file creation simpler with a single
    >>> mon_config_addfile() (possibly with more parameters) used to add both files to
    >>> avoid the code duplication introduced by mon_config_addfile() above.
    >>>
    >>> What do you think?
    >> Yes. We could do that. Something like this.
    >>
    >> struct mon_evt {
    >>         u32                     evtid;
    >>         char                    *name;
    >>
    >> +      bool                     configurable;
    >>
    >>          char                    *config;
    >>         struct list_head        list;
    >> };
    >>
    >> Set the configurable if  the  system has X86_FEATURE_BMEC feature in
    >> rdt_get_mon_l3_config.
    > This would work (using bool in struct is something resctrl already do
    > in many places). I also think that "config" should rather be named to
    > "config_name" to make clear that it is not the actual configuration of
    > the event.
    Sure.
    > Remember to update struct mon_evt's kerneldoc (I just noticed it is
    > missing from this series).

    Oh,, Will do.

    Thanks

    Babu


    >
    >> Create both files  mbm_local_bytes and  mbm_local_config in mon_addfile.
    >>
    >> Change the mon_addfile to pass mon_evt structure, so it have all
    >> information to create both the files.
    > Providing the structure to the function would make all the information
    > available but I am not sure that doing so would make it easy to eliminate the
    > duplicate code needed to create the other file. Giving more parameters
    > to mon_addfile() is another option but it should be more clear to
    > you as you write this code.
    >
    >> Then we can remove  rdt_resource->configurable. 
    >>
    >> Does that make sense?
    >>
    > Yes.
    >
    > Reinette
    >
    --
    Thanks
    Babu Moger

    \
     
     \ /
      Last update: 2022-08-26 18:58    [W:4.441 / U:0.236 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site