Messages in this thread | | | Date | Thu, 21 May 2015 09:36:54 -0700 (PDT) | From | Vikas Shivappa <> | Subject | Re: [PATCH 3/7] x86/intel_rdt: Add support for cache bit mask management |
| |
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 > > > > > >
| |