Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1590339 > unrolled thread

Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata Thomas Gleixner <tglx@linutronix.de> - 2017-03-01 15:10 +0100
    Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata Shivappa Vikas <vikas.shivappa@linux.intel.com> - 2017-03-10 01:10 +0100
      Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata Thomas Gleixner <tglx@linutronix.de> - 2017-03-10 12:00 +0100
        Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata Shivappa Vikas <vikas.shivappa@intel.com> - 2017-03-10 19:30 +0100
          Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata Thomas Gleixner <tglx@linutronix.de> - 2017-03-10 20:00 +0100
            Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata "Luck, Tony" <tony.luck@intel.com> - 2017-03-10 23:10 +0100
              Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata Thomas Gleixner <tglx@linutronix.de> - 2017-03-11 08:50 +0100

#1590339 — Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata

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

[toc] | [next] | [standalone]

#1596499

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

Do the users want to see the data in the same order they entered ? If we want to 
do that then its easy to implement when we enforce the order .. (because right 
now resources are always displayed in a predefined order).
Otherwise i can drop this patch.

Thanks,
Vikas

>
> Thanks,
>
> 	tglx
>

[toc] | [prev] | [next] | [standalone]

#1597082

On Thu, 9 Mar 2017, Shivappa Vikas wrote:

> 
> > Anyway, first of all we want to know WHY this ordering is required. If it's
> > not required then why would we enforce it?
> 
> Do the users want to see the data in the same order they entered ? If we want
> to do that then its easy to implement when we enforce the order .. (because
> right now resources are always displayed in a predefined order).
> Otherwise i can drop this patch.

It's fine to display them in a defined order, but there is no point to
enforce the ordering on write.

The real question here is whether we really have to write every line on
every update. IMO it's sufficient to write a single resource line and not
require to update all resources every time.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1598010

On Fri, 10 Mar 2017, Thomas Gleixner wrote:

> On Thu, 9 Mar 2017, Shivappa Vikas wrote:
>
>>
>>> Anyway, first of all we want to know WHY this ordering is required. If it's
>>> not required then why would we enforce it?
>>
>> Do the users want to see the data in the same order they entered ? If we want
>> to do that then its easy to implement when we enforce the order .. (because
>> right now resources are always displayed in a predefined order).
>> Otherwise i can drop this patch.
>
> It's fine to display them in a defined order, but there is no point to
> enforce the ordering on write.
>
> The real question here is whether we really have to write every line on
> every update. IMO it's sufficient to write a single resource line and not
> require to update all resources every time.

Ok in that case we can drop this. because my thought was that user wants to see 
the contents he wrote when he overwrites the whole file like below , IOW its 
wierd for user to do

# echo "L3:0=0xff;1=0xf0" > /sys/fs/resctrl/schemata
then

# cat /sys/fs/resctrl/schemata
L3:0=0xff;1=0xf0
L2:0=0xff;1=0xf0
MB:....

But he did not write the L2,MB when he did an overwrite of the whole file.


Thanks,
Vikas

>
> Thanks,
>
> 	tglx
>
>
>

[toc] | [prev] | [next] | [standalone]

#1598028

On Fri, 10 Mar 2017, Shivappa Vikas wrote:
> On Fri, 10 Mar 2017, Thomas Gleixner wrote:
> > It's fine to display them in a defined order, but there is no point to
> > enforce the ordering on write.
> > 
> > The real question here is whether we really have to write every line on
> > every update. IMO it's sufficient to write a single resource line and not
> > require to update all resources every time.
> 
> Ok in that case we can drop this. because my thought was that user wants to
> see the contents he wrote when he overwrites the whole file like below , IOW
> its wierd for user to do
> 
> # echo "L3:0=0xff;1=0xf0" > /sys/fs/resctrl/schemata
> then
> 
> # cat /sys/fs/resctrl/schemata
> L3:0=0xff;1=0xf0
> L2:0=0xff;1=0xf0
> MB:....
> 
> But he did not write the L2,MB when he did an overwrite of the whole file.

Well, we have several options to tackle this:

1) Have schemata files for each resource

   schemata_l2, _l3 _mb

2) Request a full overwrite every time (all entries required)

   That still does not require ordering

3) Allow full overwrite and 'append' mode

   echo "...." > schemata

   Overwrites the whole file. It does not require all entries to be
   supplied.  Non supplied entries are reset to default

   echo "...." >> schemata
		 
   "Appends" the supplied entries by overwriting the existing ones.

My favourite would be #1, but I have no strong opinions other than not
caring about resource write ordering for #2 and #3.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1598137

On Fri, Mar 10, 2017 at 07:58:51PM +0100, Thomas Gleixner wrote:
> Well, we have several options to tackle this:
> 
> 1) Have schemata files for each resource
> 
>    schemata_l2, _l3 _mb
> 
> 2) Request a full overwrite every time (all entries required)
> 
>    That still does not require ordering
> 
> 3) Allow full overwrite and 'append' mode
> 
>    echo "...." > schemata
> 
>    Overwrites the whole file. It does not require all entries to be
>    supplied.  Non supplied entries are reset to default
> 
>    echo "...." >> schemata
> 		 
>    "Appends" the supplied entries by overwriting the existing ones.
> 
> My favourite would be #1, but I have no strong opinions other than not
> caring about resource write ordering for #2 and #3.

If you are going to head in the direction of partial update, then
why not go for:

4) Drop the code that check that the user wrote all
   the fields as well as the check for all the lines. Just update
   the bits they list, and leave the rest unchanged.

I.e. the user could say:

# echo "L3:1=0x3f" > schemata

if they just wanted to update resource L3, instance 1.

I don't think there is much benefit to the overwrite vs. append
semantics for the user.

-Tony

[toc] | [prev] | [next] | [standalone]

#1598285

On Fri, 10 Mar 2017, Luck, Tony wrote:
> On Fri, Mar 10, 2017 at 07:58:51PM +0100, Thomas Gleixner wrote:
> > Well, we have several options to tackle this:
> > 
> > 1) Have schemata files for each resource
> > 
> >    schemata_l2, _l3 _mb
> > 
> > 2) Request a full overwrite every time (all entries required)
> > 
> >    That still does not require ordering
> > 
> > 3) Allow full overwrite and 'append' mode
> > 
> >    echo "...." > schemata
> > 
> >    Overwrites the whole file. It does not require all entries to be
> >    supplied.  Non supplied entries are reset to default
> > 
> >    echo "...." >> schemata
> > 		 
> >    "Appends" the supplied entries by overwriting the existing ones.
> > 
> > My favourite would be #1, but I have no strong opinions other than not
> > caring about resource write ordering for #2 and #3.
> 
> If you are going to head in the direction of partial update, then
> why not go for:
> 
> 4) Drop the code that check that the user wrote all
>    the fields as well as the check for all the lines. Just update
>    the bits they list, and leave the rest unchanged.
> 
> I.e. the user could say:
> 
> # echo "L3:1=0x3f" > schemata
> 
> if they just wanted to update resource L3, instance 1.

Even better

> I don't think there is much benefit to the overwrite vs. append
> semantics for the user.

Agreed.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web