Re: [PATCH 3/6] sched_ext: Make @rf optional for dispatch_to_local_dsq()

[Tejun Heo <tj@kernel.org>]

Hello,

On Wed, Jul 10, 2024 at 11:10:25AM -0500, David Vernet wrote:
> >  static void dispatch_to_local_dsq_lock(struct rq *rq, struct rq_flags *rf,
> >  				       struct rq *src_rq, struct rq *dst_rq)
> >  {
> > -	rq_unpin_lock(rq, rf);
> > +	if (rf)
> > +		rq_unpin_lock(rq, rf);
> >  
> >  	if (src_rq == dst_rq) {
> >  		raw_spin_rq_unlock(rq);
> >  		raw_spin_rq_lock(dst_rq);
> >  	} else if (rq == src_rq) {
> >  		double_lock_balance(rq, dst_rq);
> > -		rq_repin_lock(rq, rf);
> > +		if (rf)
> > +			rq_repin_lock(rq, rf);
> >  	} else if (rq == dst_rq) {
> >  		double_lock_balance(rq, src_rq);
> > -		rq_repin_lock(rq, rf);
> > +		if (rf)
> > +			rq_repin_lock(rq, rf);
> 
> It feels kind of weird to have the callee need to know about pinning
> requirements in the caller instead of vice versa. I mean, I guess it's inherent
> to doing an unpin / repin inside of a locked region, but it'd be nice if we
> could minimize the amount of variance in that codepath regardless. I think what
> you have is correct, but maybe it'd simpler if we just pinned in the caller on
> the enqueue path? That way the semantics of when locks can be dropped is
> consistent in dispatch_to_local_dsq().

Yeah, it's kinda nasty. I think going the other direction probalby is
better. The reason why we're doing unpin/repin down in the callstack is
because this is being called from sched_class->balance() which is invoked
with rq locked and pinned, but also with @rf so that the lock can be
unpinned. There isn't much that can benefit from extending rq lock pinning
deeper into balance implementation, so it probably makes more sense to do so
in the outer balance function so that the inner functions don't have to
worry about it.

Thanks.

-- 
tejun