Re: [PATCH] sched: Avoid unnecessary work in reweight_entity

[Michael Wang <wangyun@linux.vnet.ibm.com>]

On 02/06/2012 05:37 PM, Michael Wang wrote:

> From: Michael Wang <wangyun@linux.vnet.ibm.com>
> 
> In original code, using account_entity_dequeue and account_entity_enqueue
> as a pair will do some work unnecessary, this patch combine the work of
> them to save time.
> 
> Signed-off-by: Michael Wang <wangyun@linux.vnet.ibm.com>
> ---
>  kernel/sched/fair.c  |   32 ++++++++++++++++++++++++--------
>  kernel/sched/sched.h |    8 ++++++++
>  2 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 84adb2d..e380518 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -782,11 +782,23 @@ add_cfs_task_weight(struct cfs_rq *cfs_rq, unsigned long weight)
>  {
>  	cfs_rq->task_weight += weight;
>  }
> +
> +static void
> +sub_add_cfs_task_weight(struct cfs_rq *cfs_rq, unsigned long sub, unsigned long add)
> +{
> +	cfs_rq->task_weight -= sub;
> +	cfs_rq->task_weight += add;
> +}
>  #else
>  static inline void
>  add_cfs_task_weight(struct cfs_rq *cfs_rq, unsigned long weight)
>  {
>  }
> +
> +static void
> +sub_add_cfs_task_weight(struct cfs_rq *cfs_rq, unsigned long sub, unsigned long add)
> +{
> +}
>  #endif
>  
>  static void
> @@ -938,20 +950,24 @@ static inline void update_entity_shares_tick(struct cfs_rq *cfs_rq)
>  {
>  }
>  # endif /* CONFIG_SMP */
> +
>  static void reweight_entity(struct cfs_rq *cfs_rq, struct sched_entity *se,
>  			    unsigned long weight)
>  {
> +	/* commit outstanding execution time */
> +	if (cfs_rq->curr == se)
> +		update_curr(cfs_rq);

I suppose "se is curr" means "se is already on rq", and "se is not on
rq" means "se can not be curr", so I change the logic up.

> +

>  	if (se->on_rq) {
> -		/* commit outstanding execution time */
> -		if (cfs_rq->curr == se)
> -			update_curr(cfs_rq);
> -		account_entity_dequeue(cfs_rq, se);
> +		update_load_sub_add(&cfs_rq->load, se->load.weight, weight);
> +		if (!parent_entity(se)) {
> +			update_load_sub_add(&rq_of(cfs_rq), se->load.weight, weight);
> +		}
> +		if (entity_is_task(se)) {
> +			sub_add_cfs_task_weight(cfs_rq, se->load.weight, weight);
> +		}
>  	}
> -
>  	update_load_set(&se->load, weight);

I wonder will this sequence change result some mistake, but I think this
function should already be protected.

Thanks,
Michael Wang

> -
> -	if (se->on_rq)
> -		account_entity_enqueue(cfs_rq, se);
>  }
>  
>  static void update_cfs_shares(struct cfs_rq *cfs_rq)
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 98c0c26..ec4430f 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -779,6 +779,14 @@ static inline void update_load_sub(struct load_weight *lw, unsigned long dec)
>  	lw->inv_weight = 0;
>  }
>  
> +static inline void update_load_sub_add(struct load_weight *lw, unsigned long dec,
> +					unsigned long inc)
> +{
> +	lw->weight -= dec;
> +	lw->weight += inc;
> +	lw->inv_weight = 0;
> +}
> +
>  static inline void update_load_set(struct load_weight *lw, unsigned long w)
>  {
>  	lw->weight = w;