+Andrew On Wed, May 17, 2023 at 9:33 AM Eric Dumazet <edumazet@xxxxxxxxxx> wrote: > > On Wed, May 17, 2023 at 6:24 PM Shakeel Butt <shakeelb@xxxxxxxxxx> wrote: > > > > On Tue, May 16, 2023 at 01:46:55PM +0800, Oliver Sang wrote: > > > hi Shakeel, > > > > > > On Mon, May 15, 2023 at 12:50:31PM -0700, Shakeel Butt wrote: > > > > +Feng, Yin and Oliver > > > > > > > > > > > > > > > Thanks a lot Cathy for testing. Do you see any performance improvement for > > > > > > the memcached benchmark with the patch? > > > > > > > > > > Yep, absolutely :- ) RPS (with/without patch) = +1.74 > > > > > > > > Thanks a lot Cathy. > > > > > > > > Feng/Yin/Oliver, can you please test the patch at [1] with other > > > > workloads used by the test robot? Basically I wanted to know if it has > > > > any positive or negative impact on other perf benchmarks. > > > > > > is it possible for you to resend patch with Signed-off-by? > > > without it, test robot will regard the patch as informal, then it cannot feed > > > into auto test process. > > > and could you tell us the base of this patch? it will help us apply it > > > correctly. > > > > > > on the other hand, due to resource restraint, we normally cannot support > > > this type of on-demand test upon a single patch, patch set, or a branch. > > > instead, we try to merge them into so-called hourly-kernels, then distribute > > > tests and auto-bisects to various platforms. > > > after we applying your patch and merging it to hourly-kernels sccussfully, > > > if it really causes some performance changes, the test robot could spot out > > > this patch as 'fbc' and we will send report to you. this could happen within > > > several weeks after applying. > > > but due to the complexity of whole process (also limited resourse, such like > > > we cannot run all tests on all platforms), we cannot guanrantee capture all > > > possible performance impacts of this patch. and it's hard for us to provide > > > a big picture like what's the general performance impact of this patch. > > > this maybe is not exactly what you want. is it ok for you? > > > > > > > > > > Yes, that is fine and thanks for the help. The patch is below: > > > > > > From 93b3b4c5f356a5090551519522cfd5740ae7e774 Mon Sep 17 00:00:00 2001 > > From: Shakeel Butt <shakeelb@xxxxxxxxxx> > > Date: Tue, 16 May 2023 20:30:26 +0000 > > Subject: [PATCH] memcg: skip stock refill in irq context > > > > The linux kernel processes incoming packets in softirq on a given CPU > > and those packets may belong to different jobs. This is very normal on > > large systems running multiple workloads. With memcg enabled, network > > memory for such packets is charged to the corresponding memcgs of the > > jobs. > > > > Memcg charging can be a costly operation and the memcg code implements > > a per-cpu memcg charge caching optimization to reduce the cost of > > charging. More specifically, the kernel charges the given memcg for more > > memory than requested and keep the remaining charge in a local per-cpu > > cache. The insight behind this heuristic is that there will be more > > charge requests for that memcg in near future. This optimization works > > well when a specific job runs on a CPU for long time and majority of the > > charging requests happen in process context. However the kernel's > > incoming packet processing does not work well with this optimization. > > > > Recently Cathy Zhang has shown [1] that memcg charge flushing within the > > memcg charge path can become a performance bottleneck for the memcg > > charging of network traffic. > > > > Perf profile: > > > > 8.98% mc-worker [kernel.vmlinux] [k] page_counter_cancel > > | > > --8.97%--page_counter_cancel > > | > > --8.97%--page_counter_uncharge > > drain_stock > > __refill_stock > > refill_stock > > | > > --8.91%--try_charge_memcg > > mem_cgroup_charge_skmem > > | > > --8.91%--__sk_mem_raise_allocated > > __sk_mem_schedule > > | > > |--5.41%--tcp_try_rmem_schedule > > | tcp_data_queue > > | tcp_rcv_established > > | tcp_v4_do_rcv > > | tcp_v4_rcv > > > > The simplest way to solve this issue is to not refill the memcg charge > > stock in the irq context. Since networking is the main source of memcg > > charging in the irq context, other users will not be impacted. In > > addition, this will preseve the memcg charge cache of the application > > running on that CPU. > > > > There are also potential side effects. What if all the packets belong to > > the same application and memcg? More specifically, users can use Receive > > Flow Steering (RFS) to make sure the kernel process the packets of the > > application on the CPU where the application is running. This change may > > cause the kernel to do slowpath memcg charging more often in irq > > context. > > Could we have per-memcg per-cpu caches, instead of one set of per-cpu caches > needing to be drained evertime a cpu deals with 'another memcg' ? > The hierarchical nature of memcg makes that a bit complicated. We have something similar for memcg stats which is rstat infra where the stats are saved per-memcg per-cpu and get accumulated hierarchically every 2 seconds. This works fine for stats but for limits there would be a need for some additional restrictions. Also sometime ago Andrew asked me to explore replacing the atomic counter in page_counter with percpu_counter. Intuition is that most of the time the usage is not hitting the limit, so we can use __percpu_counter_compare for enforcement. Let me spend some time to explore per-memcg per-cpu cache or if percpu_counter would be better. For now, this patch is more like an RFC.
