Re: [PATCH v3 05/18] Documentation, x86: Documentation for Intel resource allocation user interface

From: "Luck, Tony" <tony.luck@intel.com>
To: Nilay Vaish <nilayvaish@gmail.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	"H. Peter Anvin" <h.peter.anvin@intel.com>,
	Ingo Molnar <mingo@elte.hu>,
	Peter Zijlstra <peterz@infradead.org>,
	Stephane Eranian <eranian@google.com>,
	Borislav Petkov <bp@suse.de>, Dave Hansen <dave.hansen@intel.com>,
	Shaohua Li <shli@fb.com>,
	David Carrillo-Cisneros <davidcc@google.com>,
	Ravi V Shankar <ravi.v.shankar@intel.com>,
	Sai Prakhya <sai.praneeth.prakhya@intel.com>,
	Vikas Shivappa <vikas.shivappa@linux.intel.com>,
	linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>
Subject: Re: [PATCH v3 05/18] Documentation, x86: Documentation for Intel resource allocation user interface
Date: Tue, 11 Oct 2016 11:04:07 -0700	[thread overview]
Message-ID: <20161011180406.GA13932@intel.com> (raw)
In-Reply-To: <CACbG30-oAftpUZ0K0VdtN-HacWiQfD8ZXU5EvY_NYB4Vsr0gUQ@mail.gmail.com>

On Tue, Oct 11, 2016 at 12:07:33PM -0500, Nilay Vaish wrote:
> On 10 October 2016 at 12:19, Luck, Tony <tony.luck@intel.com> wrote:
> > The next resource coming will have values that are simple ranges {0 .. max}
> >
> 
> Regarding addition of more resources, I was wondering if one common
> definition of struct rdt_resource to be used for each resource the way
> to take.  As you point out, L2 caches will not support CDP, but cdp
> would be part of the variable describing L2 cache.  Should we have
> separate structs for each resource?

Yes and no.

CDP is currently the only real difference between L2 and L3, and
the current code will just never set the @cdp_capable field to true.
It seems less of a burdern to carry two extra "bool" fields in the
resource that won't use them, than to architect some complex scheme
that allows different resources to have different structures.

The structure essentially has to be the same for each resource
so that we can do:

	for_each_rdt_resource(r) {
		something()
	}

all over the code (from bringing cpus online and taking them off again
to deciding which lines to print in the schemata file). There are over
a dozen places where this happens, and it is the key to making it easy
to add support for new resources.

However, I expect the structure to morph as new resources require a
different "something()".  E.g. validating a new resource limit value in
the schemata file. Right now the function that does that has the weird
rules about consecutive bits in the bitmask hard wired into it. When
we get a resource that allows a simple range, we'll want a simpler
function. Spoiler: that resource also expects "0" as the default value
which doesn't restrict accesss, bigger numbers limiting access more.
So we may also want an "initial_value", or "default_value" field so the
discovery and cleanup code can set the per resource MSR array correctly.

So we'll add some per-resource flags. Perhaps the validation function
will be a function pointer in the structure. If there are many such
differences, we might end up with all the common fields at the front
of the structure, and a union of structures with the per resource type
bits. But we can make that call as we add new resources.

-Tony