Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230330135558.1019658-1-peternewman@google.com>
 <20230330135558.1019658-3-peternewman@google.com> <678ab8b6-9465-cdeb-da65-448c0cf075f5@intel.com>
In-Reply-To: <678ab8b6-9465-cdeb-da65-448c0cf075f5@intel.com>
From:   Peter Newman <peternewman@google.com>
Date:   Wed, 19 Apr 2023 11:38:52 +0200
Message-ID: <CALPaoCh=1Qq5sqWpY8We8zykmmvk81YVnigtZftqMH6FefnQjw@mail.gmail.com>
Subject: Re: [PATCH v5 2/3] x86/resctrl: Implement rename op for mon groups
To:     Reinette Chatre <reinette.chatre@intel.com>
Cc:     fenghua.yu@intel.com, Babu.Moger@amd.com, bp@alien8.de,
        dave.hansen@linux.intel.com, eranian@google.com,
        gupasani@google.com, hpa@zytor.com, james.morse@arm.com,
        linux-kernel@vger.kernel.org, mingo@redhat.com, skodak@google.com,
        tglx@linutronix.de, tony.luck@intel.com, x86@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Reinette,

On Tue, Apr 18, 2023 at 11:53=E2=80=AFPM Reinette Chatre
<reinette.chatre@intel.com> wrote:
> On 3/30/2023 6:55 AM, Peter Newman wrote:
> > If a container manager is additionally tracking containers' bandwidth
> > usage by placing tasks from each into their own monitoring group, it
>
> The above sentence seems to be missing something after the "for each".
> It seems to still parse if "for each" is removed.

Did you mean "from each"? In any case, I'll further disambiguate to
this in my next update:

"If the container manager is using monitoring groups to separately
track the bandwidth of containers assigned to the same control group,
it must first move the container's tasks to the default monitoring
group of the new control group before it can move these tasks into the
container's replacement monitoring groups under the destination
control group."

> > +     /*
> > +      * Don't allow kernfs_to_rdtgroup() to return a parent rdtgroup i=
f
> > +      * either kernfs_node is a file.
> > +      */
> > +     if (kernfs_type(kn) !=3D KERNFS_DIR ||
> > +         kernfs_type(new_parent) !=3D KERNFS_DIR) {
> > +             rdt_last_cmd_puts("Source and destination must be group d=
irectories");
>
> I do not think it is obvious what a "group directory" is. The source must=
 be a
> monitoring group and the destination must be the "mon_groups" directory. =
Maybe
> the "group" term can just be dropped to read "Source and destination must=
 be
> directories" (which is exactly what is tested for).

Sounds good.

>
> > +             ret =3D -EPERM;
> > +             goto out;
> > +     }
> > +
> > +     if ((rdtgrp->flags & RDT_DELETED) || (new_prdtgrp->flags & RDT_DE=
LETED)) {
> > +             ret =3D -ENOENT;
> > +             goto out;
> > +     }
> > +
> > +     if (rdtgrp->type !=3D RDTMON_GROUP || !kn->parent ||
> > +         !is_mon_groups(kn->parent, kn->name)) {
> > +             rdt_last_cmd_puts("Source must be a MON group\n");
> > +             ret =3D -EPERM;
> > +             goto out;
> > +     }
> > +
> > +     if (!is_mon_groups(new_parent, new_name)) {
> > +             rdt_last_cmd_puts("Destination must be a mon_groups subdi=
rectory\n");
> > +             ret =3D -EPERM;
> > +             goto out;
> > +     }
> > +
>
> Thanks. I think using these terms ("MON" and "mon_groups") in the error m=
essages
> are useful since it gives the user something to search for in the documen=
tation.
>
> > +     /*
> > +      * If the MON group is monitoring CPUs, the CPUs must be assigned=
 to the
> > +      * current parent CTRL_MON group and therefore cannot be assigned=
 to
> > +      * the new parent, making the move illegal.
> > +      */
> > +     if (!cpumask_empty(&rdtgrp->cpu_mask) &&
> > +         (rdtgrp->mon.parent !=3D new_prdtgrp)) {
>
> You can remove the extra parentheses so that this patch can get a clean s=
late
> from "checkpatch.pl --strict" done as this work moves to the next level.

Ok

>
> Thank you very much.
>
> Just the few minor comments. With those addressed:
>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Thanks again for your careful review. Also thank you for suggesting
this solution. It's a big improvement in maintainability over what
we've been using downstream.

-Peter