linux-kernel - Re: [PATCH v6 3/8] x86/resctrl: Prepare for different scope for control/monitor operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALPaoCgb35dGiC87e_cqTOXKKZ-nQcokWou+u=mC7m2gR36G8w@mail.gmail.com>
Date:   Fri, 29 Sep 2023 15:10:18 +0200
From:   Peter Newman <peternewman@...gle.com>
To:     Tony Luck <tony.luck@...el.com>
Cc:     Fenghua Yu <fenghua.yu@...el.com>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Jonathan Corbet <corbet@....net>,
        Shuah Khan <skhan@...uxfoundation.org>, x86@...nel.org,
        Shaopeng Tan <tan.shaopeng@...itsu.com>,
        James Morse <james.morse@....com>,
        Jamie Iles <quic_jiles@...cinc.com>,
        Babu Moger <babu.moger@....com>,
        Randy Dunlap <rdunlap@...radead.org>,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        patches@...ts.linux.dev
Subject: Re: [PATCH v6 3/8] x86/resctrl: Prepare for different scope for
 control/monitor operations

Hi Tony,

On Thu, Sep 28, 2023 at 9:14 PM Tony Luck <tony.luck@...el.com> wrote:
>
> @@ -352,7 +355,7 @@ struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r)
>  {
>         struct rdt_domain *d;
>
> -       list_for_each_entry(d, &r->domains, hdr.list) {
> +       list_for_each_entry(d, &r->ctrl_domains, hdr.list) {

If someone were to call get_domain_from_cpu() looking for a
mon_domain, I don't think they'd be happy with the result.

This problem seems adequately addressed by the next patch where a type
mismatch on the return value would result.

In any case, perhaps the name could be updated to set expectations better.

> @@ -549,44 +552,101 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
>
>         rdt_domain_reconfigure_cdp(r);
>
> -       if (r->alloc_capable && domain_setup_ctrlval(r, d)) {
> +       if (domain_setup_ctrlval(r, d)) {
>                 domain_free(hw_dom);
>                 return;
>         }
>
> -       if (r->mon_capable && arch_domain_mbm_alloc(r->num_rmid, hw_dom)) {
> +       list_add_tail(&d->hdr.list, add_pos);
> +
> +       err = resctrl_online_ctrl_domain(r, d);
> +       if (err) {
> +               list_del(&d->hdr.list);
>                 domain_free(hw_dom);
> +       }
> +}
> +
> +static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> +{
> +       int id = get_domain_id_from_scope(cpu, r->mon_scope);
> +       struct list_head *add_pos = NULL;
> +       struct rdt_hw_domain *hw_mondom;

It's still hw_dom in domain_add_cpu_ctrl(), so why hw_mondom here?

> @@ -3711,16 +3711,16 @@ static void domain_destroy_mon_state(struct rdt_domain *d)
>         kfree(d->mbm_local);
>  }
>
> -void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d)
> +void resctrl_offline_ctrl_domain(struct rdt_resource *r, struct rdt_domain *d)
>  {
>         lockdep_assert_held(&rdtgroup_mutex);
>
>         if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA)
>                 mba_sc_domain_destroy(r, d);
> +}
>
> -       if (!r->mon_capable)
> -               return;
> -
> +void resctrl_offline_mon_domain(struct rdt_resource *r, struct rdt_domain *d)
> +{
>         /*
>          * If resctrl is mounted, remove all the
>          * per domain monitor data directories.

We did a lockdep_assert_held(&rdtgroup_mutex) for both types before.
Should we continue to do so here?

> --
> 2.41.0
>

In the resctrl2 prototype I complained that resctrl_resource was
awkwardly disjoint in its support for control and monitoring
groups[1]. In this patch, you seem to have already done most of the
hard work in separating the control and monitoring functionality, so
taking the next step and using a different structure to represent
control and monitoring resources would further improve the code by
statically typing monitoring and control resources, which would be
less error-prone than run-time checks on the alloc_capable and
mon_capable fields, which seem easy to forget.

I don't think this is necessary to complete SNC support, but it would
give me confidence that there isn't a misplaced {alloc,mon}_capable
check resulting in the wrong domain list being traversed. I will
probably do this myself later if you don't.

Thanks!
-Peter

[1] https://lore.kernel.org/all/CALPaoCj_oa=nATvOO_uysYvu+PdTQtd0pvssvm9_M1+fP-Z8JA@mail.gmail.com/