[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALPaoCgiFCG8FcBS5Svj63oeqbmARByCGh9ox-ZPUY-6gkBMqg@mail.gmail.com>
Date: Thu, 30 Mar 2023 15:43:57 +0200
From: Peter Newman <peternewman@...gle.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: fenghua.yu@...el.com, Babu.Moger@....com, bp@...en8.de,
dave.hansen@...ux.intel.com, eranian@...gle.com,
gupasani@...gle.com, hpa@...or.com, james.morse@....com,
linux-kernel@...r.kernel.org, mingo@...hat.com, skodak@...gle.com,
tglx@...utronix.de, tony.luck@...el.com, x86@...nel.org
Subject: Re: [PATCH v4 3/3] x86/resctrl: Implement rename op for mon groups
Hi Reinette,
On Thu, Mar 23, 2023 at 7:08 PM Reinette Chatre
<reinette.chatre@...el.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 = new_prdtgrp;
> > + rdtgrp->closid = 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 *new_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 if
> > + * either kernfs_node is a file.
> > + */
> > + if (kernfs_type(kn) != KERNFS_DIR ||
> > + kernfs_type(new_parent) != 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 added
> 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 maybe
> 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
Powered by blists - more mailing lists