Re: [PATCH 4/6 v5] sched: propagate load during synchronous attach/detach

[Vincent Guittot]

On 26 October 2016 at 13:16, Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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