Messages in this thread | | | Date | Wed, 13 Jul 2022 12:34:22 -0700 | Subject | Re: [PATCH] sched/fair: no sync wakeup from interrupt context | From | Libo Chen <> |
| |
linux-kernel@vger.kernel.org complains about html subpart... I am resending it in plain-text only so everybody on the mailing list can see.
On 7/13/22 12:17, Libo Chen wrote: > > > On 7/13/22 09:40, Tim Chen wrote: >> On Mon, 2022-07-11 at 15:47 -0700, Libo Chen wrote: >>> Barry Song first pointed out that replacing sync wakeup with regular wakeup >>> seems to reduce overeager wakeup pulling and shows noticeable performance >>> improvement.[1] >>> >>> This patch argues that allowing sync for wakeups from interrupt context >>> is a bug and fixing it can improve performance even when irq/softirq is >>> evenly spread out. >>> >>> For wakeups from ISR, the waking CPU is just the CPU of ISR and the so-called >>> waker can be any random task that happens to be running on that CPU when the >>> interrupt comes in. This is completely different from other wakups where the >>> task running on the waking CPU is the actual waker. For example, two tasks >>> communicate through a pipe or mutiple tasks access the same critical section, >>> etc. This difference is important because with sync we assume the waker will >>> get off the runqueue and go to sleep immedately after the wakeup. The >>> assumption is built into wake_affine() where it discounts the waker's presence >>> from the runqueue when sync is true. The random waker from interrupts bears no >>> relation to the wakee and don't usually go to sleep immediately afterwards >>> unless wakeup granularity is reached. Plus the scheduler no longer enforces the >>> preepmtion of waker for sync wakeup as it used to before >>> patch f2e74eeac03ffb7 ("sched: Remove WAKEUP_SYNC feature"). Enforcing sync >>> wakeup preemption for wakeups from interrupt contexts doesn't seem to be >>> appropriate too but at least sync wakeup will do what it's supposed to do. >> Will there be scenarios where you do want the task being woken up be pulled >> to the CPU where the interrupt happened, as the data that needs to be accessed is >> on local CPU/NUMA that interrupt happened? For example, interrupt associated with network >> packets received. Sync still seems desirable, at least if there is no task currently >> running on the CPU where interrupt happened. So maybe we should have some consideration >> of the load on the CPU/NUMA before deciding whether we should do sync wake for such >> interrupt. >> > There are only two places where sync wakeup matters: > wake_affine_idle() and wake_affine_weight(). > In wake_affine_idle(), it considers pulling if there is one runnable > on the waking CPU because > of the belief that this runnable will voluntarily get off the > runqueue. In wake_affine_weight(), > it basically takes off the waker's load again assuming the waker goes > to sleep after the wakeup. > My argument is that this assumption doesn't really hold for wakeups > from the interrupt contexts > when the waking CPU is non-idle. Wakeups from task context? sure, it > seems to be a reasonable > assumption. For your idle case, I totally agree but I don't think > having sync or not will actually > have any impacts here giving what the code does. Real impact comes > fromMel's patch 7332dec055f2457c3 > which makes it less likely to pull tasks when the waking CPU is idle. > I believe we should consider > reverting 7332dec055f2 because a significant RDS latency regression > has been spotted recently on our > system due to this patch. > >>> Add a check to make sure that sync can only be set when in_task() is true. If >>> a wakeup is from interrupt context, sync flag will be 0 because in_task() >>> returns 0. >>> >>> Tested in two scenarios: wakeups from 1) task contexts, expected to see no >>> performance changes; 2) interrupt contexts, expected to see better performance >>> under low/medium load and no regression under heavy load. >>> >>> Use hackbench for scenario 1 and pgbench for scenarios 2 both from mmtests on >>> a 2-socket Xeon E5-2699v3 box with 256G memory in total. Running 5.18 kernel >>> with SELinux disabled. Scheduler/MM tunables are all default. Irqbalance >>> daemon is active. >>> >>> Hackbench (config-scheduler-unbound) >>> ========= >>> process-pipes: >>> Baseline Patched >>> Amean 1 0.4300 ( 0.00%) 0.4420 ( -2.79%) >>> Amean 4 0.8757 ( 0.00%) 0.8774 ( -0.20%) >>> Amean 7 1.3712 ( 0.00%) 1.3789 ( -0.56%) >>> Amean 12 2.3541 ( 0.00%) 2.3714 ( -0.73%) >>> Amean 21 4.2229 ( 0.00%) 4.2439 ( -0.50%) >>> Amean 30 5.9113 ( 0.00%) 5.9451 ( -0.57%) >>> Amean 48 9.3873 ( 0.00%) 9.4898 ( -1.09%) >>> Amean 79 15.9281 ( 0.00%) 16.1385 ( -1.32%) >>> Amean 110 22.0961 ( 0.00%) 22.3433 ( -1.12%) >>> Amean 141 28.2973 ( 0.00%) 28.6209 ( -1.14%) >>> Amean 172 34.4709 ( 0.00%) 34.9347 ( -1.35%) >>> Amean 203 40.7621 ( 0.00%) 41.2497 ( -1.20%) >>> Amean 234 47.0416 ( 0.00%) 47.6470 ( -1.29%) >>> Amean 265 53.3048 ( 0.00%) 54.1625 ( -1.61%) >>> Amean 288 58.0595 ( 0.00%) 58.8096 ( -1.29%) >>> >>> process-sockets: >>> Baseline Patched >>> Amean 1 0.6776 ( 0.00%) 0.6611 ( 2.43%) >>> Amean 4 2.6183 ( 0.00%) 2.5769 ( 1.58%) >>> Amean 7 4.5662 ( 0.00%) 4.4801 ( 1.89%) >>> Amean 12 7.7638 ( 0.00%) 7.6201 ( 1.85%) >>> Amean 21 13.5335 ( 0.00%) 13.2915 ( 1.79%) >>> Amean 30 19.3369 ( 0.00%) 18.9811 ( 1.84%) >>> Amean 48 31.0724 ( 0.00%) 30.6015 ( 1.52%) >>> Amean 79 51.1881 ( 0.00%) 50.4251 ( 1.49%) >>> Amean 110 71.3399 ( 0.00%) 70.4578 ( 1.24%) >>> Amean 141 91.4675 ( 0.00%) 90.3769 ( 1.19%) >>> Amean 172 111.7463 ( 0.00%) 110.3947 ( 1.21%) >>> Amean 203 131.6927 ( 0.00%) 130.3270 ( 1.04%) >>> Amean 234 151.7459 ( 0.00%) 150.1320 ( 1.06%) >>> Amean 265 171.9101 ( 0.00%) 169.9751 ( 1.13%) >>> Amean 288 186.9231 ( 0.00%) 184.7706 ( 1.15%) >>> >>> thread-pipes: >>> Baseline Patched >>> Amean 1 0.4523 ( 0.00%) 0.4535 ( -0.28%) >>> Amean 4 0.9041 ( 0.00%) 0.9085 ( -0.48%) >>> Amean 7 1.4111 ( 0.00%) 1.4146 ( -0.25%) >>> Amean 12 2.3532 ( 0.00%) 2.3688 ( -0.66%) >>> Amean 21 4.1550 ( 0.00%) 4.1701 ( -0.36%) >>> Amean 30 6.1043 ( 0.00%) 6.2391 ( -2.21%) >>> Amean 48 10.2077 ( 0.00%) 10.3511 ( -1.40%) >>> Amean 79 16.7922 ( 0.00%) 17.0427 ( -1.49%) >>> Amean 110 23.3350 ( 0.00%) 23.6522 ( -1.36%) >>> Amean 141 29.6458 ( 0.00%) 30.2617 ( -2.08%) >>> Amean 172 35.8649 ( 0.00%) 36.4225 ( -1.55%) >>> Amean 203 42.4477 ( 0.00%) 42.8332 ( -0.91%) >>> Amean 234 48.7117 ( 0.00%) 49.4042 ( -1.42%) >>> Amean 265 54.9171 ( 0.00%) 55.6551 ( -1.34%) >>> Amean 288 59.5282 ( 0.00%) 60.2903 ( -1.28%) >>> >>> thread-sockets: >>> Baseline Patched >>> Amean 1 0.6917 ( 0.00%) 0.6892 ( 0.37%) >>> Amean 4 2.6651 ( 0.00%) 2.6017 ( 2.38%) >>> Amean 7 4.6734 ( 0.00%) 4.5637 ( 2.35%) >>> Amean 12 8.0156 ( 0.00%) 7.8079 ( 2.59%) >>> Amean 21 14.0451 ( 0.00%) 13.6679 ( 2.69%) >>> Amean 30 20.0963 ( 0.00%) 19.5657 ( 2.64%) >>> Amean 48 32.4115 ( 0.00%) 31.6001 ( 2.50%) >>> Amean 79 53.1104 ( 0.00%) 51.8395 ( 2.39%) >>> Amean 110 74.0929 ( 0.00%) 72.4391 ( 2.23%) >>> Amean 141 95.1506 ( 0.00%) 93.0992 ( 2.16%) >>> Amean 172 116.1969 ( 0.00%) 113.8307 ( 2.04%) >>> Amean 203 137.4413 ( 0.00%) 134.5247 ( 2.12%) >>> Amean 234 158.5395 ( 0.00%) 155.2793 ( 2.06%) >>> Amean 265 179.7729 ( 0.00%) 176.1099 ( 2.04%) >>> Amean 288 195.5573 ( 0.00%) 191.3977 ( 2.13%) >>> >>> Pgbench (config-db-pgbench-timed-ro-small) >>> ======= >>> Baseline Patched >>> Hmean 1 68.54 ( 0.00%) 69.72 ( 1.71%) >>> Hmean 6 27725.78 ( 0.00%) 34119.11 ( 23.06%) >>> Hmean 12 55724.26 ( 0.00%) 63158.22 ( 13.34%) >>> Hmean 22 72806.26 ( 0.00%) 73389.98 ( 0.80%) >>> Hmean 30 79000.45 ( 0.00%) 75197.02 ( -4.81%) >>> Hmean 48 78180.16 ( 0.00%) 75074.09 ( -3.97%) >>> Hmean 80 75001.93 ( 0.00%) 70590.72 ( -5.88%) >>> Hmean 110 74812.25 ( 0.00%) 74128.57 ( -0.91%) >>> Hmean 142 74261.05 ( 0.00%) 72910.48 ( -1.82%) >>> Hmean 144 75375.35 ( 0.00%) 71295.72 ( -5.41%) >>> >>> For hackbench, +-3% fluctuation is normal on this two-socket box, it's safe to >>> conclude that there are no performance differences for wakeups from task context. >>> For pgbench, after many runs, 10~30% gains are very consistent at lower >>> client counts (< #cores per socket). >> Can you provide some further insights on why pgebench is helped at low load >> case? Is it because the woken tasks tend to stay put and not get moved around with interrupts >> and maintain cache warmth? > Yes, and for read-only database workloads, the cache (whether it's > incoming packet or not) on the interrupt > CPU isn't as performance critical as cache from its previous CPU where > the db task run to process data. > To give you an example, consider a db client/server case, a client > sends a request for a select query > through the network, the server accepts the query request and does all > the heavy lifting and sends the result > back. For the server, the incoming packet is just a line of query > whereas the CPU and its L3 db process previously > on has all the warm db caches, pulling it away from them is a crime > :) This may seem to be a little contradictory > to what I said earlier about the idle case and Mel's patch, but > ¯\_(ツ)_/¯ it's hard to make all the workloads out > there happy. Anyway like I said earlier, this patch doesn't affect the > idle case > > At higher load, sync in wake_affine_idle() doesn't really matter > because the waking CPU could easily have more than > 1 runnable tasks. Sync in wake_affine_weight() also doesn't matter > much as both sides have work to do, and cache > benefit of not pulling decreases simply because there are a lot more > db processes under the same L3, they can compete > for the same cachelines. > > Hope my explanation helps! > > Libo >> Tim >> >>> For higher client counts, both kernels are >>> very close, +-5% swings are quite common. Also NET_TX|RX softirq load >>> does spread out across both NUMA nodes in this test so there is no need to do >>> any explicit RPS/RFS. >>> >>> [1]:https://urldefense.com/v3/__https://lkml.org/lkml/2021/11/5/234__;!!ACWV5N9M2RV99hQ!PTDVWQJ1pb2KldrRnXA3EJ2CPKyHAolke1QRjVLadEl2ctd3xYxTo5uZ5Dp91L3ExImjbrrqbNM27uEA-E9do7LM$ >>> >>> Signed-off-by: Libo Chen<libo.chen@oracle.com> >>> --- >>> kernel/sched/fair.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >>> index 794c2cb945f8..59b210d2cdb5 100644 >>> --- a/kernel/sched/fair.c >>> +++ b/kernel/sched/fair.c >>> @@ -6704,7 +6704,9 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu) >>> static int >>> select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags) >>> { >>> - int sync = (wake_flags & WF_SYNC) && !(current->flags & PF_EXITING); >>> + /* Don't set sync for wakeup from irq/soft ctx */ >>> + int sync = in_task() && (wake_flags & WF_SYNC) >>> + && !(current->flags & PF_EXITING); >>> struct sched_domain *tmp, *sd = NULL; >>> int cpu = smp_processor_id(); >>> int new_cpu = prev_cpu; >>> -- >>> 2.31.1 >>> >
| |