Re: [PATCH v4 3/3] x86/resctrl: Implement rename op for mon groups

[Peter Newman]

Hi Reinette,

On Thu, Mar 23, 2023 at 7:08 PM Reinette Chatre
<reinette.chatre@xxxxxxxxx> 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