Re: [PATCH 3/7] x86/intel_rdt: Add support for cache bit mask management

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 20 May 2015, Thomas Gleixner wrote:
> On Wed, 20 May 2015, Vikas Shivappa wrote:
> > On Mon, 18 May 2015, Thomas Gleixner wrote:
> > 
> > > On Mon, 18 May 2015, Vikas Shivappa wrote:
> > > > On Fri, 15 May 2015, Thomas Gleixner wrote:
> > > > > > +static inline bool intel_rdt_update_cpumask(int cpu)
> > > > > > +{
> > > > > > +	}
> > > > > 
> > > > > You must be kidding.
> > > > 
> > > > the rapl and cqm use similar code. You want me to keep a seperate package
> > > > mask
> > > > for this code which not would be that frequent at all ?
> > > 
> > > You find for everything a place where you copied your stuff from
> > > without thinking about it, right?
> > > 
> > > Other people dessperately try to fix the cpu online times which are
> > > more and more interesting the larger the systems become. So it might
> > > be a good idea to come up with a proper fast implementation which can
> > > be used everywhere instead of blindly copying code.
> > 
> > Ok , i can try to do this as a seperate patch after the cache allocation to
> 
> Hell no. We do preparatory patches first. I'm not believing in 'can
> try' promises.
> 
> > get a support for faster implementation for traversing package and cpus in the
> > packages which can be used by everyone. we would need to start from scratch
> > with having packagemask_t equivalent to cpumask_t. hope that is fair ?
> 
> Yes, that's what I want to see.

I was kidding. You need two functionalities:

  1) A way to figure out whether you already have a CPU taking care of the
     package to which the newly online CPU belongs.

     That's something you need to track in your own code and I already
     gave you the 5 lines of code which can handle that. Remember?

        id =  topology_physical_package_id(cpu);
        if (!__test_and_set_bit(id, &package_map)) {
                cpumask_set_cpu(cpu, &rdt_cpumask);
                cbm_update_msrs(cpu);
        }

     So you cannot add much infrastructure for that because you need
     to track your own state in the CAT relevant package bitmap.

  2) A way to find out whether the to be offlined CPU is the last
     online CPU of a package. If it's not the last one, then you need
     a fast way to find the next online cpu which belongs to that
     package. If it's the last one you need to kill the cat.

     The information is already available, so it's not rocket
     science. And it takes maximal 7 lines of code to implement
     it.

     Q: Why 'maximal' 7?
     A: Because I'm a lazy bastard and cannot be bothered to figure
     	out whether one of the lines is superfluous.
     Q: Why can't I be bothered?
     A: It's none of my problems and it actually does not matter much.

     Here is the pseudo code:

     	   do_magic_stuff(tmp, TCC, COM);
	   clr(c, tmp);
	   n = do_more_stuff(tmp);
     	   if (notavailable(n))
	      	kill_the_cat();
	   else
	        set(n, RCM);

     Hint: One of the NNN placeholders resolves to topology_core_cpumask()

Now once you figured that out, you will notice that the above 5
lines of code to solve problem #1 can be simplified because you can
avoid the package_map bitmap completely.

Sorry, no pseudo code for this. But you get more than one hint this
time:
	- Hint #1 still applies
	- The logic is very similar to the above #2 pseudo code
	- It takes maximal 6 lines of code to implement it

There is one caveat:

    If Hint #1 cannot solve your problem, then you need to figure out
    why and then work with the people who are responsible for it to
    figure out how it can be resolved.

A few general hints:

    - The line counts are neither including the conditionals which
      invoke that code nor the function body nor variable
      declarations. But they include braces,

      All I care about is the logic itself. See the pseudo code
      example above.

    - Please provide a solution for #2 and #1 before you bother me
      with another patch series.

Thanks,

	tglx