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: <20230308131452.383914-1-peternewman@google.com>
 <20230308131452.383914-4-peternewman@google.com> <4a276e2d-9f56-437d-8284-c95def0d2487@intel.com>
In-Reply-To: <4a276e2d-9f56-437d-8284-c95def0d2487@intel.com>
From:   Peter Newman <peternewman@google.com>
Date:   Thu, 30 Mar 2023 15:43:57 +0200
Message-ID: <CALPaoCgiFCG8FcBS5Svj63oeqbmARByCGh9ox-ZPUY-6gkBMqg@mail.gmail.com>
Subject: Re: [PATCH v4 3/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 Thu, Mar 23, 2023 at 7:08=E2=80=AFPM Reinette Chatre
<reinette.chatre@intel.com> wrote:
> On 3/8/2023 5:14 AM, Peter Newman wrote:
> > application container, a container manager must write all of the tasks'
> > IDs into the tasks file interface of the new control group.
> >
> > If a container manager is tracking containers' bandwidth usage by
>
> "is tracking" -> "is additionally"

That doesn't sound right. Did you mean "is additionally tracking"?


> > +     WARN_ON(list_empty(&prdtgrp->mon.crdtgrp_list));
> > +     list_del(&rdtgrp->mon.crdtgrp_list);
> > +
> > +     list_add_tail(&rdtgrp->mon.crdtgrp_list,
> > +                   &new_prdtgrp->mon.crdtgrp_list);
>
> Could list_move_tail() be used here?

Yes

>
> > +     rdtgrp->mon.parent =3D new_prdtgrp;
> > +     rdtgrp->closid =3D new_prdtgrp->closid;
> > +
> > +     /* Propagate updated closid to all tasks in this group. */
> > +     rdt_move_group_tasks(rdtgrp, rdtgrp, cpus, mongrp_reparent_match)=
;
> > +
> > +     update_closid_rmid(cpus, NULL);
> > +}
> > +
> > +static int rdtgroup_rename(struct kernfs_node *kn,
> > +                        struct kernfs_node *new_parent, const char *ne=
w_name)
> > +{
> > +     struct rdtgroup *new_prdtgrp;
> > +     struct rdtgroup *rdtgrp;
> > +     cpumask_var_t tmpmask;
> > +     int ret;
> > +
> > +     /*
> > +      * 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)
> > +             return -EPERM;
>
> This would be one scenario where the user may attempt an interaction
> with resctrl that results in an error while peeking at "last_cmd_status"
> will report "ok". This is not the only case in which this may happen and
> I think the code is ok. To help users to not need to read the kernel code
> to understand what is going on, could a snippet about this feature be add=
ed
> to the "Resource alloc and monitor groups" section in
> Documentation/x86/rescrl.rst. It does not have to be elaborate
> but in the area where directory removal is discussed there could be
> a snippet that documents this new feature.

I only skipped the last_cmd_status update here because it was before
obtaining the lock. Even though it doesn't need the lock for this check,
I don't see anything wrong moving it until after the lock has been
obtained.

I'll also add some documentation.


> > +
> > +     if (!zalloc_cpumask_var(&tmpmask, GFP_KERNEL))
> > +             return -ENOMEM;
>
> It remains strange to do the allocation here. I understand its usage so m=
aybe
> just a comment like: "Perform early allocation as part
> of ensuring the later resource group move cannot fail."

I think the other reason I put it so early was so all the other error
exits could free it unconditionally. The other clean approach is
allocating it last before calling kernfs_rename(). That should be easier
to follow.

I will try to address all of your other comments.

Thanks again for your review!

-Peter