lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 19 Apr 2023 11:38:52 +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 v5 2/3] x86/resctrl: Implement rename op for mon groups

Hi Reinette,

On Tue, Apr 18, 2023 at 11:53 PM Reinette Chatre
<reinette.chatre@...el.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 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@...el.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