Re: [PATCH v6] sched_ext: idle: Refresh idle masks during idle-to-idle transitions

[Tejun Heo]

Hello, Andrea.

On Fri, Jan 10, 2025 at 09:46:25AM +0100, Andrea Righi wrote:
...
> +	if (do_notify) {
> +		if (SCX_HAS_OP(update_idle) && !scx_rq_bypassing(rq))
> +			SCX_CALL_OP(SCX_KF_REST, update_idle, cpu_of(rq), idle);
> +	} else {
> +		bool is_prev_idle;
> +
> +		/* Refresh idle masks during idle-to-idle transitions */

Can you add a bit more explanation on what case this path is handling here
or in the function comment? The function comment explains what it's about
but doesn't quite explain the exact sequence which isn't very intuitive.

> +		rcu_read_lock();
> +		is_prev_idle = is_idle_task(rcu_dereference(rq->curr));
> +		rcu_read_unlock();
> +
> +		if (!is_prev_idle)
>  			return;

This function is always called under the rq lock, right? We can assert that
and skip the rcu dancing.

>  	}
>  
> +	if (!static_branch_likely(&scx_builtin_idle_enabled))
> +		return;

Would structure like the following be better? It makes clear that the last
condition checks are for the builtin idle path.

        if (SCX_HAS_OP(update_idle) && do_notify && !scx_rq_bypassing(rq))
                // call ops.update_idle().

        if (!scx_builtin_idle_enabled || (!do_notify && !is_idle_task(rq->curr)))
                return;

Thanks.

-- 
tejun