Re: [PATCH 1/5] sched/fair: Convert cfs bandwidth throttling to use guards

[Benjamin Segall]

K Prateek Nayak <kprateek.nayak@xxxxxxx> writes:

> Routine conversion of rcu_read_lock(), spin_lock*, and rq_lock usage
> within the cfs bandwidth controller to use class guards.
>
> Only notable changes are:
>
> - Conversion of do_sched_cfs_slack_timer() to return a bool and moving
>   the call to distribute_cfs_runtime() into the caller for a cleaner
>   guard usage.
>
> - Reordering of list_del_rcu() against throttled_clock indicator update
>   in unthrottle_cfs_rq(). Both are done with "cfs_b->lock" held after
>   the "cfs_rq->throttled" is cleared which make the reordering safe
>   against concurrent list modifications.
>
> No functional changes intended.

Some minor style nitpicks that I don't care /that/ much about, so:

Reviewed-By: Ben Segall <bsegall@xxxxxxxxxx>

> @@ -7011,40 +7006,33 @@ static bool distribute_cfs_runtime(struct cfs_bandwidth *cfs_b)
>  		if (cfs_rq->runtime_remaining > 0) {
>  			if (cpu_of(rq) != this_cpu) {
>  				unthrottle_cfs_rq_async(cfs_rq);
> -			} else {
> -				/*
> -				 * We currently only expect to be unthrottling
> -				 * a single cfs_rq locally.
> -				 */
> -				WARN_ON_ONCE(!list_empty(&local_unthrottle));
> -				list_add_tail(&cfs_rq->throttled_csd_list,
> -					      &local_unthrottle);
> +				continue;
>  			}
> -		} else {
> -			throttled = true;
> +
> +			/*
> +			 * We currently only expect to be unthrottling
> +			 * a single cfs_rq locally.
> +			 */
> +			WARN_ON_ONCE(!list_empty(&local_unthrottle));
> +			list_add_tail(&cfs_rq->throttled_csd_list, &local_unthrottle);
> +			continue;
>  		}
>  
> -next:
> -		rq_unlock_irqrestore(rq, &rf);
> +		throttled = true;
>  	}

I don't think the extra continues wind up being clearer here than the
original if/else code and just removing the next/unlock.

> @@ -7197,29 +7185,25 @@ static __always_inline void return_cfs_rq_runtime(struct cfs_rq *cfs_rq)
>   * This is done with a timer (instead of inline with bandwidth return) since
>   * it's necessary to juggle rq->locks to unthrottle their respective cfs_rqs.
>   */
> -static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> +static bool do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
>  {
>  	u64 runtime = 0, slice = sched_cfs_bandwidth_slice();
> -	unsigned long flags;
>  
>  	/* confirm we're still not at a refresh boundary */
> -	raw_spin_lock_irqsave(&cfs_b->lock, flags);
> +	guard(raw_spinlock_irqsave)(&cfs_b->lock);
> +
>  	cfs_b->slack_started = false;
>  
> -	if (runtime_refresh_within(cfs_b, min_bandwidth_expiration)) {
> -		raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> -		return;
> -	}
> +	if (runtime_refresh_within(cfs_b, min_bandwidth_expiration))
> +		return false;
>  
>  	if (cfs_b->quota != RUNTIME_INF && cfs_b->runtime > slice)
>  		runtime = cfs_b->runtime;
>  
> -	raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> -
>  	if (!runtime)
> -		return;
> +		return false;
>  
> -	distribute_cfs_runtime(cfs_b);
> +	return true;
>  }
>  
>  /*

Here I think it's also nicer to just use a scoped_guard rather than move
part of it out and have part of the work done outside of the do_ helper.