Messages in this thread | | | Date | Sat, 17 Aug 2013 02:07:10 +0900 | Subject | Re: [PATCH v3 3/3] sched: clean-up struct sd_lb_stat | From | JoonSoo Kim <> |
| |
2013/8/15 Peter Zijlstra <peterz@infradead.org>: > 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@lge.com> > > 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.
| |