Messages in this thread | | | From | Vincent Guittot <> | Date | Wed, 26 Oct 2016 14:31:26 +0200 | Subject | Re: [PATCH 4/6 v5] sched: propagate load during synchronous attach/detach |
| |
On 26 October 2016 at 13:16, Peter Zijlstra <peterz@infradead.org> wrote: > 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.
OK
| |