lkml.org 
[lkml]   [2010]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2] powerpc: implement arch_scale_smt_power for Power7

    >> +
    >> +static inline int thread_in_smt4core(int x)
    >> +{
    >> + return x % 4;
    >> +}
    >>
    >
    > Needs a whitespace here though I don't really like the above. Any reason
    > why you can't use the existing cpu_thread_in_core() ?
    >
    I will change it to cpu_thread_in_core()
    >
    >> +unsigned long arch_scale_smt_power(struct sched_domain *sd, int cpu)
    >> +{
    >> + int cpu2;
    >> + int idle_count = 0;
    >> +
    >> + struct cpumask *cpu_map = sched_domain_span(sd);
    >> +
    >> + unsigned long weight = cpumask_weight(cpu_map);
    >> + unsigned long smt_gain = sd->smt_gain;
    >>
    >
    > More whitespace damage above.
    >
    You are better than checkpatch.pl, will fix.
    >
    >> + if (cpu_has_feature(CPU_FTRS_POWER7) && weight == 4) {
    >> + for_each_cpu(cpu2, cpu_map) {
    >> + if (idle_cpu(cpu2))
    >> + idle_count++;
    >> + }
    >>
    >
    > I'm not 100% sure about the use of the CPU feature above. First I wonder
    > if the right approach is to instead do something like
    >
    > if (!cpu_has_feature(...) !! weigth < 4)
    > return default_scale_smt_power(sd, cpu);
    >
    > Though we may be better off using a ppc_md. hook here to avoid
    > calculating the weight etc... on processors that don't need any
    > of that.
    >
    > I also dislike your naming. I would suggest you change cpu_map to
    > sibling_map() and cpu2 to sibling (or just c). One thing I wonder is how
    > sure we are that sched_domain_span() is always going to give us the
    > threads btw ? If we introduce another sched domain level for NUMA
    > purposes can't we get confused ?
    >
    Right now it's 100% always giving us threads. My development version of
    the patch had a BUG_ON() to check this. I expect this to stay the case
    in the future as the name of the function is arch_scale_smt_power(),
    which clearly denotes threads are expected.

    I am not stuck on the names, I'll change it to sibling instead of cpu2
    and sibling_map instead of cpu_map. It seems clear to me either way.

    As for testing the ! case it seems funcationally equivalent, and mine
    seems less confusing.

    Having a ppc.md hook with exactly 1 user is pointless, especially since
    you'll still have to calculate the weight with the ability to
    dynamically disable smt.

    > Also, how hot is this code path ?
    >
    It's every load balance, which is to say not hot, but fairly frequent.
    I haven't been able to measure an impact from doing very hairy
    calculations (without actually changing the weights) here vs not having
    it at all in actual end workloads.
    >
    >> + /* the following section attempts to tweak cpu power based
    >> + * on current idleness of the threads dynamically at runtime
    >> + */
    >> + if (idle_count == 2 || idle_count == 3 || idle_count == 4) {
    >>
    >
    > if (idle_count > 1) ? :-)
    >
    Yes :) Originally I had done different weightings for each of the 3
    cases, which gained in some workloads but regressed some others. But
    since I'm not doing that anymore I'll fold it down to > 1


    \
     
     \ /
      Last update: 2010-01-20 23:39    [W:0.026 / U:29.576 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site