Messages in this thread | | | Date | Wed, 26 Oct 2016 13:16:17 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH 4/6 v5] sched: propagate load during synchronous attach/detach |
| |
On Wed, Oct 26, 2016 at 09:05:49AM +0200, Vincent Guittot wrote: > > > > The 'detach across' and 'attach across' in detach_task_cfs_rq() and > > attach_entity_cfs_rq() do the same so couldn't you not create a > > function propagate_foo() for it? This would avoid this ifdef as > > well. > > > > You could further create in your '[PATCH 1/6 v5] sched: factorize attach entity': > > > > detach_entity_cfs_rq() { > > update_load_avg() > > detach_entity_load_avg() > > update_tg_load_avg() > > propagate_load_avg() > > } > > > > and then we would have: > > > > attach_task_cfs_rq() -> attach_entity_cfs_rq() -> propagate_foo() > > detach_task_cfs_rq() -> detach_entity_cfs_rq() -> propagate_foo() > > > > I guess you didn't because it would be only called one time but this > > symmetric approaches are easier to remember (at least for me). > > Yes i haven't created attach_entity_cfs_rq because it would be used only once. > Regarding the creation of a propagate_foo function, i have just > followed a similar skeleton as what is done in > enqueue/dequeue_task_fair > > I don't have strong opinion about creating this indirection for code > readability. Others, have you got a preference ?
I think I agree with Dietmar. Duplicate code needs a helper function and it would be nice to keep symmetry, even if there's only a single call site.
| |