Date: Wed, 1 Mar 2017 15:03:03 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Vikas Shivappa <vikas.shivappa@linux.intel.com>
cc: vikas.shivappa@intel.com, linux-kernel@vger.kernel.org, x86@kernel.org,
        hpa@zytor.com, mingo@kernel.org, peterz@infradead.org,
        ravi.v.shankar@intel.com, tony.luck@intel.com, fenghua.yu@intel.com,
        andi.kleen@intel.com
Subject: Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata
In-Reply-To: <1487360328-6768-3-git-send-email-vikas.shivappa@linux.intel.com>
Message-ID: <alpine.DEB.2.20.1703011431480.4005@nanos>
References: <1487360328-6768-1-git-send-email-vikas.shivappa@linux.intel.com> <1487360328-6768-3-git-send-email-vikas.shivappa@linux.intel.com>
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: 4079
Lines: 144

On Fri, 17 Feb 2017, Vikas Shivappa wrote:

> The schemata file requires all RDT (Resource director technology)
> resources be entered in the same order they are shown in the root
> schemata file.
> Hence remove the looping through all resources while parsing each
> schemata and get the next enabled resource after processing a resource.

Again, you desribe WHAT you are doing and not WHY.

 x86/intel_rdt: Improveme schemata parsing

  The schemata file requires all resources be written in the same order
  as they are shown in the root schemata file.

  The current parser searches all resources to find a matching resource for
  each resource line in the schemata file. This is suboptimal as the order
  of the resources is fixed.

  Avoid the repeating lookups by walking the resource descriptors linearly
  while processing the input lines.

So that would describe again the context, the problem and the solution in a
precise and understandable way. It's not that hard.

Though, I have to ask the question WHY is that required. It's neither
required by the current implementation nor by anything else. The current
implementation can nicely deal with any ordering of the resource lines due
to the lookup. So now the question is, what makes this change necessary and
what's the advantage of doing it this way?

> +/*
> + * Parameter r must be NULL or pointing to
> + * a valid rdt_resource_all entry.
> + * returns next enabled RDT resource.

New sentences start with an upper case letter. Aside of that, please do not
make arbitrary line breaks at randomly chosen locations. Use the 80 chars
estate unless there is a structural reason not to do so.

> +static inline struct rdt_resource*
> +get_next_enabled_rdt_resource(struct rdt_resource *r)
> +{
> +	struct rdt_resource *it = r;
> +
> +	if (!it)
> +		it = rdt_resources_all;
> +	else
> +		it++;
> +	for (; it < rdt_resources_all + RDT_NUM_RESOURCES; it++)
> +		if (it->enabled)
> +			return it;

Once more. This lacks curly braces around the for() construct. See

  http://lkml.kernel.org/r/alpine.DEB.2.20.1701171956290.3645@nanos

But that's the least of the problems with this code.

> +
> +	return NULL;
> +}
> +
>  ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>  				char *buf, size_t nbytes, loff_t off)
>  {
> @@ -171,22 +192,21 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>  		r->num_tmp_cbms = 0;
>  	}
>  
> +	r = NULL;
>  	while ((tok = strsep(&buf, "\n")) != NULL) {
>  		resname = strsep(&tok, ":");
>  		if (!tok) {
>  			ret = -EINVAL;
>  			goto out;
>  		}
> -		for_each_enabled_rdt_resource(r) {
> -			if (!strcmp(resname, r->name) &&
> -			    closid < r->num_closid) {
> -				ret = parse_line(tok, r);
> -				if (ret)
> -					goto out;
> -				break;
> -			}
> -		}
> -		if (!r->name) {
> +
> +		r = get_next_enabled_rdt_resource(r);
> +
> +		if (r && !strcmp(resname, r->name)) {
> +			ret = parse_line(tok, r);
> +			if (ret)
> +				goto out;
> +		} else {
>  			ret = -EINVAL;
>  			goto out;
>  		}

I really have a hard time to figure out, why this convoluted
get_next_enabled_rdt_resource() is better than what we have now.

The write function is hardly a hot path and enforcing the resource write
ordering is questionable at best.

If there is a real reason to do so, then this can be written way less
convoluted.

static struct rdt_resource *get_enabled_resource(struct rdt_resource *r)
{
	for (; r < rdt_resources_all + RDT_NUM_RESOURCES; r++) {
		if (r->enabled)
			return r;
}

ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 				char *buf, size_t nbytes, loff_t off)
{
.....

	r = get_enabled_resource(rdt_resources_all);

 	while (r && (tok = strsep(&buf, "\n")) != NULL) {
		ret = -EINVAL;
		
 		resname = strsep(&tok, ":");
 		if (!tok || strcmp(resname, r->name))
 			goto out;

		ret = parse_line(tok, r);
		if (ret)
			goto out;

		r = get_enabled_resource(++r);
	}

Can you spot the difference?

Anyway, first of all we want to know WHY this ordering is required. If it's
not required then why would we enforce it?

Thanks,

	tglx