| Message ID | 20211115134951.85286-3-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: Rework zap ptes on swap entries | expand |
On Mon, Nov 15, 2021 at 09:49:51PM +0800, Peter Xu wrote: > Clean the code up by merging the device private/exclusive swap entry handling > with the rest, then we merge the pte clear operation too. > > struct* page is defined in multiple places in the function, move it upward. Is that actually a good thing? There was a time when declaring variables more locally helped compilers with liveness analysis and register allocation. Compilers are probably smarter now.
On Mon, Nov 15, 2021 at 01:57:32PM +0000, Matthew Wilcox wrote: > On Mon, Nov 15, 2021 at 09:49:51PM +0800, Peter Xu wrote: > > Clean the code up by merging the device private/exclusive swap entry handling > > with the rest, then we merge the pte clear operation too. > > > > struct* page is defined in multiple places in the function, move it upward. > > Is that actually a good thing? There was a time when declaring > variables more locally helped compilers with liveness analysis and > register allocation. Compilers are probably smarter now. > I see, I don't know the history of that, but I did give it a shot with a patch that recovered all the "struct page*" back to the origin, I found that it'll generated exactly the same assembly of unmap_page_range (actually, the whole mm/memory.o) no matter what. I only tested on an aarch64 system, with below gcc version: $ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/libexec/gcc/aarch64-redhat-linux/11/lto-wrapper Target: aarch64-redhat-linux Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl=/builddir/build/BUILD/gcc-11.0.1-20210324/obj-aarch64-redhat-linux/isl-install --enable-gnu-indirect-function --build=aarch64-redhat-linux Thread model: posix Supported LTO compression algorithms: zlib zstd gcc version 11.0.1 20210324 (Red Hat 11.0.1-0) (GCC) Thanks,
On 11/15/21 05:57, Matthew Wilcox wrote: > On Mon, Nov 15, 2021 at 09:49:51PM +0800, Peter Xu wrote: >> Clean the code up by merging the device private/exclusive swap entry handling >> with the rest, then we merge the pte clear operation too. >> >> struct* page is defined in multiple places in the function, move it upward. > > Is that actually a good thing? There was a time when declaring Yes. It is a very good thing. Having multiple cases of shadowed variables (in this case I'm using programming language terminology, or what I remember it as, anyway) provides lots of opportunities to create hard-to-spot bugs. > variables more locally helped compilers with liveness analysis and > register allocation. Compilers are probably smarter now. > ...as long as the above checks out, and I see from Peter's response that we're OK. thanks,
On Tue, Nov 16, 2021 at 12:51:13AM -0800, John Hubbard wrote: > On 11/15/21 05:57, Matthew Wilcox wrote: > > On Mon, Nov 15, 2021 at 09:49:51PM +0800, Peter Xu wrote: > > > Clean the code up by merging the device private/exclusive swap entry handling > > > with the rest, then we merge the pte clear operation too. > > > > > > struct* page is defined in multiple places in the function, move it upward. > > > > Is that actually a good thing? There was a time when declaring > > Yes. It is a very good thing. Having multiple cases of shadowed variables > (in this case I'm using programming language terminology, or what I > remember it as, anyway) provides lots of opportunities to create > hard-to-spot bugs. I think you're misremembering. These are shadowed variables: int a; int b(void) { int a; if (c) { int a; } } This is not: int b(void) { if (c) { int a; } else { int a; } } I really wish we could turn on -Wshadow, but we get compilation warnings from header files right now. Or we did last time I checked.
On 11/16/21 05:11, Matthew Wilcox wrote: > On Tue, Nov 16, 2021 at 12:51:13AM -0800, John Hubbard wrote: >> On 11/15/21 05:57, Matthew Wilcox wrote: >>> On Mon, Nov 15, 2021 at 09:49:51PM +0800, Peter Xu wrote: >>>> Clean the code up by merging the device private/exclusive swap entry handling >>>> with the rest, then we merge the pte clear operation too. >>>> >>>> struct* page is defined in multiple places in the function, move it upward. >>> >>> Is that actually a good thing? There was a time when declaring >> >> Yes. It is a very good thing. Having multiple cases of shadowed variables >> (in this case I'm using programming language terminology, or what I >> remember it as, anyway) provides lots of opportunities to create >> hard-to-spot bugs. > > I think you're misremembering. These are shadowed variables: OK, I remembered correctly, but read the diffs a little too quickly, and... > > int a; > > int b(void) > { > int a; > if (c) { > int a; > } > } > > This is not: > > int b(void) > { ...missed that there is no longer a "int a" at the top level. But it does still present a small land mine, in that just adding a top level "int a" creates all these shadowed variables (not necessarily bugs, yet, I know). It's less of an issue here, then I first thought. Generally, it's probably best to either use "int a" throughout, or differently named variables at lower levels...or make smaller functions. Because if a variable name is reused a lot in the same function then there is likely a relationship of sorts between the instances, and it's worth deciding what that is. > if (c) { > int a; > } else { > int a; > } > } > > I really wish we could turn on -Wshadow, but we get compilation warnings > from header files right now. Or we did last time I checked. > ...and as you say, it would be nice if the programmer could just let the compiler figure out if there is a real problem. The elaborate rituals to stay out of harm's way are not as good as a tool that checks. :) thanks,
diff --git a/mm/memory.c b/mm/memory.c index e454f3c6aeb9..e5d59a6b6479 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -1326,6 +1326,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, arch_enter_lazy_mmu_mode(); do { pte_t ptent = *pte; + struct page *page; + if (pte_none(ptent)) continue; @@ -1333,8 +1335,6 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, break; if (pte_present(ptent)) { - struct page *page; - page = vm_normal_page(vma, addr, ptent); if (unlikely(zap_skip_check_mapping(details, page))) continue; @@ -1368,32 +1368,23 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, entry = pte_to_swp_entry(ptent); if (is_device_private_entry(entry) || is_device_exclusive_entry(entry)) { - struct page *page = pfn_swap_entry_to_page(entry); - + page = pfn_swap_entry_to_page(entry); if (unlikely(zap_skip_check_mapping(details, page))) continue; - pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); rss[mm_counter(page)]--; - if (is_device_private_entry(entry)) page_remove_rmap(page, false); - put_page(page); - continue; - } - - if (!non_swap_entry(entry)) - rss[MM_SWAPENTS]--; - else if (is_migration_entry(entry)) { - struct page *page; - + } else if (is_migration_entry(entry)) { page = pfn_swap_entry_to_page(entry); if (unlikely(zap_skip_check_mapping(details, page))) continue; rss[mm_counter(page)]--; + } else if (!non_swap_entry(entry)) { + rss[MM_SWAPENTS]--; + if (unlikely(!free_swap_and_cache(entry))) + print_bad_pte(vma, addr, ptent, NULL); } - if (unlikely(!free_swap_and_cache(entry))) - print_bad_pte(vma, addr, ptent, NULL); pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); } while (pte++, addr += PAGE_SIZE, addr != end);
Clean the code up by merging the device private/exclusive swap entry handling with the rest, then we merge the pte clear operation too. struct* page is defined in multiple places in the function, move it upward. free_swap_and_cache() is only useful for !non_swap_entry() case, put it into the condition. No functional change intended. Signed-off-by: Peter Xu <peterx@redhat.com> --- mm/memory.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)