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

From: David Vernet
Date: Thu Aug 08 2024 - 14:19:40 EST


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