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

[Peter Newman]

Hi Reinette,

On Tue, Apr 18, 2023 at 11:53 PM Reinette Chatre
<reinette.chatre@xxxxxxxxx> 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 if
> > +      * either kernfs_node is a file.
> > +      */
> > +     if (kernfs_type(kn) != KERNFS_DIR ||
> > +         kernfs_type(new_parent) != KERNFS_DIR) {
> > +             rdt_last_cmd_puts("Source and destination must be group directories");
>
> 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 = -EPERM;
> > +             goto out;
> > +     }
> > +
> > +     if ((rdtgrp->flags & RDT_DELETED) || (new_prdtgrp->flags & RDT_DELETED)) {
> > +             ret = -ENOENT;
> > +             goto out;
> > +     }
> > +
> > +     if (rdtgrp->type != RDTMON_GROUP || !kn->parent ||
> > +         !is_mon_groups(kn->parent, kn->name)) {
> > +             rdt_last_cmd_puts("Source must be a MON group\n");
> > +             ret = -EPERM;
> > +             goto out;
> > +     }
> > +
> > +     if (!is_mon_groups(new_parent, new_name)) {
> > +             rdt_last_cmd_puts("Destination must be a mon_groups subdirectory\n");
> > +             ret = -EPERM;
> > +             goto out;
> > +     }
> > +
>
> Thanks. I think using these terms ("MON" and "mon_groups") in the error messages
> are useful since it gives the user something to search for in the documentation.
>
> > +     /*
> > +      * 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 != new_prdtgrp)) {
>
> You can remove the extra parentheses so that this patch can get a clean slate
> 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@xxxxxxxxx>

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