Re: [PATCH v3 3/3] sched: clean-up struct sd_lb_stat

[JoonSoo Kim]

2013/8/15 Peter Zijlstra <peterz@xxxxxxxxxxxxx>:
> On Tue, Aug 06, 2013 at 05:36:43PM +0900, Joonsoo Kim wrote:
>> There is no reason to maintain separate variables for this_group
>> and busiest_group in sd_lb_stat, except saving some space.
>> But this structure is always allocated in stack, so this saving
>> isn't really benificial.
>>
>> This patch unify these variables, so IMO, readability may be improved.
>>
>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
>
> So I like the idea I had to reformat some of the code and I think we can
> do less memsets. See how the below reduces the sds memset by the two
> sgs. If we never set busiest we'll never look at sds->busiest_stat so we
> don't need to clear that. And if we make the sgs memset in
> update_sd_lb_stats() unconditional we'll cover sds->this_stats while
> reducing complexity.

At first glance, below changes look good.
When I return to the office on next Monday, I will look at it in detail.
Just one comment below.

> @@ -4890,13 +4893,12 @@ static inline void calculate_imbalance(s
>   *                return the least loaded group whose CPUs can be
>   *                put to idle by rebalancing its tasks onto our group.
>   */
> -static struct sched_group *
> -find_busiest_group(struct lb_env *env)
> +static struct sched_group *find_busiest_group(struct lb_env *env)
>  {
> -       struct sd_lb_stats sds;
>         struct sg_lb_stats *this, *busiest;
> +       struct sd_lb_stats sds;
>
> -       memset(&sds, 0, sizeof(sds));
> +       memset(&sds, 0, sizeof(sds) - 2*sizeof(struct sg_lb_stats));

How about using offsetof() macro, instead of using subtraction to
calculate size?

Thanks.
--
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/