Re: [PATCH v2 3/3] sched: increase SCHED_LOAD_SCALE resolution

[Nikhil Rao]

On Wed, May 18, 2011 at 1:26 PM, Ingo Molnar <mingo@xxxxxxx> wrote:
>
> * Nikhil Rao <ncrao@xxxxxxxxxx> wrote:
>
>> +#if BITS_PER_LONG > 32
>> +# define SCHED_LOAD_RESOLUTION Â Â Â 10
>> +# define scale_load(w) Â Â Â Â Â Â Â (w << SCHED_LOAD_RESOLUTION)
>> +# define scale_load_down(w) Â(w >> SCHED_LOAD_RESOLUTION)
>> +#else
>> +# define SCHED_LOAD_RESOLUTION Â Â Â 0
>> +# define scale_load(w) Â Â Â Â Â Â Â (w)
>> +# define scale_load_down(w) Â(w)
>> +#endif
>
> We want (w) in the other definitions as well, to protect potential operators
> with lower precedence than <<. (Roughly half of the C operators are such so
> it's a real issue, should anyone use these macros with operators.)
>
>> +#define SCHED_LOAD_SHIFT Â Â (10 + (SCHED_LOAD_RESOLUTION))
>
> that () is not needed actually, if you look at the definition of
> SCHED_LOAD_RESOLUTION.
>
> So you could move the superfluous () from here up to the two definitions above
> and thus no parentheses would be hurt during the making of this patch.
>

Ah, thanks for the explanation. Does this look OK?

diff --git a/include/linux/sched.h b/include/linux/sched.h
index f2f4402..c34a718 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -788,9 +788,28 @@ enum cpu_idle_type {
 };

 /*
- * Increase resolution of nice-level calculations:
+ * Increase resolution of nice-level calculations for 64-bit architectures.
+ * The extra resolution improves shares distribution and load balancing of
+ * low-weight task groups (eg. nice +19 on an autogroup), deeper taskgroup
+ * hierarchies, especially on larger systems. This is not a user-visible change
+ * and does not change the user-interface for setting shares/weights.
+ *
+ * We increase resolution only if we have enough bits to allow this increased
+ * resolution (i.e. BITS_PER_LONG > 32). The costs for increasing resolution
+ * when BITS_PER_LONG <= 32 are pretty high and the returns do not justify the
+ * increased costs.
  */
-#define SCHED_LOAD_SHIFT       10
+#if BITS_PER_LONG > 32
+# define SCHED_LOAD_RESOLUTION 10
+# define scale_load(w)         ((w) << SCHED_LOAD_RESOLUTION)
+# define scale_load_down(w)    ((w) >> SCHED_LOAD_RESOLUTION)
+#else
+# define SCHED_LOAD_RESOLUTION 0
+# define scale_load(w)         (w)
+# define scale_load_down(w)    (w)
+#endif
+
+#define SCHED_LOAD_SHIFT       (10 + SCHED_LOAD_RESOLUTION)
 #define SCHED_LOAD_SCALE       (1L << SCHED_LOAD_SHIFT)

 /*


>> + Â Â Â Â Â Â if (BITS_PER_LONG > 32 && unlikely(w >= WMULT_CONST))
>> Â Â Â Â Â Â Â Â Â Â Â lw->inv_weight = 1;
>> + Â Â Â Â Â Â else if (unlikely(!w))
>> + Â Â Â Â Â Â Â Â Â Â lw->inv_weight = WMULT_CONST;
>> Â Â Â Â Â Â Â else
>> + Â Â Â Â Â Â Â Â Â Â lw->inv_weight = WMULT_CONST / w;
>
> Ok, i just noticed that you made use of BITS_PER_LONG here too.
>
> It's better to put that into a helper define, something like
> SCHED_LOAD_HIGHRES, which could thus be used like this:
>
> Â Â Â Â Â Â Â Âif (SCHED_LOAD_HIGHRES && unlikely(w >= WMULT_CONST))
>
> then, should anyone want to tweak the condition for SCHED_LOAD_HIGHRES, it
> could be done in a single place. It would also self-document.
>

Hmm, that particular use of BITS_PER_LONG was not touched by this
patch. This patch only changes lw->weight to use the local variable w.
The (BITS_PER_LONG & > WMULT_CONST) check is required on 64-bit
systems irrespective of the load-resolution changes.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/