From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 37732E7717F for ; Tue, 10 Dec 2024 15:46:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 79DF66B01FC; Tue, 10 Dec 2024 10:46:57 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 74E846B0200; Tue, 10 Dec 2024 10:46:57 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5EE9B6B0201; Tue, 10 Dec 2024 10:46:57 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 452BE6B01FC for ; Tue, 10 Dec 2024 10:46:57 -0500 (EST) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id C64D541F24 for ; Tue, 10 Dec 2024 15:46:56 +0000 (UTC) X-FDA: 82879476792.07.2CF0C7F Received: from mail-pf1-f172.google.com (mail-pf1-f172.google.com [209.85.210.172]) by imf03.hostedemail.com (Postfix) with ESMTP id D2C3F20008 for ; Tue, 10 Dec 2024 15:46:44 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=a8EJKeuA; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf03.hostedemail.com: domain of 42.hyeyoo@gmail.com designates 209.85.210.172 as permitted sender) smtp.mailfrom=42.hyeyoo@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1733845604; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=pQKRtbTwPveavtSE9vt+cx671iUeNPGsP1aIxfnitkw=; b=rGJMS3LP97Cwdl84CvWPIiuXbDBSRDNa8kXE7aS6tWToLhWY2s7VGU2NZmN4d+rcGulO08 sT4t2tzW/m+Th+aGt9estjbDYJ9cab28ktvJbMMAGE2Xggpki+FsyqAkK7UX0aQQ++jqIC YbhKYAj8docTRH3hGKdKePeS9IR6WSc= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1733845604; a=rsa-sha256; cv=none; b=6soWymC4juji7szW4NoVgGO8nKWcjUqyIltspDaBOgJUZiugQriuZHJ9ccBSQIjOg/dFMc 4rnvRTXO1WZZ9N+6Hnvs4o+S8zsJQ1wI93X0EopidiWq1V95U24PJczm6oFu7p7Ck+fWeB /n2aRGVpd8SS60kBnhtjMpEYipEqscs= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=a8EJKeuA; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf03.hostedemail.com: domain of 42.hyeyoo@gmail.com designates 209.85.210.172 as permitted sender) smtp.mailfrom=42.hyeyoo@gmail.com Received: by mail-pf1-f172.google.com with SMTP id d2e1a72fcca58-72764c995e5so928264b3a.2 for ; Tue, 10 Dec 2024 07:46:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1733845613; x=1734450413; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=pQKRtbTwPveavtSE9vt+cx671iUeNPGsP1aIxfnitkw=; b=a8EJKeuAHIm/pLx7MBkMXr1vZ9LMYEVKdMv82csT5CC/gmfF4c13yeRWyjY0ugPVm+ o0NhIjPKgSZO3d2Py3figf/lvO7c7pTZYJH3mAK8MWk04bg8uqz/PQyySqBNGBwnCnZP hDj+y61hbguisBT3WmKUPAq51BkqQjco7RuNER9Kj5TWeaRR1BjSnHEhA34UbvQNExKq J+fYB4a3sEbkigljxfC8MaQrJA0qeWADotIzVSV6eHwQdUC9puTh7qOr5ilKEB1aVlBK u6Ohse6IYMij/ctwDeicycZDM6ditDp4Az5mcZJ/YwMGsmhugxvkeb8PK8RyHkjppnb9 uZAA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733845613; x=1734450413; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=pQKRtbTwPveavtSE9vt+cx671iUeNPGsP1aIxfnitkw=; b=HBPzJPKMyskqQNwLMzDF1DShocxhpUSujcQ096kNormnPqvwdXPSbuXqebsVgRVmcP IPmRqXtXEkbaokyiFSxt0O3AwTRdBiTiPKBGGrBiaxo2rebU0AoHxlFyuF6zC5zO8zhi 46aHJPZAoVy/61NuFGZYieosIxPqk7JxM+WN5ewWf7TD57Sx9P4/h36ltjCiISJIXnAq InTx3rJJlGmBfDSOidVdnqEGQQv2tSgrPYR60HTCwXXtYDvyEppPcwoskPAAQtQG8JHL Wdpu+3NgYR+FoIl4BMP538G+wAhYqOMpDSAvIkCjJieD5B7nudzgcm1u9TkKM2SIx83u PUEA== X-Forwarded-Encrypted: i=1; AJvYcCUhxjGHoKWY2vYCra5w3yP6ydSJnI5vBWHw5282HB9DCQD85v71cQPnp9dc4S/LgBlkIivdIkd4Lw==@kvack.org X-Gm-Message-State: AOJu0YziVBaRH04EPkvhUU4y6omCnxqT6zIG1jcvIRvTGuEziOOUHEQz VncODs6HP81YX9by2JtWbeDtnKje6hTcA8YYwGtEMScYoQnI/AYQ X-Gm-Gg: ASbGncuEt8QZivdxB9rQ5LoSO7FMmmjE0q/meFF6/2fqyj5vLFdpikuH09xyzloHcOt r2qC/LnDXR7/1V0ladKZfBGB4h0WkbySGkJcpPy78ZUvb1c6QepDLUFoX3LhPz/iKWT3BeVy7Ct kr+j8HOZU5IPCgvQgngUhHhwHEqPU7tHhSblG5JRUQHqtZzUZcndOzwPuWeL4wFGzu2D1Qr/lQv 78RyRtdjkrvONSnzGy+W2B+05WCSwMHX64w6tguhiNEQZaokOsNJRPzdpoC9mLRauV7HBMN00yJ /2cGLMJJbw4= X-Google-Smtp-Source: AGHT+IEASMQEy0g471GgB5LOpJqBMtdqoKeB5Dk9g/XQ8UL7qaEaaHHc/OErdpTd0l8OQ5OLJ+KXKQ== X-Received: by 2002:a05:6a00:3e0e:b0:725:ef4b:de30 with SMTP id d2e1a72fcca58-725ef4be5e4mr9339969b3a.14.1733845613101; Tue, 10 Dec 2024 07:46:53 -0800 (PST) Received: from MacBook-Air-5.local ([2001:2d8:690f:73f:ecb8:cdc:8c50:63fd]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-725dad8ade2sm5642699b3a.49.2024.12.10.07.46.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 10 Dec 2024 07:46:52 -0800 (PST) Date: Wed, 11 Dec 2024 00:46:43 +0900 From: Hyeonggon Yoo <42.hyeyoo@gmail.com> To: "Matthew Wilcox (Oracle)" Cc: Minchan Kim , Sergey Senozhatsky , Alex Shi , linux-mm@kvack.org Subject: Re: [PATCH v8 21/21] mm/zsmalloc: update comments for page->zpdesc changes Message-ID: References: <20241205175000.3187069-1-willy@infradead.org> <20241205175000.3187069-22-willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20241205175000.3187069-22-willy@infradead.org> X-Stat-Signature: stro1o6yhp7hwtfxw3qgetm8win4yxfn X-Rspamd-Queue-Id: D2C3F20008 X-Rspam-User: X-Rspamd-Server: rspam01 X-HE-Tag: 1733845604-998142 X-HE-Meta: U2FsdGVkX1+XqXw4IsMkzz9NSutCiP7bNLHYwFxNyAGIWmlLrhDU4yvAcD7fkG50Ev5gP0wkoOGvzDZUhmiUxLc6f4i1pjhddxN7frQfkq0mu3lJxX8dv6NN6Fyw2ViHOofdl/FPVJ2qwZxW3/yoSeyYUWw5GSWgfSncVQB5tWIVHJWx9LmyJNvCn4/TB38ek+O3RG0FhM3BHo0Niv35Gl/tcMWRlv6lTdyNOIIBRRQGONg+cVa0fVTM/P9fNNvfgid+tOTlmFJO9SO2Uz52bfHrPaOmNOM8d9eyPh1HeZ/RKIg1K58YOgTjMzHuPHepgZD+E3SzJOBnAlk7YNUkD2EVJUezSIS+p3GOo/0td3X800hvi2fFswNZpeGr5miptIdIz/Q5s/1jf/XQrwV4tV/0qX+sWZFY3omNpKOdxOxgoDWiGzyznBFORyYF1Nf8jRYgvxERpVbwaNB1nHusOoP6IICZL1KiPmOhUPf9gvNj9WTurnuJzPoZ5oJVzblrYCFC9QU71tnIGKap8Y/flSLMc7Pih2FWNjhGugpWObJOahYdVMzgRTUcW6veLiJ/GM+mIlGKvEjr5E5RG2pWBxZ/fN7XS15CDm7hjrNJpFskVE6XYwgQUQcawogQEk9I2hOANhi4uOMOTycEUYnzVWSE0XYIWOsuU5JFpt/R1B49D9dmm2gH+Gs8UEDC9rm7Idpw/seUqQF1AoJkS4m+Xxy4fty/HE8pa6Xx06fv/wtYZispyulxmvXdntqTPwqr8OnVyEuVbIUsPvHFGc/ohDlr0p5HJdcUzfZ/1ykLQKU1nj3dHRk/0l6YaCLqdvJewVyZB9CifS28Y0zed8JM0dmCif/xjz0EZKxa47fRGBt3xjxuYSKBNXwGQH39/PvgG+TT9rYBlaWTfzp9gyQRW0Lah4QB0dtoeGS8yIrXESlP1UO/P8WAJTiJya2L/LonDqQwy7fODBGZ2SQXEsK HNmTesZ1 dMb+mmY2WzGJ4B2Ji5ZZ+1M1t4ZIJ7fybqYERwFoyMlEp5tt1kwCKjjDVdw+0F2ctmbWHCCGUFvGd4tC9mDt7oMirRPqiypXVMPE+l3+KzlzGHEdJWJ/vjQ/0qmmPujV9DONUU/t9ZrlxFybTJnKCpyGSI6azAIUX1s8d3VExXWmx0/SpwTFTIrz8GhCSFTV3PZyFgEKNMjtENWDi8OI4GIE1pF7oU3gSJ0SAVZw2SMb3B+512877cedgFX8XQtrptfX/DgHCbxU5vaoM7b9c7KDcIF4us+3Do8nlhljrHXVt9b9zcXkzbfwzUZbuKkfMOIWptskPT3XY1/OCZfwq47crF8VABV/fccbesjNaGMTShCZXNzVXGJoVM/fGFFsVtg5ef3HE2+pnbYz4Jd2OssEIeyuGTszmIX7+BIsJnEZ/RyG8PdClV96Xakiw8ekM1lsF7SjaFsmYNjVZxMDgqRjtKuYWYSaU3xmhQak1D8950ZCIXGZJx+nwoQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000125, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Thu, Dec 05, 2024 at 05:49:58PM +0000, Matthew Wilcox (Oracle) wrote: > From: Alex Shi > > After the page to zpdesc conversion, there still left few comments or > function named with page not zpdesc, let's update the comments and > rename function create_page_chain() as create_zpdesc_chain(). Talking about updating comments and code by replacing 'page' with 'zpdesc', I'm not sure if it makes sense to replace all instances of 'page' with 'zpdesc'. A zpdesc is a descriptor, not a page that contains data (at least that's what I have been thinking while writing the initial patch series). In that context I'm still not sure if saying "a sub-zpdesc of a zspage", "lock zpdesc", or "migrate zpdesc" makes sense because replacing 'page descriptor (aka. struct page)' with 'zpool descriptor (aka. zpdesc)' doesn't mean zsmalloc is throwing away the conecept of pages. (...Or I might be thinking about this the wrong way) > Signed-off-by: Alex Shi > --- > mm/zsmalloc.c | 61 ++++++++++++++++++++++++++------------------------- > 1 file changed, 31 insertions(+), 30 deletions(-) > > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c > index c0e7c055847a..1f5ff0fdeb42 100644 > --- a/mm/zsmalloc.c > +++ b/mm/zsmalloc.c > @@ -15,20 +15,19 @@ > > /* > * Following is how we use various fields and flags of underlying > - * struct page(s) to form a zspage. > + * struct zpdesc(page) to form a zspage. > * > - * Usage of struct page fields: > - * page->private: points to zspage > - * page->index: links together all component pages of a zspage > + * Usage of struct zpdesc fields: > + * zpdesc->zspage: points to zspage > + * zpdesc->next: links together all component zpdescs of a zspage > * For the huge page, this is always 0, so we use this field > * to store handle. > - * page->page_type: PGTY_zsmalloc, lower 24 bits locate the first object > - * offset in a subpage of a zspage > - * > - * Usage of struct page flags: > - * PG_private: identifies the first component page > - * PG_owner_priv_1: identifies the huge component page > + * zpdesc->first_obj_offset: PGTY_zsmalloc, lower 24 bits locate the first > + * object offset in a subpage of a zspage > * > + * Usage of struct zpdesc(page) flags: > + * PG_private: identifies the first component zpdesc > + * PG_lock: lock all component zpdescs for a zspage free, serialize with > */ I think this comment is unnecessary, as it's already documented in mm/zpdesc.h. It can be removed in patch 01. > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > @@ -194,7 +193,10 @@ struct size_class { > */ > int size; > int objs_per_zspage; > - /* Number of PAGE_SIZE sized pages to combine to form a 'zspage' */ > + /* > + * Number of PAGE_SIZE sized zpdescs/pages to combine to > + * form a 'zspage' > + */ > int pages_per_zspage; > > unsigned int index; > @@ -908,7 +910,7 @@ static void free_zspage(struct zs_pool *pool, struct size_class *class, > > /* > * Since zs_free couldn't be sleepable, this function cannot call > - * lock_page. The page locks trylock_zspage got will be released > + * lock_page. The zpdesc locks trylock_zspage got will be released > * by __free_zspage. > */ > if (!trylock_zspage(zspage)) { > @@ -965,7 +967,7 @@ static void init_zspage(struct size_class *class, struct zspage *zspage) > set_freeobj(zspage, 0); > } > > -static void create_page_chain(struct size_class *class, struct zspage *zspage, > +static void create_zpdesc_chain(struct size_class *class, struct zspage *zspage, > struct zpdesc *zpdescs[]) > { > int i; > @@ -974,9 +976,9 @@ static void create_page_chain(struct size_class *class, struct zspage *zspage, > int nr_zpdescs = class->pages_per_zspage; > > /* > - * Allocate individual pages and link them together as: > - * 1. all pages are linked together using zpdesc->next > - * 2. each sub-page point to zspage using zpdesc->zspage > + * Allocate individual zpdescs and link them together as: > + * 1. all zpdescs are linked together using zpdesc->next > + * 2. each sub-zpdesc point to zspage using zpdesc->zspage > * > * we set PG_private to identify the first zpdesc (i.e. no other zpdesc > * has this flag set). > @@ -1034,7 +1036,7 @@ static struct zspage *alloc_zspage(struct zs_pool *pool, > zpdescs[i] = zpdesc; > } > > - create_page_chain(class, zspage, zpdescs); > + create_zpdesc_chain(class, zspage, zpdescs); > init_zspage(class, zspage); > zspage->pool = pool; > zspage->class = class->index; > @@ -1351,7 +1353,7 @@ static unsigned long obj_malloc(struct zs_pool *pool, > /* record handle in the header of allocated chunk */ > link->handle = handle | OBJ_ALLOCATED_TAG; > else > - /* record handle to page->index */ > + /* record handle to zpdesc->handle */ > zspage->first_zpdesc->handle = handle | OBJ_ALLOCATED_TAG; the name of the field is already 'handle', so we don't need a comment to explain it. > kunmap_local(vaddr); > @@ -1441,7 +1443,6 @@ static void obj_free(int class_size, unsigned long obj) > unsigned int f_objidx; > void *vaddr; > > - > obj_to_location(obj, &f_zpdesc, &f_objidx); > f_offset = offset_in_page(class_size * f_objidx); > zspage = get_zspage(f_zpdesc); > @@ -1684,19 +1685,19 @@ static int putback_zspage(struct size_class *class, struct zspage *zspage) > #ifdef CONFIG_COMPACTION > /* > * To prevent zspage destroy during migration, zspage freeing should > - * hold locks of all pages in the zspage. > + * hold locks of all component zpdesc in the zspage. > */ > static void lock_zspage(struct zspage *zspage) > { > struct zpdesc *curr_zpdesc, *zpdesc; > > /* > - * Pages we haven't locked yet can be migrated off the list while we're > + * Zpdesc we haven't locked yet can be migrated off the list while we're > * trying to lock them, so we need to be careful and only attempt to > - * lock each page under migrate_read_lock(). Otherwise, the page we lock > - * may no longer belong to the zspage. This means that we may wait for > - * the wrong page to unlock, so we must take a reference to the page > - * prior to waiting for it to unlock outside migrate_read_lock(). > + * lock each zpdesc under migrate_read_lock(). Otherwise, the zpdesc we > + * lock may no longer belong to the zspage. This means that we may wait > + * for the wrong zpdesc to unlock, so we must take a reference to the > + * zpdesc prior to waiting for it to unlock outside migrate_read_lock(). > */ > while (1) { > migrate_read_lock(zspage); > @@ -1771,7 +1772,7 @@ static void replace_sub_page(struct size_class *class, struct zspage *zspage, > idx++; > } while ((zpdesc = get_next_zpdesc(zpdesc)) != NULL); > > - create_page_chain(class, zspage, zpdescs); > + create_zpdesc_chain(class, zspage, zpdescs); > first_obj_offset = get_first_obj_offset(oldzpdesc); > set_first_obj_offset(newzpdesc, first_obj_offset); > if (unlikely(ZsHugePage(zspage))) > @@ -1782,8 +1783,8 @@ static void replace_sub_page(struct size_class *class, struct zspage *zspage, > static bool zs_page_isolate(struct page *page, isolate_mode_t mode) > { > /* > - * Page is locked so zspage couldn't be destroyed. For detail, look at > - * lock_zspage in free_zspage. > + * Page/zpdesc is locked so zspage couldn't be destroyed. For detail, > + * look at lock_zspage in free_zspage. > */ > VM_BUG_ON_PAGE(PageIsolated(page), page); > > @@ -1810,7 +1811,7 @@ static int zs_page_migrate(struct page *newpage, struct page *page, > /* We're committed, tell the world that this is a Zsmalloc page. */ > __zpdesc_set_zsmalloc(newzpdesc); > > - /* The page is locked, so this pointer must remain valid */ > + /* The zpdesc/page is locked, so this pointer must remain valid */ > zspage = get_zspage(zpdesc); > pool = zspage->pool; > > @@ -1883,7 +1884,7 @@ static const struct movable_operations zsmalloc_mops = { > }; > > /* > - * Caller should hold page_lock of all pages in the zspage > + * Caller should hold zpdesc locks of all in the zspage > * In here, we cannot use zspage meta data. > */ > static void async_free_zspage(struct work_struct *work) > -- > 2.45.2 > >