Re: [PATCH net-next 1/2] net: Keep sk->sk_forward_alloc as a proper size

[Shakeel Butt <shakeelb@google.com>]

+Andrew

On Wed, May 17, 2023 at 9:33 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, May 17, 2023 at 6:24 PM Shakeel Butt <shakeelb@google.com> 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@google.com>
> > 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.