Date: Fri, 7 Jul 2017 08:44:58 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Shivappa Vikas <vikas.shivappa@intel.com>
cc: Vikas Shivappa <vikas.shivappa@linux.intel.com>, x86@kernel.org,
        linux-kernel@vger.kernel.org, hpa@zytor.com, peterz@infradead.org,
        "Shankar, Ravi V" <ravi.v.shankar@intel.com>, tony.luck@intel.com,
        "Yu, Fenghua" <fenghua.yu@intel.com>
Subject: Re: [PATCH 13/21] x86/intel_rdt/cqm: Add cpus file support
In-Reply-To: <alpine.DEB.2.10.1707061432511.3798@vshiva-Udesk>
Message-ID: <alpine.DEB.2.20.1707070822550.1871@nanos>
References: <1498503368-20173-1-git-send-email-vikas.shivappa@linux.intel.com> <1498503368-20173-14-git-send-email-vikas.shivappa@linux.intel.com> <alpine.DEB.2.20.1707021313150.2296@nanos> <alpine.DEB.2.10.1707061432511.3798@vshiva-Udesk>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2737
Lines: 80

On Thu, 6 Jul 2017, Shivappa Vikas wrote:
> On Sun, 2 Jul 2017, Thomas Gleixner wrote:
> > > +	/* Check whether cpus belong to parent ctrl group */
> > > +	cpumask_andnot(tmpmask, newmask, &pr->cpu_mask);
> > > +	if (cpumask_weight(tmpmask)) {
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +
> > > +	/* Check whether cpus are dropped from this group */
> > > +	cpumask_andnot(tmpmask, &rdtgrp->cpu_mask, newmask);
> > > +	if (cpumask_weight(tmpmask)) {
> > > +		/* Give any dropped cpus to parent rdtgroup */
> > > +		cpumask_or(&pr->cpu_mask, &pr->cpu_mask, tmpmask);
> > 
> > This does not make any sense. The check above verifies that all cpus in
> > newmask belong to the parent->cpu_mask. If they don't then you return
> > -EINVAL, but here you give them back to parent->cpu_mask. How is that
> > supposed to work? You never get into this code path!
> 
> The parent->cpu_mask always is the parent->cpus_valid_mask if i understand
> right. With monitor group, the cpu is present is always present in "one"
> ctrl_mon group and one mon_group. And the mon group can have only cpus in its
> parent. May be it needs a comment? (its explaind in the documentation patch).

Sigh, the code needs to be written in a way that it is halfways obvious
what's going on.

> # mkdir /sys/fs/resctrl/p1
> # mkdir /sys/fs/resctrl/p1/mon_groups/m1
> # echo 5-10 > /sys/fs/resctr/p1/cpus_list
> Say p1 has RMID 2
> cpus 5-10 have RMID 2

So what you say, is that parent is always the resource control group
itself.

Can we please have a proper distinction in the code? I tripped over that
ambigiousities several times.

The normal meaning of parent->child relations is that both have the same
type. While this is the case at the implementation detail level (both are
type struct rdtgroup), from a conceptual level they are different:

  parent is a resource group and child is a monitoring group

That should be expressed in the code, at the very least by variable naming,
so it becomes immediately clear that this operates on two different
entities.

The proper solution is to have different data types or at least embedd the
monitoring bits in a seperate entity inside of struct rdtgroup.

struct mongroup {
	monitoring stuff;
};

struct rdtgroup {
	common stuff;
	struct mongroup mon;
};

So the code can operate on r->mon.foo or mon->foo which makes it entirely
clear what kind of operation this is.

Sigh, cramming everything into a single struct without distinction is the
same as operating on a pile of global variables, which is the most common
pattern used by people learning C. You certainly belong not to that group,
so dammit, get your act together and structure the code so it's obvious and
maintainable.

Thanks,

	tglx