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

[Vikas Shivappa <vikas.shivappa@intel.com>]

On Wed, 20 May 2015, Thomas Gleixner wrote:

> 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 a lot for the very detailed expectations. I appreciate your time. Will 
work on the requirements and send a patch before anything else.

Vikas

>
> Thanks,
>
> 	tglx
>
>
>
>
>
>