On 11.01.22 08:40, Alistair Popple wrote: > On Monday, 10 January 2022 7:37:15 PM AEDT David Hildenbrand wrote: >> On 15.11.21 14:49, Peter Xu wrote: >>> This check existed since the 1st git commit of Linux repository, but at that >>> time there's no page migration yet so I think it's okay. >>> >>> With page migration enabled, it should logically be possible that we zap some >>> shmem pages during migration. When that happens, IIUC the old code could have >>> the RSS counter accounted wrong on MM_SHMEMPAGES because we will zap the ptes >>> without decreasing the counters for the migrating entries. I have no unit test >>> to prove it as I don't know an easy way to trigger this condition, though. >>> >>> Besides, the optimization itself is already confusing IMHO to me in a few points: >>> >>> - The wording "skip swap entries" is confusing, because we're not skipping all >>> swap entries - we handle device private/exclusive pages before that. >> >> I think one part of the confusion is "swap vs non-swap" entries. >> For !pte_none() && !pte_present() we can have >> >> * swap entry >> * non-swap entry >> ** device exclusive entry >> ** device private entry >> ** HWpoison entry >> ** migration entry >> >> So the comment claims to skip "swap entries" but also skips HWpoison and >> migration entries, and I think that's the confusing part. >> Both only apply to PageAnon(). > > I must be missing something but why do these only apply to PageAnon()? My memory might be wrong. I remember that for PageAnon() we need migration/hwpoison entries because there is no way we could refault the page from a mapping once we zap the entry. For everything else, we could zap and refault. But looks like we indeed also use migration/hwpoison entries for pages with a mapping, although it might not be strictly required. > >> IIUC, the only way we could get details != NULL is via unmap_mapping_page()+unmap_mapping_pages(). >> >> I do wonder if any of the callers really cares about PageAnon() pages where this would be relevant. >> >> Am I wrong or is unmap_mapping_pages() never called with "even_cows == true" and we can remove >> that paremeter: > > Except that unmap_mapping_range() takes `even_cows` as a parameter and passes > that to unmap_mapping_pages(), and from what I can tell there are callers of > unmap_mapping_range() that set `even_cows = true`. You're right. -- Thanks, David / dhildenb
