Re: [PATCH -next 8/8] block, bfq: cleanup bfq_bfqq_handle_idle_busy_switch()

[Jan Kara <jack@suse.cz>]

On Sat 14-05-22 17:05:22, Yu Kuai wrote:
> 'wr_or_deserves_wr' is only used in bfq_update_bfqq_wr_on_rq_arrival(),
> which is only called from bfq_bfqq_handle_idle_busy_switch() in specific
> code branch, thus there is no need to precaculate 'wr_or_deserves_wr'
> each time bfq_bfqq_handle_idle_busy_switch() is called.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

With this patch there's the same problem as with the previous one. Some of
the variables passed to bfq_update_bfqq_wr_on_rq_arrival() (in_burst,
soft_rt) would actually evaluate differently later in the function.

								Honza

> ---
>  block/bfq-iosched.c | 110 +++++++++++++++++++++++++-------------------
>  1 file changed, 62 insertions(+), 48 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 1e57d76c8dd3..cea8cb3f5ee2 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -1624,15 +1624,65 @@ static unsigned long bfq_smallest_from_now(void)
>  	return jiffies - MAX_JIFFY_OFFSET;
>  }
>  
> +/*
> + * bfqq deserves to be weight-raised if:
> + * - it is sync,
> + * - it does not belong to a large burst,
> + * - it has been idle for enough time or is soft real-time,
> + * - is linked to a bfq_io_cq (it is not shared in any sense),
> + * - has a default weight (otherwise we assume the user wanted
> + *   to control its weight explicitly)
> + *
> + * Merged bfq_queues are kept out of weight-raising
> + * (low-latency) mechanisms. The reason is that these queues
> + * are usually created for non-interactive and
> + * non-soft-real-time tasks. Yet this is not the case for
> + * stably-merged queues. These queues are merged just because
> + * they are created shortly after each other. So they may
> + * easily serve the I/O of an interactive or soft-real time
> + * application, if the application happens to spawn multiple
> + * processes. So let also stably-merged queued enjoy weight
> + * raising.
> + */
> +static bool bfqq_wr_or_deserves_wr(struct bfq_data *bfqd,
> +				   struct bfq_queue *bfqq,
> +				   struct request *rq,
> +				   bool interactive, bool soft_rt)
> +{
> +	if (!bfqd->low_latency)
> +		return false;
> +
> +	if (bfqq->wr_coeff > 1)
> +		return true;
> +
> +	if (!bfq_bfqq_sync(bfqq))
> +		return false;
> +
> +	if (!bfqq->bic && !RQ_BIC(rq)->stably_merged)
> +		return false;
> +
> +	if (!interactive && !soft_rt)
> +		return false;
> +
> +	return true;
> +}
> +
>  static void bfq_update_bfqq_wr_on_rq_arrival(struct bfq_data *bfqd,
>  					     struct bfq_queue *bfqq,
>  					     unsigned int old_wr_coeff,
> -					     bool wr_or_deserves_wr,
> -					     bool interactive,
> -					     bool in_burst,
> -					     bool soft_rt)
> -{
> -	if (old_wr_coeff == 1 && wr_or_deserves_wr) {
> +					     struct request *rq,
> +					     bool interactive)
> +{
> +	bool in_burst = bfq_bfqq_in_large_burst(bfqq);
> +	bool soft_rt = bfqd->bfq_wr_max_softrt_rate > 0 &&
> +		       !BFQQ_TOTALLY_SEEKY(bfqq) &&
> +		       !in_burst &&
> +		       time_is_before_jiffies(bfqq->soft_rt_next_start) &&
> +		       bfqq->dispatched == 0 &&
> +		       bfqq->entity.new_weight == 40;
> +
> +	if (old_wr_coeff == 1 &&
> +	    bfqq_wr_or_deserves_wr(bfqd, bfqq, rq, interactive, soft_rt)) {
>  		/* start a weight-raising period */
>  		if (interactive) {
>  			bfqq->service_from_wr = 0;
> @@ -1674,9 +1724,9 @@ static void bfq_update_bfqq_wr_on_rq_arrival(struct bfq_data *bfqd,
>  		if (interactive) { /* update wr coeff and duration */
>  			bfqq->wr_coeff = bfqd->bfq_wr_coeff;
>  			bfqq->wr_cur_max_time = bfq_wr_duration(bfqd);
> -		} else if (in_burst)
> +		} else if (in_burst) {
>  			bfqq->wr_coeff = 1;
> -		else if (soft_rt) {
> +		} else if (soft_rt) {
>  			/*
>  			 * The application is now or still meeting the
>  			 * requirements for being deemed soft rt.  We
> @@ -1768,44 +1818,11 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd,
>  					     struct request *rq,
>  					     bool *interactive)
>  {
> -	bool soft_rt, in_burst,	wr_or_deserves_wr,
> -		idle_for_long_time = bfq_bfqq_idle_for_long_time(bfqd, bfqq);
> +	bool in_burst = bfq_bfqq_in_large_burst(bfqq);
> +	bool idle_for_long_time = bfq_bfqq_idle_for_long_time(bfqd, bfqq);
>  
> -	/*
> -	 * bfqq deserves to be weight-raised if:
> -	 * - it is sync,
> -	 * - it does not belong to a large burst,
> -	 * - it has been idle for enough time or is soft real-time,
> -	 * - is linked to a bfq_io_cq (it is not shared in any sense),
> -	 * - has a default weight (otherwise we assume the user wanted
> -	 *   to control its weight explicitly)
> -	 */
> -	in_burst = bfq_bfqq_in_large_burst(bfqq);
> -	soft_rt = bfqd->bfq_wr_max_softrt_rate > 0 &&
> -		!BFQQ_TOTALLY_SEEKY(bfqq) &&
> -		!in_burst &&
> -		time_is_before_jiffies(bfqq->soft_rt_next_start) &&
> -		bfqq->dispatched == 0 &&
> -		bfqq->entity.new_weight == 40;
>  	*interactive = !in_burst && idle_for_long_time &&
>  		bfqq->entity.new_weight == 40;
> -	/*
> -	 * Merged bfq_queues are kept out of weight-raising
> -	 * (low-latency) mechanisms. The reason is that these queues
> -	 * are usually created for non-interactive and
> -	 * non-soft-real-time tasks. Yet this is not the case for
> -	 * stably-merged queues. These queues are merged just because
> -	 * they are created shortly after each other. So they may
> -	 * easily serve the I/O of an interactive or soft-real time
> -	 * application, if the application happens to spawn multiple
> -	 * processes. So let also stably-merged queued enjoy weight
> -	 * raising.
> -	 */
> -	wr_or_deserves_wr = bfqd->low_latency &&
> -		(bfqq->wr_coeff > 1 ||
> -		 (bfq_bfqq_sync(bfqq) &&
> -		  (bfqq->bic || RQ_BIC(rq)->stably_merged) &&
> -		   (*interactive || soft_rt)));
>  
>  	/*
>  	 * If bfqq happened to be activated in a burst, but has been
> @@ -1840,11 +1857,8 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd,
>  		if (time_is_before_jiffies(bfqq->split_time +
>  					   bfqd->bfq_wr_min_idle_time)) {
>  			bfq_update_bfqq_wr_on_rq_arrival(bfqd, bfqq,
> -							 old_wr_coeff,
> -							 wr_or_deserves_wr,
> -							 *interactive,
> -							 in_burst,
> -							 soft_rt);
> +							 old_wr_coeff, rq,
> +							 *interactive);
>  
>  			if (old_wr_coeff != bfqq->wr_coeff)
>  				bfqq->entity.prio_changed = 1;
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR