Date:   Thu, 6 Jan 2022 16:05:30 -0800
From:   Roman Gushchin <guro@fb.com>
To:     Muchun Song <songmuchun@bytedance.com>
CC:     <willy@infradead.org>, <akpm@linux-foundation.org>,
        <hannes@cmpxchg.org>, <mhocko@kernel.org>,
        <vdavydov.dev@gmail.com>, <shakeelb@google.com>,
        <shy828301@gmail.com>, <alexs@kernel.org>,
        <richard.weiyang@gmail.com>, <david@fromorbit.com>,
        <trond.myklebust@hammerspace.com>, <anna.schumaker@netapp.com>,
        <jaegeuk@kernel.org>, <chao@kernel.org>,
        <kari.argillander@gmail.com>, <linux-fsdevel@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>,
        <linux-nfs@vger.kernel.org>, <zhengqi.arch@bytedance.com>,
        <duanxiongchun@bytedance.com>, <fam.zheng@bytedance.com>,
        <smuchun@gmail.com>
Subject: Re: [PATCH v5 01/16] mm: list_lru: optimize memory consumption of
 arrays of per cgroup lists
Message-ID: <YdeDym9IUghnagrK@carbon.dhcp.thefacebook.com>
References: <20211220085649.8196-1-songmuchun@bytedance.com>
 <20211220085649.8196-2-songmuchun@bytedance.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20211220085649.8196-2-songmuchun@bytedance.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?yE/3I50AXpE1qbdbkprCueyHNF1LqrL4/658c1OIlfJWAuXkP1cIWNRXUgv4?=
 =?us-ascii?Q?/8EqlSMV4bu5azh62qJWoh0dl46D5WsXAsjd1FisDtyCvtVyKX0jAOJycN0c?=
 =?us-ascii?Q?1qvDPPIu9oHwpyXBGmMXnMhAFNBssshPYnAu6H676al1C+elJZMD4gmX9+92?=
 =?us-ascii?Q?T7vIfaWub142fJv1uHpPdq0DIbKZFHDqUdy3l68VRb0CwDCLk+RgoTgCmsh7?=
 =?us-ascii?Q?i6aIxAVNQFZHSPpAMS9rPuBnYSxGVQzkQrzsbpuhQ1bkMhemFAtyRX+n3vp8?=
 =?us-ascii?Q?k+9TRBqxJKPsJfZV1nZjNyIgnam7cAm+6VTI5AqDQEAVTRcoHJJ/dfrLNK5c?=
 =?us-ascii?Q?WFCzOGycjZUTHVpSLAxX2tizeMCdSadk+bo0DUvhWc2JBp3rYs1gJmd1WjbA?=
 =?us-ascii?Q?oXJzqMJEqklu3tQ4dGaMy0aoJQABI0fH7xjgCWVDmcHe5JGJouTjN0pgMt2F?=
 =?us-ascii?Q?0J2QnpPeD9/FkJTxR9lDB0Z6mEjSHcEe6VMr74dGhLGz/OQpDfowi4CyREir?=
 =?us-ascii?Q?b8xv5asqL6MuG53IATYZOx770j75c7VmBn8yKXhPH0PRjTEusOVPbDCNXbf1?=
 =?us-ascii?Q?pzuUtM5bPL/1qweTxa1MmpgmuH8KkHT0bGheuKHHZdgy95qYzNGZx8yJPD3p?=
 =?us-ascii?Q?L/MDQ3PgJW43XbGf1lmCd+HPTFA0OI/lh+tT+jo8ZDvybi9YThpQplPHUwKc?=
 =?us-ascii?Q?7EQvJDJtIwPnZxIzfApQNd+DECKYTbgAbD9aBoRcuHBwEnjW7n47/SNWK6mr?=
 =?us-ascii?Q?+BRh0plmxJC+4T6Rk096Brro68KIV2CfavsHczNK5VtuZyOpaswMlWln2mna?=
 =?us-ascii?Q?aMCYsigPjgnNyPET4nInsdt2sw29BWLXH/0uL9Cxe9n7rIHaIKZp3LgEwBrw?=
 =?us-ascii?Q?8XN+jyYgtP/uJHxL4whGfQ4QGDzFdunXJTSpQaO6AYvBOqQItMnLimqP2SMZ?=
 =?us-ascii?Q?kfdOn66BQYG0rwyXeSpGCtexY6r0xbLALJ+ETy0/nk5GPdpTARpApnCIUWws?=
 =?us-ascii?Q?e+7p4Mxuw3b7KjHi3zSwL9HegLB5bteGxYKF8NZeuhkp+mNQmDAqGBvmID9n?=
 =?us-ascii?Q?r9HKgmEhKozsGNGBow/9wPDNx39avU4HFmPMfJgGUsIhoLHSdQopRu8Qis0a?=
 =?us-ascii?Q?wdynpM04VBDHS9mylEfaDFsMaqsha5aj1s26tRTrrCKO1PLezY0fuyHBmkTf?=
 =?us-ascii?Q?ghcU4CkTCGdOBhunmrKhLhKq/FQ39rw7/9/8NdI2rLBr44OeE9RV90Ep/sd2?=
 =?us-ascii?Q?jVxNmHytsHk1uLVhEfZI26s5LxH9Wo27iCqPAJwILW4epwH4kIShP+KzrqPN?=
 =?us-ascii?Q?Epjz0N7Bjum1AoFrw/ApBN9J+kT9kCaW3C3CaL16suQ1R0CllgUY9dHvQ3fq?=
 =?us-ascii?Q?dzBwv7QH+Ie/ZfQRLJaQumkzWAecNr2MGSy8z46G1gMcyy8h3gQtqEJfPAU7?=
 =?us-ascii?Q?AaLSO/WXNuqNDaQVgi2VPdUhhXy6tknOiqnOfL/pOJc8YlyQx2l0gLFPjbf0?=
 =?us-ascii?Q?4R2wZ0URUMmqEX0tKXXeVSKqh5Wu+FgNaiTDekK4fp87Ny/gx1Gb94ujNJ0y?=
 =?us-ascii?Q?fKOKxE/kYcbvL7V+yC7IuyBraLbyYdYD+T+SvIqq?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 379f3078-4249-4def-648d-08d9d1716dae
