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

Zhang, Cathy
2023-05-12T02:38:58Z

> -----Original Message-----
> From: Shakeel Butt <shakeelb@google.com>
> Sent: Friday, May 12, 2023 5:19 AM
> To: Zhang@google.com; Zhang, Cathy <cathy.zhang@intel.com>
> Cc: Eric Dumazet <edumazet@google.com>; Linux MM <linux-
> mm@kvack.org>; Cgroups <cgroups@vger.kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; davem@davemloft.net; kuba@kernel.org;
> Brandeburg@google.com; Brandeburg, Jesse <jesse.brandeburg@intel.com>;
> Srinivas@google.com; Srinivas, Suresh <suresh.srinivas@intel.com>;
> Chen@google.com; Chen, Tim C <tim.c.chen@intel.com>; You@google.com;
> You, Lizhen <lizhen.you@intel.com>; eric.dumazet@gmail.com;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next 1/2] net: Keep sk->sk_forward_alloc as a proper
> size
>
> On Thu, May 11, 2023 at 09:26:46AM +0000, Zhang, Cathy wrote:
> >
> [...]
> >
> > 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
>
> I do want to understand for above which specific condition in __refill_stock is
> causing to drain stock in the charge code path. Can you please re-run and
> profile your test with following code snippet (or use any other mechanism
> which can answer the question)?
>
> From f1d91043f21f4b29717c78615b374d79fc021d1f Mon Sep 17 00:00:00
> 2001
> From: Shakeel Butt <shakeelb@google.com>
> Date: Thu, 11 May 2023 20:00:19 +0000
> Subject: [PATCH] Debug drain on charging.
>
> ---
> mm/memcontrol.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c index
> d31fb1e2cb33..4c1c3d90a4a3 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2311,6 +2311,16 @@ static void drain_local_stock(struct work_struct
> *dummy)
> obj_cgroup_put(old);
> }
>
> +static noinline void drain_stock_1(struct memcg_stock_pcp *stock) {
> + drain_stock(stock);
> +}
> +
> +static noinline void drain_stock_2(struct memcg_stock_pcp *stock) {
> + drain_stock(stock);
> +}
> +
> /*
> * Cache charges(val) to local per_cpu area.
> * This will be consumed by consume_stock() function, later.
> @@ -2321,14 +2331,14 @@ static void __refill_stock(struct mem_cgroup
> *memcg, unsigned int nr_pages)
>
> stock = this_cpu_ptr(&memcg_stock);
> if (READ_ONCE(stock->cached) != memcg) { /* reset if necessary */
> - drain_stock(stock);
> + drain_stock_1(stock);
> css_get(&memcg->css);
> WRITE_ONCE(stock->cached, memcg);
> }
> stock->nr_pages += nr_pages;
>
> if (stock->nr_pages > MEMCG_CHARGE_BATCH)
> - drain_stock(stock);
> + drain_stock_2(stock);
> }
>
> static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
> --
> 2.40.1.606.ga4b1b128d6-goog

Hi Shakeel,

Run with the temp change you provided, the output shows it comes to drain_stock_1(),
Here is the call trace:

8.96% mc-worker [kernel.vmlinux] [k] page_counter_cancel
|
--8.95%--page_counter_cancel
|
--8.95%--page_counter_uncharge
drain_stock_1
__refill_stock
refill_stock
|
--8.88%--try_charge_memcg
mem_cgroup_charge_skmem
|
--8.87%--__sk_mem_raise_allocated
__sk_mem_schedule
|
|--5.37%--tcp_try_rmem_schedule
| tcp_data_queue
| tcp_rcv_established
| tcp_v4_do_rcv