Re: [PATCH RESEND sched_ext/for-6.12] sched_ext: Don't use double locking to migrate tasks across CPUs

[David Vernet]

On Wed, Aug 07, 2024 at 01:15:19PM -1000, Tejun Heo wrote:

Hi Tejun,

[...]

> -static bool move_task_to_local_dsq(struct rq *rq, struct task_struct *p,
> -				   u64 enq_flags)
> +static bool move_task_to_local_dsq(struct task_struct *p, u64 enq_flags,
> +				   struct rq *src_rq, struct rq *dst_rq)
>  {
> -	struct rq *task_rq;
> -
> -	lockdep_assert_rq_held(rq);
> +	lockdep_assert_rq_held(src_rq);
>  
>  	/*
> -	 * If dequeue got to @p while we were trying to lock both rq's, it'd
> -	 * have cleared @p->scx.holding_cpu to -1. While other cpus may have
> -	 * updated it to different values afterwards, as this operation can't be
> +	 * If dequeue got to @p while we were trying to lock @src_rq, it'd have
> +	 * cleared @p->scx.holding_cpu to -1. While other cpus may have updated
> +	 * it to different values afterwards, as this operation can't be
>  	 * preempted or recurse, @p->scx.holding_cpu can never become
>  	 * raw_smp_processor_id() again before we're done. Thus, we can tell
>  	 * whether we lost to dequeue by testing whether @p->scx.holding_cpu is
>  	 * still raw_smp_processor_id().
>  	 *
> +	 * @p->rq couldn't have changed if we're still the holding cpu.
> +	 *
>  	 * See dispatch_dequeue() for the counterpart.
>  	 */
> -	if (unlikely(p->scx.holding_cpu != raw_smp_processor_id()))
> +	if (unlikely(p->scx.holding_cpu != raw_smp_processor_id()) ||
> +	    WARN_ON_ONCE(src_rq != task_rq(p))) {
> +		raw_spin_rq_unlock(src_rq);
> +		raw_spin_rq_lock(dst_rq);
>  		return false;
> +	}
>  
> -	/* @p->rq couldn't have changed if we're still the holding cpu */
> -	task_rq = task_rq(p);
> -	lockdep_assert_rq_held(task_rq);
> -
> -	WARN_ON_ONCE(!cpumask_test_cpu(cpu_of(rq), p->cpus_ptr));
> -	deactivate_task(task_rq, p, 0);
> -	set_task_cpu(p, cpu_of(rq));
> -	p->scx.sticky_cpu = cpu_of(rq);
> +	/* the following marks @p MIGRATING which excludes dequeue */
> +	deactivate_task(src_rq, p, 0);
> +	set_task_cpu(p, cpu_of(dst_rq));
> +	p->scx.sticky_cpu = cpu_of(dst_rq);
> +
> +	raw_spin_rq_unlock(src_rq);
> +	raw_spin_rq_lock(dst_rq);
>  
>  	/*
>  	 * We want to pass scx-specific enq_flags but activate_task() will
>  	 * truncate the upper 32 bit. As we own @rq, we can pass them through
>  	 * @rq->scx.extra_enq_flags instead.
>  	 */
> -	WARN_ON_ONCE(rq->scx.extra_enq_flags);
> -	rq->scx.extra_enq_flags = enq_flags;
> -	activate_task(rq, p, 0);
> -	rq->scx.extra_enq_flags = 0;
> +	WARN_ON_ONCE(!cpumask_test_cpu(cpu_of(dst_rq), p->cpus_ptr));

Hmmm, what's to stop someone from issuing a call to
set_cpus_allowed_ptr() after we drop the src_rq lock above?  Before we
held any relevant rq lock so everything should have been protected, but
I'm not following how we prevent racing with the cpus_allowed logic in
core.c here.

> +	WARN_ON_ONCE(dst_rq->scx.extra_enq_flags);
> +	dst_rq->scx.extra_enq_flags = enq_flags;
> +	activate_task(dst_rq, p, 0);
> +	dst_rq->scx.extra_enq_flags = 0;
>  
>  	return true;
>  }

Thanks,
David

Attachment: signature.asc
Description: PGP signature