X-MS-Exchange-CrossTenant-AuthSource: BYAPR15MB4136.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jan 2022 00:05:35.6290
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: MVmZwJeDDcqFFQBtWQnq0msMi4MM7gm8ZkzEmIYXb+6M1T2aHq5QnpRsGwKBcZbl
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR15MB3521
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: kATk6L6Vgraw8S2e947YFprAVEKvkTyj
X-Proofpoint-ORIG-GUID: kATk6L6Vgraw8S2e947YFprAVEKvkTyj
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.11.62.513
 definitions=2022-01-06_10,2022-01-06_01,2021-12-02_01
X-Proofpoint-Spam-Details: rule=fb_outbound_notspam policy=fb_outbound score=0 mlxscore=0 phishscore=0
 spamscore=0 adultscore=0 suspectscore=0 lowpriorityscore=0 malwarescore=0
 impostorscore=0 priorityscore=1501 mlxlogscore=623 bulkscore=0
 clxscore=1011 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2110150000 definitions=main-2201060146
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Dec 20, 2021 at 04:56:34PM +0800, Muchun Song wrote:
> The list_lru uses an array (list_lru_memcg->lru) to store pointers
> which point to the list_lru_one. And the array is per memcg per node.
> Therefore, the size of the arrays will be 10K * number_of_node * 8 (
> a pointer size on 64 bits system) when we run 10k containers in the
> system. The memory consumption of the arrays becomes significant. The
> more numa node, the more memory it consumes.
> 
> I have done a simple test, which creates 10K memcg and mount point
> each in a two-node system. The memory consumption of the list_lru
> will be 24464MB. After converting the array from per memcg per node
> to per memcg, the memory consumption is going to be 21957MB. It is
> reduces by 2.5GB. In our AMD servers with 8 numa nodes in those
> sysuem, the memory consumption could be more significant. The savings
> come from the list_lru_one heads, that it also simplifies the
> alloc/dealloc path.
> 
> The new scheme looks like the following.
> 
>   +----------+   mlrus   +----------------+   mlru   +----------------------+
>   | list_lru +---------->| list_lru_memcg +--------->|  list_lru_per_memcg  |
>   +----------+           +----------------+          +----------------------+
>                                                      |  list_lru_per_memcg  |
>                                                      +----------------------+
>                                                      |          ...         |
>                           +--------------+   node    +----------------------+
>                           | list_lru_one |<----------+  list_lru_per_memcg  |
>                           +--------------+           +----------------------+
>                           | list_lru_one |
>                           +--------------+
>                           |      ...     |
>                           +--------------+
>                           | list_lru_one |
>                           +--------------+
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

As much as I like the code changes (there is indeed a significant simplification!),
I don't like the commit message and title, because I wasn't able to understand
what the patch is doing and some parts look simply questionable. Overall it
sounds like you reduce the number of list_lru_one structures, which is not true.

How about something like this?

--
mm: list_lru: transpose the array of per-node per-memcg lru lists

The current scheme of maintaining per-node per-memcg lru lists looks like:
  struct list_lru {
    struct list_lru_node *node;           (for each node)
      struct list_lru_memcg *memcg_lrus;
        struct list_lru_one *lru[];       (for each memcg)
  }

By effectively transposing the two-dimension array of list_lru_one's structures
(per-node per-memcg => per-memcg per-node) it's possible to save some memory
and simplify alloc/dealloc paths. The new scheme looks like:
  struct list_lru {
    struct list_lru_memcg *mlrus;
      struct list_lru_per_memcg *mlru[];  (for each memcg)
        struct list_lru_one node[0];      (for each node)
  }

Memory savings are coming from having fewer list_lru_memcg structures, which
contain an extra struct rcu_head to handle the destruction process.
--

But what worries me is that memory savings numbers you posted don't do up.
In theory we can save
16 (size of struct rcu_head) * 10000 (number of cgroups) * 2 (number of numa nodes) = 320k
per slab cache. Did you have a ton of mount points? Otherwise I don't understand
where these 2.5Gb are coming from.

Thanks!