Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
Dave Hansen writes: > On 07/17/2018 08:25 PM, Huang, Ying wrote: >>> Seriously, though, does it hurt us to add a comment or two to say >>> something like: >>> >>> /* >>> * Should not even be attempting cluster allocations when >>> * huge page swap is disabled. Warn and fail the allocation. >>> */ >>> if (!IS_ENABLED(CONFIG_THP_SWAP)) { >>> VM_WARN_ON_ONCE(1); >>> return 0; >>> } >> I totally agree with you that we should add more comments for THP swap >> to improve the code readability. As for this specific case, >> VM_WARN_ON_ONCE() here is just to capture some programming error during >> development. Do we really need comments here? > > If it's code in mainline, we need to know what it is doing. > > If it's not useful to have in mainline, then let's remove it. OK. Will add the comments. Best Regards, Huang, Ying
Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
Dave Hansen writes: > On 07/17/2018 08:25 PM, Huang, Ying wrote: >>> Seriously, though, does it hurt us to add a comment or two to say >>> something like: >>> >>> /* >>> * Should not even be attempting cluster allocations when >>> * huge page swap is disabled. Warn and fail the allocation. >>> */ >>> if (!IS_ENABLED(CONFIG_THP_SWAP)) { >>> VM_WARN_ON_ONCE(1); >>> return 0; >>> } >> I totally agree with you that we should add more comments for THP swap >> to improve the code readability. As for this specific case, >> VM_WARN_ON_ONCE() here is just to capture some programming error during >> development. Do we really need comments here? > > If it's code in mainline, we need to know what it is doing. > > If it's not useful to have in mainline, then let's remove it. OK. Will add the comments. Best Regards, Huang, Ying
Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
On 07/17/2018 08:25 PM, Huang, Ying wrote: >> Seriously, though, does it hurt us to add a comment or two to say >> something like: >> >> /* >> * Should not even be attempting cluster allocations when >> * huge page swap is disabled. Warn and fail the allocation. >> */ >> if (!IS_ENABLED(CONFIG_THP_SWAP)) { >> VM_WARN_ON_ONCE(1); >> return 0; >> } > I totally agree with you that we should add more comments for THP swap > to improve the code readability. As for this specific case, > VM_WARN_ON_ONCE() here is just to capture some programming error during > development. Do we really need comments here? If it's code in mainline, we need to know what it is doing. If it's not useful to have in mainline, then let's remove it.
Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
On 07/17/2018 08:25 PM, Huang, Ying wrote: >> Seriously, though, does it hurt us to add a comment or two to say >> something like: >> >> /* >> * Should not even be attempting cluster allocations when >> * huge page swap is disabled. Warn and fail the allocation. >> */ >> if (!IS_ENABLED(CONFIG_THP_SWAP)) { >> VM_WARN_ON_ONCE(1); >> return 0; >> } > I totally agree with you that we should add more comments for THP swap > to improve the code readability. As for this specific case, > VM_WARN_ON_ONCE() here is just to capture some programming error during > development. Do we really need comments here? If it's code in mainline, we need to know what it is doing. If it's not useful to have in mainline, then let's remove it.
Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
Dave Hansen writes: >> @@ -878,6 +877,11 @@ static int swap_alloc_cluster(struct swap_info_struct >> *si, swp_entry_t *slot) >> unsigned long offset, i; >> unsigned char *map; >> >> +if (!IS_ENABLED(CONFIG_THP_SWAP)) { >> +VM_WARN_ON_ONCE(1); >> +return 0; >> +} > > I see you seized the opportunity to keep this code gloriously > unencumbered by pesky comments. This seems like a time when you might > have slipped up and been temped to add a comment or two. Guess not. :) > > Seriously, though, does it hurt us to add a comment or two to say > something like: > > /* >* Should not even be attempting cluster allocations when >* huge page swap is disabled. Warn and fail the allocation. >*/ > if (!IS_ENABLED(CONFIG_THP_SWAP)) { > VM_WARN_ON_ONCE(1); > return 0; > } I totally agree with you that we should add more comments for THP swap to improve the code readability. As for this specific case, VM_WARN_ON_ONCE() here is just to capture some programming error during development. Do we really need comments here? I will try to add more comments for other places in code regardless this one. Best Regards, Huang, Ying
Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
Dave Hansen writes: >> @@ -878,6 +877,11 @@ static int swap_alloc_cluster(struct swap_info_struct >> *si, swp_entry_t *slot) >> unsigned long offset, i; >> unsigned char *map; >> >> +if (!IS_ENABLED(CONFIG_THP_SWAP)) { >> +VM_WARN_ON_ONCE(1); >> +return 0; >> +} > > I see you seized the opportunity to keep this code gloriously > unencumbered by pesky comments. This seems like a time when you might > have slipped up and been temped to add a comment or two. Guess not. :) > > Seriously, though, does it hurt us to add a comment or two to say > something like: > > /* >* Should not even be attempting cluster allocations when >* huge page swap is disabled. Warn and fail the allocation. >*/ > if (!IS_ENABLED(CONFIG_THP_SWAP)) { > VM_WARN_ON_ONCE(1); > return 0; > } I totally agree with you that we should add more comments for THP swap to improve the code readability. As for this specific case, VM_WARN_ON_ONCE() here is just to capture some programming error during development. Do we really need comments here? I will try to add more comments for other places in code regardless this one. Best Regards, Huang, Ying
Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
> @@ -878,6 +877,11 @@ static int swap_alloc_cluster(struct swap_info_struct > *si, swp_entry_t *slot) > unsigned long offset, i; > unsigned char *map; > > + if (!IS_ENABLED(CONFIG_THP_SWAP)) { > + VM_WARN_ON_ONCE(1); > + return 0; > + } I see you seized the opportunity to keep this code gloriously unencumbered by pesky comments. This seems like a time when you might have slipped up and been temped to add a comment or two. Guess not. :) Seriously, though, does it hurt us to add a comment or two to say something like: /* * Should not even be attempting cluster allocations when * huge page swap is disabled. Warn and fail the allocation. */ if (!IS_ENABLED(CONFIG_THP_SWAP)) { VM_WARN_ON_ONCE(1); return 0; }
Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
> @@ -878,6 +877,11 @@ static int swap_alloc_cluster(struct swap_info_struct > *si, swp_entry_t *slot) > unsigned long offset, i; > unsigned char *map; > > + if (!IS_ENABLED(CONFIG_THP_SWAP)) { > + VM_WARN_ON_ONCE(1); > + return 0; > + } I see you seized the opportunity to keep this code gloriously unencumbered by pesky comments. This seems like a time when you might have slipped up and been temped to add a comment or two. Guess not. :) Seriously, though, does it hurt us to add a comment or two to say something like: /* * Should not even be attempting cluster allocations when * huge page swap is disabled. Warn and fail the allocation. */ if (!IS_ENABLED(CONFIG_THP_SWAP)) { VM_WARN_ON_ONCE(1); return 0; }
[PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
In mm/swapfile.c, THP (Transparent Huge Page) swap specific code is enclosed by #ifdef CONFIG_THP_SWAP/#endif to avoid code dilating when THP isn't enabled. But #ifdef/#endif in .c file hurt the code readability, so Dave suggested to use IS_ENABLED(CONFIG_THP_SWAP) instead and let compiler to do the dirty job for us. This has potential to remove some duplicated code too. From output of `size`, text data bss dec hex filename THP=y: 26269 2076 340 28685700d mm/swapfile.o ifdef/endif: 24115 2028 340 264836773 mm/swapfile.o IS_ENABLED:24179 2028 340 2654767b3 mm/swapfile.o IS_ENABLED() based solution works quite well, almost as good as that of #ifdef/#endif. And from the diffstat, the removed lines are more than added lines. One #ifdef for split_swap_cluster() is kept. Because it is a public function with a stub implementation for CONFIG_THP_SWAP=n in swap.h. Signed-off-by: "Huang, Ying" Suggested-by: Dave Hansen Cc: Michal Hocko Cc: Johannes Weiner Cc: Shaohua Li Cc: Hugh Dickins Cc: Minchan Kim Cc: Rik van Riel Cc: Daniel Jordan Cc: Dan Williams --- mm/swapfile.c | 56 1 file changed, 16 insertions(+), 40 deletions(-) diff --git a/mm/swapfile.c b/mm/swapfile.c index 0a2a9643dd78..dd9263411f11 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -870,7 +870,6 @@ static int scan_swap_map_slots(struct swap_info_struct *si, return n_ret; } -#ifdef CONFIG_THP_SWAP static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot) { unsigned long idx; @@ -878,6 +877,11 @@ static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot) unsigned long offset, i; unsigned char *map; + if (!IS_ENABLED(CONFIG_THP_SWAP)) { + VM_WARN_ON_ONCE(1); + return 0; + } + if (cluster_list_empty(>free_clusters)) return 0; @@ -908,13 +912,6 @@ static void swap_free_cluster(struct swap_info_struct *si, unsigned long idx) unlock_cluster(ci); swap_range_free(si, offset, SWAPFILE_CLUSTER); } -#else -static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot) -{ - VM_WARN_ON_ONCE(1); - return 0; -} -#endif /* CONFIG_THP_SWAP */ static unsigned long scan_swap_map(struct swap_info_struct *si, unsigned char usage) @@ -1260,7 +1257,6 @@ static void swapcache_free(swp_entry_t entry) } } -#ifdef CONFIG_THP_SWAP static void swapcache_free_cluster(swp_entry_t entry) { unsigned long offset = swp_offset(entry); @@ -1271,6 +1267,9 @@ static void swapcache_free_cluster(swp_entry_t entry) unsigned int i, free_entries = 0; unsigned char val; + if (!IS_ENABLED(CONFIG_THP_SWAP)) + return; + si = _swap_info_get(entry); if (!si) return; @@ -1306,6 +1305,7 @@ static void swapcache_free_cluster(swp_entry_t entry) } } +#ifdef CONFIG_THP_SWAP int split_swap_cluster(swp_entry_t entry) { struct swap_info_struct *si; @@ -1320,11 +1320,7 @@ int split_swap_cluster(swp_entry_t entry) unlock_cluster(ci); return 0; } -#else -static inline void swapcache_free_cluster(swp_entry_t entry) -{ -} -#endif /* CONFIG_THP_SWAP */ +#endif void put_swap_page(struct page *page, swp_entry_t entry) { @@ -1483,7 +1479,6 @@ int swp_swapcount(swp_entry_t entry) return count; } -#ifdef CONFIG_THP_SWAP static bool swap_page_trans_huge_swapped(struct swap_info_struct *si, swp_entry_t entry) { @@ -1494,6 +1489,9 @@ static bool swap_page_trans_huge_swapped(struct swap_info_struct *si, int i; bool ret = false; + if (!IS_ENABLED(CONFIG_THP_SWAP)) + return swap_swapcount(si, entry) != 0; + ci = lock_cluster_or_swap_info(si, offset); if (!ci || !cluster_is_huge(ci)) { if (map[roffset] != SWAP_HAS_CACHE) @@ -1516,7 +1514,7 @@ static bool page_swapped(struct page *page) swp_entry_t entry; struct swap_info_struct *si; - if (likely(!PageTransCompound(page))) + if (!IS_ENABLED(CONFIG_THP_SWAP) || likely(!PageTransCompound(page))) return page_swapcount(page) != 0; page = compound_head(page); @@ -1540,10 +1538,8 @@ static int page_trans_huge_map_swapcount(struct page *page, int *total_mapcount, /* hugetlbfs shouldn't call it */ VM_BUG_ON_PAGE(PageHuge(page), page); - if (likely(!PageTransCompound(page))) { - mapcount = atomic_read(>_mapcount) + 1; - if (total_mapcount) - *total_mapcount = mapcount; + if (!IS_ENABLED(CONFIG_THP_SWAP) || likely(!PageTransCompound(page))) { + mapcount = page_trans_huge_mapcount(page,
[PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
In mm/swapfile.c, THP (Transparent Huge Page) swap specific code is enclosed by #ifdef CONFIG_THP_SWAP/#endif to avoid code dilating when THP isn't enabled. But #ifdef/#endif in .c file hurt the code readability, so Dave suggested to use IS_ENABLED(CONFIG_THP_SWAP) instead and let compiler to do the dirty job for us. This has potential to remove some duplicated code too. From output of `size`, text data bss dec hex filename THP=y: 26269 2076 340 28685700d mm/swapfile.o ifdef/endif: 24115 2028 340 264836773 mm/swapfile.o IS_ENABLED:24179 2028 340 2654767b3 mm/swapfile.o IS_ENABLED() based solution works quite well, almost as good as that of #ifdef/#endif. And from the diffstat, the removed lines are more than added lines. One #ifdef for split_swap_cluster() is kept. Because it is a public function with a stub implementation for CONFIG_THP_SWAP=n in swap.h. Signed-off-by: "Huang, Ying" Suggested-by: Dave Hansen Cc: Michal Hocko Cc: Johannes Weiner Cc: Shaohua Li Cc: Hugh Dickins Cc: Minchan Kim Cc: Rik van Riel Cc: Daniel Jordan Cc: Dan Williams --- mm/swapfile.c | 56 1 file changed, 16 insertions(+), 40 deletions(-) diff --git a/mm/swapfile.c b/mm/swapfile.c index 0a2a9643dd78..dd9263411f11 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -870,7 +870,6 @@ static int scan_swap_map_slots(struct swap_info_struct *si, return n_ret; } -#ifdef CONFIG_THP_SWAP static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot) { unsigned long idx; @@ -878,6 +877,11 @@ static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot) unsigned long offset, i; unsigned char *map; + if (!IS_ENABLED(CONFIG_THP_SWAP)) { + VM_WARN_ON_ONCE(1); + return 0; + } + if (cluster_list_empty(>free_clusters)) return 0; @@ -908,13 +912,6 @@ static void swap_free_cluster(struct swap_info_struct *si, unsigned long idx) unlock_cluster(ci); swap_range_free(si, offset, SWAPFILE_CLUSTER); } -#else -static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot) -{ - VM_WARN_ON_ONCE(1); - return 0; -} -#endif /* CONFIG_THP_SWAP */ static unsigned long scan_swap_map(struct swap_info_struct *si, unsigned char usage) @@ -1260,7 +1257,6 @@ static void swapcache_free(swp_entry_t entry) } } -#ifdef CONFIG_THP_SWAP static void swapcache_free_cluster(swp_entry_t entry) { unsigned long offset = swp_offset(entry); @@ -1271,6 +1267,9 @@ static void swapcache_free_cluster(swp_entry_t entry) unsigned int i, free_entries = 0; unsigned char val; + if (!IS_ENABLED(CONFIG_THP_SWAP)) + return; + si = _swap_info_get(entry); if (!si) return; @@ -1306,6 +1305,7 @@ static void swapcache_free_cluster(swp_entry_t entry) } } +#ifdef CONFIG_THP_SWAP int split_swap_cluster(swp_entry_t entry) { struct swap_info_struct *si; @@ -1320,11 +1320,7 @@ int split_swap_cluster(swp_entry_t entry) unlock_cluster(ci); return 0; } -#else -static inline void swapcache_free_cluster(swp_entry_t entry) -{ -} -#endif /* CONFIG_THP_SWAP */ +#endif void put_swap_page(struct page *page, swp_entry_t entry) { @@ -1483,7 +1479,6 @@ int swp_swapcount(swp_entry_t entry) return count; } -#ifdef CONFIG_THP_SWAP static bool swap_page_trans_huge_swapped(struct swap_info_struct *si, swp_entry_t entry) { @@ -1494,6 +1489,9 @@ static bool swap_page_trans_huge_swapped(struct swap_info_struct *si, int i; bool ret = false; + if (!IS_ENABLED(CONFIG_THP_SWAP)) + return swap_swapcount(si, entry) != 0; + ci = lock_cluster_or_swap_info(si, offset); if (!ci || !cluster_is_huge(ci)) { if (map[roffset] != SWAP_HAS_CACHE) @@ -1516,7 +1514,7 @@ static bool page_swapped(struct page *page) swp_entry_t entry; struct swap_info_struct *si; - if (likely(!PageTransCompound(page))) + if (!IS_ENABLED(CONFIG_THP_SWAP) || likely(!PageTransCompound(page))) return page_swapcount(page) != 0; page = compound_head(page); @@ -1540,10 +1538,8 @@ static int page_trans_huge_map_swapcount(struct page *page, int *total_mapcount, /* hugetlbfs shouldn't call it */ VM_BUG_ON_PAGE(PageHuge(page), page); - if (likely(!PageTransCompound(page))) { - mapcount = atomic_read(>_mapcount) + 1; - if (total_mapcount) - *total_mapcount = mapcount; + if (!IS_ENABLED(CONFIG_THP_SWAP) || likely(!PageTransCompound(page))) { + mapcount = page_trans_huge_mapcount(page,