From: "Huang\, Ying" <ying.huang@intel.com> To: Dave Hansen <dave.hansen@linux.intel.com> Cc: Andrew Morton <akpm@linux-foundation.org>, <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>, "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>, Andrea Arcangeli <aarcange@redhat.com>, Michal Hocko <mhocko@suse.com>, Johannes Weiner <hannes@cmpxchg.org>, "Shaohua Li" <shli@kernel.org>, Hugh Dickins <hughd@google.com>, Minchan Kim <minchan@kernel.org>, Rik van Riel <riel@redhat.com>, Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>, Zi Yan <zi.yan@cs.rutgers.edu>, Daniel Jordan <daniel.m.jordan@oracle.com> Subject: Re: [PATCH -mm -v4 04/21] mm, THP, swap: Support PMD swap mapping in swapcache_free_cluster() Date: Tue, 10 Jul 2018 14:53:35 +0800 [thread overview] Message-ID: <874lh7intc.fsf@yhuang-dev.intel.com> (raw) In-Reply-To: <dd7b3dd7-9e10-4b9f-b931-915298bfd627@linux.intel.com> (Dave Hansen's message of "Mon, 9 Jul 2018 10:11:57 -0700") Dave Hansen <dave.hansen@linux.intel.com> writes: >> +#ifdef CONFIG_THP_SWAP >> +static inline int cluster_swapcount(struct swap_cluster_info *ci) >> +{ >> + if (!ci || !cluster_is_huge(ci)) >> + return 0; >> + >> + return cluster_count(ci) - SWAPFILE_CLUSTER; >> +} >> +#else >> +#define cluster_swapcount(ci) 0 >> +#endif > > Dumb questions, round 2: On a CONFIG_THP_SWAP=n build, presumably, > cluster_is_huge()=0 always, so cluster_swapout() always returns 0. Right? > > So, why the #ifdef? #ifdef here is to reduce the code size for !CONFIG_THP_SWAP. >> /* >> * It's possible scan_swap_map() uses a free cluster in the middle of free >> * cluster list. Avoiding such abuse to avoid list corruption. >> @@ -905,6 +917,7 @@ static void swap_free_cluster(struct swap_info_struct *si, unsigned long idx) >> struct swap_cluster_info *ci; >> >> ci = lock_cluster(si, offset); >> + memset(si->swap_map + offset, 0, SWAPFILE_CLUSTER); >> cluster_set_count_flag(ci, 0, 0); >> free_cluster(si, idx); >> unlock_cluster(ci); > > This is another case of gloriously comment-free code, but stuff that > _was_ covered in the changelog. I'd much rather have code comments than > changelog comments. Could we fix that? > > I'm generally finding it quite hard to review this because I keep having > to refer back to the changelog to see if what you are doing matches what > you said you were doing. Sure. Will fix this. >> @@ -1288,24 +1301,30 @@ static void swapcache_free_cluster(swp_entry_t entry) >> >> ci = lock_cluster(si, offset); >> VM_BUG_ON(!cluster_is_huge(ci)); >> + VM_BUG_ON(!is_cluster_offset(offset)); >> + VM_BUG_ON(cluster_count(ci) < SWAPFILE_CLUSTER); >> map = si->swap_map + offset; >> - for (i = 0; i < SWAPFILE_CLUSTER; i++) { >> - val = map[i]; >> - VM_BUG_ON(!(val & SWAP_HAS_CACHE)); >> - if (val == SWAP_HAS_CACHE) >> - free_entries++; >> + if (!cluster_swapcount(ci)) { >> + for (i = 0; i < SWAPFILE_CLUSTER; i++) { >> + val = map[i]; >> + VM_BUG_ON(!(val & SWAP_HAS_CACHE)); >> + if (val == SWAP_HAS_CACHE) >> + free_entries++; >> + } >> + if (free_entries != SWAPFILE_CLUSTER) >> + cluster_clear_huge(ci); >> } > > Also, I'll point out that cluster_swapcount() continues the horrific > naming of cluster_couunt(), not saying what the count is *of*. The > return value doesn't help much: > > return cluster_count(ci) - SWAPFILE_CLUSTER; We have page_swapcount() for page, swp_swapcount() for swap entry. cluster_swapcount() tries to mimic them for swap cluster. But I am not good at naming in general. What's your suggestion? Best Regards, Huang, Ying
WARNING: multiple messages have this Message-ID (diff)
From: "Huang\, Ying" <ying.huang@intel.com> To: Dave Hansen <dave.hansen@linux.intel.com> Cc: Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, linux-kernel@vger.kernel.org, "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>, Andrea Arcangeli <aarcange@redhat.com>, Michal Hocko <mhocko@suse.com>, Johannes Weiner <hannes@cmpxchg.org>, Shaohua Li <shli@kernel.org>, Hugh Dickins <hughd@google.com>, Minchan Kim <minchan@kernel.org>, Rik van Riel <riel@redhat.com>, Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>, Zi Yan <zi.yan@cs.rutgers.edu>, Daniel Jordan <daniel.m.jordan@oracle.com> Subject: Re: [PATCH -mm -v4 04/21] mm, THP, swap: Support PMD swap mapping in swapcache_free_cluster() Date: Tue, 10 Jul 2018 14:53:35 +0800 [thread overview] Message-ID: <874lh7intc.fsf@yhuang-dev.intel.com> (raw) In-Reply-To: <dd7b3dd7-9e10-4b9f-b931-915298bfd627@linux.intel.com> (Dave Hansen's message of "Mon, 9 Jul 2018 10:11:57 -0700") Dave Hansen <dave.hansen@linux.intel.com> writes: >> +#ifdef CONFIG_THP_SWAP >> +static inline int cluster_swapcount(struct swap_cluster_info *ci) >> +{ >> + if (!ci || !cluster_is_huge(ci)) >> + return 0; >> + >> + return cluster_count(ci) - SWAPFILE_CLUSTER; >> +} >> +#else >> +#define cluster_swapcount(ci) 0 >> +#endif > > Dumb questions, round 2: On a CONFIG_THP_SWAP=n build, presumably, > cluster_is_huge()=0 always, so cluster_swapout() always returns 0. Right? > > So, why the #ifdef? #ifdef here is to reduce the code size for !CONFIG_THP_SWAP. >> /* >> * It's possible scan_swap_map() uses a free cluster in the middle of free >> * cluster list. Avoiding such abuse to avoid list corruption. >> @@ -905,6 +917,7 @@ static void swap_free_cluster(struct swap_info_struct *si, unsigned long idx) >> struct swap_cluster_info *ci; >> >> ci = lock_cluster(si, offset); >> + memset(si->swap_map + offset, 0, SWAPFILE_CLUSTER); >> cluster_set_count_flag(ci, 0, 0); >> free_cluster(si, idx); >> unlock_cluster(ci); > > This is another case of gloriously comment-free code, but stuff that > _was_ covered in the changelog. I'd much rather have code comments than > changelog comments. Could we fix that? > > I'm generally finding it quite hard to review this because I keep having > to refer back to the changelog to see if what you are doing matches what > you said you were doing. Sure. Will fix this. >> @@ -1288,24 +1301,30 @@ static void swapcache_free_cluster(swp_entry_t entry) >> >> ci = lock_cluster(si, offset); >> VM_BUG_ON(!cluster_is_huge(ci)); >> + VM_BUG_ON(!is_cluster_offset(offset)); >> + VM_BUG_ON(cluster_count(ci) < SWAPFILE_CLUSTER); >> map = si->swap_map + offset; >> - for (i = 0; i < SWAPFILE_CLUSTER; i++) { >> - val = map[i]; >> - VM_BUG_ON(!(val & SWAP_HAS_CACHE)); >> - if (val == SWAP_HAS_CACHE) >> - free_entries++; >> + if (!cluster_swapcount(ci)) { >> + for (i = 0; i < SWAPFILE_CLUSTER; i++) { >> + val = map[i]; >> + VM_BUG_ON(!(val & SWAP_HAS_CACHE)); >> + if (val == SWAP_HAS_CACHE) >> + free_entries++; >> + } >> + if (free_entries != SWAPFILE_CLUSTER) >> + cluster_clear_huge(ci); >> } > > Also, I'll point out that cluster_swapcount() continues the horrific > naming of cluster_couunt(), not saying what the count is *of*. The > return value doesn't help much: > > return cluster_count(ci) - SWAPFILE_CLUSTER; We have page_swapcount() for page, swp_swapcount() for swap entry. cluster_swapcount() tries to mimic them for swap cluster. But I am not good at naming in general. What's your suggestion? Best Regards, Huang, Ying
next prev parent reply other threads:[~2018-07-10 6:53 UTC|newest] Thread overview: 100+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-06-22 3:51 [PATCH -mm -v4 00/21] mm, THP, swap: Swapout/swapin THP in one piece Huang, Ying 2018-06-22 3:51 ` Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 01/21] mm, THP, swap: Enable PMD swap operations for CONFIG_THP_SWAP Huang, Ying 2018-07-07 21:11 ` Dan Williams 2018-07-09 5:40 ` Huang, Ying 2018-07-09 5:40 ` Huang, Ying 2018-07-09 6:08 ` Dan Williams 2018-07-09 6:34 ` Huang, Ying 2018-07-09 6:34 ` Huang, Ying 2018-07-09 15:59 ` Dave Hansen 2018-07-10 1:08 ` Huang, Ying 2018-07-10 1:08 ` Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 02/21] mm, THP, swap: Make CONFIG_THP_SWAP depends on CONFIG_SWAP Huang, Ying 2018-07-07 21:12 ` Dan Williams 2018-07-09 6:34 ` Huang, Ying 2018-07-09 6:34 ` Huang, Ying 2018-07-09 16:00 ` Dave Hansen 2018-07-10 1:19 ` Huang, Ying 2018-07-10 1:19 ` Huang, Ying 2018-07-10 1:59 ` Dave Hansen 2018-07-10 5:26 ` Huang, Ying 2018-07-10 5:26 ` Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 03/21] mm, THP, swap: Support PMD swap mapping in swap_duplicate() Huang, Ying 2018-06-29 6:04 ` Matthew Wilcox 2018-07-02 5:19 ` Huang, Ying 2018-07-02 5:19 ` Huang, Ying 2018-07-07 23:22 ` Dan Williams 2018-07-09 7:38 ` Huang, Ying 2018-07-09 7:38 ` Huang, Ying 2018-07-09 16:51 ` Dave Hansen 2018-07-10 6:44 ` Huang, Ying 2018-07-10 6:44 ` Huang, Ying 2018-07-10 13:50 ` Dave Hansen 2018-07-11 0:59 ` Huang, Ying 2018-07-11 0:59 ` Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 04/21] mm, THP, swap: Support PMD swap mapping in swapcache_free_cluster() Huang, Ying 2018-07-09 17:11 ` Dave Hansen 2018-07-10 6:53 ` Huang, Ying [this message] 2018-07-10 6:53 ` Huang, Ying 2018-07-10 13:54 ` Dave Hansen 2018-07-11 1:08 ` Huang, Ying 2018-07-11 1:08 ` Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 05/21] mm, THP, swap: Support PMD swap mapping in free_swap_and_cache()/swap_free() Huang, Ying 2018-07-05 18:33 ` Daniel Jordan 2018-07-06 12:49 ` Huang, Ying 2018-07-06 12:49 ` Huang, Ying 2018-07-09 17:19 ` Dave Hansen 2018-07-10 7:13 ` Huang, Ying 2018-07-10 7:13 ` Huang, Ying 2018-07-10 14:07 ` Dave Hansen 2018-07-11 1:28 ` Huang, Ying 2018-07-11 1:28 ` Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 06/21] mm, THP, swap: Support PMD swap mapping when splitting huge PMD Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 07/21] mm, THP, swap: Support PMD swap mapping in split_swap_cluster() Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 08/21] mm, THP, swap: Support to read a huge swap cluster for swapin a THP Huang, Ying 2018-06-29 6:21 ` Matthew Wilcox 2018-07-02 6:02 ` Huang, Ying 2018-07-02 6:02 ` Huang, Ying 2018-07-04 0:12 ` Daniel Jordan 2018-07-04 2:24 ` Huang, Ying 2018-07-04 2:24 ` Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 09/21] mm, THP, swap: Swapin a THP as a whole Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 10/21] mm, THP, swap: Support to count THP swapin and its fallback Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 11/21] mm, THP, swap: Add sysfs interface to configure THP swapin Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 12/21] mm, THP, swap: Support PMD swap mapping in swapoff Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 13/21] mm, THP, swap: Support PMD swap mapping in madvise_free() Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 14/21] mm, cgroup, THP, swap: Support to move swap account for PMD swap mapping Huang, Ying 2018-07-09 17:20 ` Daniel Jordan 2018-07-10 7:49 ` Huang, Ying 2018-07-10 7:49 ` Huang, Ying 2018-07-10 22:49 ` Daniel Jordan 2018-06-22 3:51 ` [PATCH -mm -v4 15/21] mm, THP, swap: Support to copy PMD swap mapping when fork() Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 16/21] mm, THP, swap: Free PMD swap mapping when zap_huge_pmd() Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 17/21] mm, THP, swap: Support PMD swap mapping for MADV_WILLNEED Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 18/21] mm, THP, swap: Support PMD swap mapping in mincore() Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 19/21] mm, THP, swap: Support PMD swap mapping in common path Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 20/21] mm, THP, swap: create PMD swap mapping when unmap the THP Huang, Ying 2018-06-22 3:51 ` [PATCH -mm -v4 21/21] mm, THP: Avoid to split THP when reclaim MADV_FREE THP Huang, Ying 2018-06-28 4:51 ` [PATCH -mm -v4 00/21] mm, THP, swap: Swapout/swapin THP in one piece Andrew Morton 2018-06-28 5:29 ` Huang, Ying 2018-06-28 5:29 ` Huang, Ying 2018-06-28 5:31 ` Andrew Morton 2018-06-28 5:31 ` Andrew Morton 2018-06-28 5:35 ` Huang, Ying 2018-06-28 5:35 ` Huang, Ying 2018-06-28 6:18 ` Andrew Morton 2018-06-28 6:18 ` Andrew Morton 2018-06-28 9:03 ` Matthew Wilcox 2018-06-28 9:03 ` Matthew Wilcox 2018-06-29 1:17 ` Huang, Ying 2018-06-29 1:17 ` Huang, Ying 2018-06-29 5:57 ` Matthew Wilcox 2018-07-02 5:19 ` Huang, Ying 2018-07-02 5:19 ` Huang, Ying 2018-07-04 2:11 ` Sergey Senozhatsky 2018-07-04 2:20 ` Huang, Ying 2018-07-04 2:20 ` Huang, Ying 2018-07-04 2:27 ` Sergey Senozhatsky 2018-07-04 2:59 ` Huang, Ying 2018-07-04 2:59 ` Huang, Ying
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=874lh7intc.fsf@yhuang-dev.intel.com \ --to=ying.huang@intel.com \ --cc=aarcange@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=daniel.m.jordan@oracle.com \ --cc=dave.hansen@linux.intel.com \ --cc=hannes@cmpxchg.org \ --cc=hughd@google.com \ --cc=kirill.shutemov@linux.intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@suse.com \ --cc=minchan@kernel.org \ --cc=n-horiguchi@ah.jp.nec.com \ --cc=riel@redhat.com \ --cc=shli@kernel.org \ --cc=zi.yan@cs.rutgers.edu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.