Skip to content

scx_lavd: restrict direct dispatch to usable, uncongested CPUs - #3735

Open
multics69 wants to merge 3 commits into
sched-ext:mainfrom
multics69:pr2-direct-dispatch
Open

scx_lavd: restrict direct dispatch to usable, uncongested CPUs#3735
multics69 wants to merge 3 commits into
sched-ext:mainfrom
multics69:pr2-direct-dispatch

Conversation

@multics69

Copy link
Copy Markdown
Contributor

can_direct_dispatch()'s low-utilization bypass decides purely on average
utilization, so it can drop a task into the (non-stealable) local DSQ of
a CPU where it will not run promptly. Refuse the bypass in two such cases.

  • a CPU running an RT/DL task, unusable to scx until the higher class yields;
  • a congested CPU, where already-queued work would be jumped ahead of.

Also carries a no-functional-change prep: pass cpdom_id to
try_find_and_kick_victim_cpu() directly instead of re-deriving it.

Tested the change with the memtier benchmark on a server with two AMD
EPYC 9R45 (96-core x2). Overall, this PR lowers the tail latency while
providing a similar level of throughput, especially when system
utilization is low.

Utilization Metric baseline (main) this PR delta
50% ops/sec 509,056 509,588 0.1% higher
50% avg_lat 0.45 0.46 2.4% higher
50% p99.9 7.08 6.93 2.2% lower
25% ops/sec 251,971 251,995 0.0% higher
25% avg_lat 0.44 0.43 0.3% lower
25% p99.9 5.21 4.29 17.7% lower
  • [1/3] scx_lavd: pass cpdom_id to try_find_and_kick_victim_cpu()
  • [2/3] scx_lavd: avoid CPUs taken by RT/DL tasks for direct dispatch
  • [3/3] scx_lavd: do not direct-dispatch to a congested CPU

The function received a DSQ id only to derive cpdom_id via
dsq_to_cpdom() and look up cpdom_ctxs / cpdom_cpumask. Pass
cpdom_id directly; the caller already has it on cpuc->cpdom_id.

No functional change intended.

Signed-off-by: Changwoo Min <changwoo@igalia.com>
can_direct_dispatch()'s low-utilization bypass inserts a task into a
CPU's local DSQ based on avg_util_wall alone. If that CPU is running an
RT/DL task, the dispatched task is parked in the (non-stealable) local
DSQ and waits out the entire higher-class runtime.
scx_bpf_reenqueue_local() does not help here because it is called only
at the SCX -> RT/DL transition.

Guard the low-utilization bypass with is_rt_or_dl_task_running() so a
CPU currently running an RT/DL task is not chosen for direct dispatch.

Signed-off-by: Changwoo Min <changwoo@igalia.com>
can_direct_dispatch() decides whether a task can skip the
deadline-ordered DSQs and go straight to a CPU's local DSQ -- either
because the CPU is idle with nothing queued, or via the
lb-local-dsq-util-pct low-utilization bypass. The bypass, however,
only looks at average utilization: it can insert into the local DSQ
of a CPU that already has tasks waiting across its DSQs, letting the
new task jump ahead of already-queued work.

Add is_cpu_congested(), which reports whether at least
LAVD_CPU_CONGESTED_THRES tasks are waiting across the CPU's DSQs
(local, per-CPU, and the cpdom's regular and turbulent DSQs), and
refuse direct dispatch when the CPU is congested.

Signed-off-by: Changwoo Min <changwoo@igalia.com>
return lb_local_dsq_util_wall > 0 &&
cpuc->avg_util_wall < lb_local_dsq_util_wall &&
!is_cpu_congested(cpuc) &&
!is_rt_or_dl_task_running(cpuc->cpu_id);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking another case could be worth a revisit—the lock holder condition. It seems that we currently don't have any restriction on the lock holder, which cannot be preempted. If the lock holder runs for a long time, it could impact the tasks waiting in the DSQs. And if it's a real bug (meaning that the lock holder runs indefinitely), it could trigger a soft lockup or other errors. I think I was overthinking the problem. Overall, it looks good to me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think avoiding a CPU that holds a lock for direct dispatch makes sense. I will experiment it.

}

__hidden
bool is_cpu_congested(struct cpu_ctx *cpuc)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was gazing at the function for a while and comparing it with the queued_on_cpu. When LAVD_CPU_CONGESTED_THRES == 1, it appears to me that the logic is almost the same except for the order of the if condition. I think the function is paving the road for future enhancements for possible longer congestions with different scenarios. There could be a case where a higher-priority task (or any kind of urgency) is chiming in and jumping the queue based on the congestion length.

@daidavid

Copy link
Copy Markdown
Contributor

I'm curious what the benchmark numbers are if we only allow dispatch on idle. I think using util is an incorrect metric to determine when to direct dispatch. Util doesn't provide any info on the duty_cycle or wake up timings, so low util tasks that match up wake up timings will result in contention anyways. I'm okay with just disabling the util bypass unless there's some benefit there.

@multics69

Copy link
Copy Markdown
Contributor Author

I'm curious what the benchmark numbers are if we only allow dispatch on idle. I think using util is an incorrect metric to determine when to direct dispatch. Util doesn't provide any info on the duty_cycle or wake up timings, so low util tasks that match up wake up timings will result in contention anyways. I'm okay with just disabling the util bypass unless there's some benefit there.

I agree that the util is not quite the right metric to decide the direct dispatch. In my experience, the direct dispatch is a double-edged sword; it helps improve the throughput by reducing the DSQ overhead, but when it is done at the wrong moment, it significantly increases the latency. So far, the util metrics have been used to opportunistically increase the chance of direct dispatch. I will revisit this once again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants