Re: [PATCH 6/6] sched_ext: Don't hold scx_tasks_lock for too long

[Waiman Long]

On 10/10/24 5:38 PM, Tejun Heo wrote:
> Hello,
>
> On Thu, Oct 10, 2024 at 02:12:37PM -0500, David Vernet wrote:
> ...
> > >   static struct task_struct *scx_task_iter_next(struct scx_task_iter *iter)
> > >   {
> > >   	struct list_head *cursor = &iter->cursor.tasks_node;
> > >   	struct sched_ext_entity *pos;
> > >   
> > > -	lockdep_assert_held(&scx_tasks_lock);
> > > +	if (!(++iter->cnt % SCX_OPS_TASK_ITER_BATCH)) {
> > > +		scx_task_iter_unlock(iter);
> > > +		cpu_relax();
> > Could you explain why we need this cpu_relax()? I thought it was only
> > necessary for busy-wait loops.
> I don't think we need them with the current queued spinlock implementation
> but back when the spinlocks were dumb, just unlocking and relocking could
> still starve out others.
>
> cc'ing Waiman who should know a lot better than me. Waiman, what's the
> current state? When releasing a spinlock to give others a chance, can we
> just do unlock/lock or is cpu_relax() still useful in some cases?

I agree with David that cpu_relax() isn't needed here. Queued spinlock 
is a fair lock. If there is a pending waiter, the current task will have 
to wait in the wait queue even if it attempts to acquire the lock again 
immediately  after an unlock.

A cpu_relax() is only useful in a spin loop in order to reduce the 
frequency of pounding the same cacheline again and again.

Cheers,
Longman