* [PATCH 0/3] mm: Some rework on zap_details
@ 2020-12-08 2:50 Peter Xu
2020-12-08 2:50 ` [PATCH 1/3] mm: Drop first_index/last_index in zap_details Peter Xu
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Peter Xu @ 2020-12-08 2:50 UTC (permalink / raw)
To: linux-kernel, linux-mm
Cc: Andrea Arcangeli, peterx, Kirill A . Shutemov, Andrew Morton
Posted this small series out to rework zap_details a bit, before adding
something new in. Hopefully it makes things slighly clearer.
Smoke test only. Please have a look, thanks.
Peter Xu (3):
mm: Drop first_index/last_index in zap_details
mm: Introduce zap_details.zap_flags
mm: Introduce ZAP_FLAG_SKIP_SWAP
include/linux/mm.h | 33 ++++++++++++++++++++++++---
mm/memory.c | 57 ++++++++++++++++++----------------------------
2 files changed, 52 insertions(+), 38 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/3] mm: Drop first_index/last_index in zap_details
2020-12-08 2:50 [PATCH 0/3] mm: Some rework on zap_details Peter Xu
@ 2020-12-08 2:50 ` Peter Xu
2020-12-08 2:50 ` [PATCH 2/3] mm: Introduce zap_details.zap_flags Peter Xu
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Peter Xu @ 2020-12-08 2:50 UTC (permalink / raw)
To: linux-kernel, linux-mm
Cc: Andrea Arcangeli, peterx, Kirill A . Shutemov, Andrew Morton
The first_index/last_index parameters in zap_details are actually only used in
unmap_mapping_range_tree(). At the meantime, this function is only called by
unmap_mapping_pages() once. Instead of passing these two variables through the
whole stack of page zapping code, remove them from zap_details and let them
simply be parameters of unmap_mapping_range_tree(), which is inlined.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/linux/mm.h | 2 --
mm/memory.c | 20 ++++++++++----------
2 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 16b799a0522c..5d977e484095 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1625,8 +1625,6 @@ extern void user_shm_unlock(size_t, struct user_struct *);
*/
struct zap_details {
struct address_space *check_mapping; /* Check page->mapping if set */
- pgoff_t first_index; /* Lowest page->index to unmap */
- pgoff_t last_index; /* Highest page->index to unmap */
};
struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
diff --git a/mm/memory.c b/mm/memory.c
index eeae590e526a..70d57c39380d 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3136,20 +3136,20 @@ static void unmap_mapping_range_vma(struct vm_area_struct *vma,
}
static inline void unmap_mapping_range_tree(struct rb_root_cached *root,
+ pgoff_t first_index,
+ pgoff_t last_index,
struct zap_details *details)
{
struct vm_area_struct *vma;
pgoff_t vba, vea, zba, zea;
- vma_interval_tree_foreach(vma, root,
- details->first_index, details->last_index) {
-
+ vma_interval_tree_foreach(vma, root, first_index, last_index) {
vba = vma->vm_pgoff;
vea = vba + vma_pages(vma) - 1;
- zba = details->first_index;
+ zba = first_index;
if (zba < vba)
zba = vba;
- zea = details->last_index;
+ zea = last_index;
if (zea > vea)
zea = vea;
@@ -3175,17 +3175,17 @@ static inline void unmap_mapping_range_tree(struct rb_root_cached *root,
void unmap_mapping_pages(struct address_space *mapping, pgoff_t start,
pgoff_t nr, bool even_cows)
{
+ pgoff_t first_index = start, last_index = start + nr - 1;
struct zap_details details = { };
details.check_mapping = even_cows ? NULL : mapping;
- details.first_index = start;
- details.last_index = start + nr - 1;
- if (details.last_index < details.first_index)
- details.last_index = ULONG_MAX;
+ if (last_index < first_index)
+ last_index = ULONG_MAX;
i_mmap_lock_write(mapping);
if (unlikely(!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root)))
- unmap_mapping_range_tree(&mapping->i_mmap, &details);
+ unmap_mapping_range_tree(&mapping->i_mmap, first_index,
+ last_index, &details);
i_mmap_unlock_write(mapping);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] mm: Introduce zap_details.zap_flags
2020-12-08 2:50 [PATCH 0/3] mm: Some rework on zap_details Peter Xu
2020-12-08 2:50 ` [PATCH 1/3] mm: Drop first_index/last_index in zap_details Peter Xu
@ 2020-12-08 2:50 ` Peter Xu
2020-12-08 2:50 ` [PATCH 3/3] mm: Introduce ZAP_FLAG_SKIP_SWAP Peter Xu
2020-12-15 20:39 ` [PATCH 0/3] mm: Some rework on zap_details Peter Xu
3 siblings, 0 replies; 5+ messages in thread
From: Peter Xu @ 2020-12-08 2:50 UTC (permalink / raw)
To: linux-kernel, linux-mm
Cc: Andrea Arcangeli, peterx, Kirill A . Shutemov, Andrew Morton
Instead of trying to introduce one variable for every new zap_details fields,
let's introduce a flag so that it can start to encode true/false informations.
Let's start to use this flag first to clean up the only check_mapping variable.
Firstly, the name "check_mapping" implies this is a "boolean", but actually it
stores the mapping inside, just in a way that it won't be set if we don't want
to check the mapping.
To make things clearer, introduce the 1st zap flag ZAP_FLAG_CHECK_MAPPING, so
that we only check against the mapping if this bit set. At the same time, we
can rename check_mapping into zap_mapping and set it always.
Since at it, introduce another helper zap_check_mapping_skip() and use it in
zap_pte_range() properly.
Some old comments have been removed in zap_pte_range() because they're
duplicated, and since now we're with ZAP_FLAG_CHECK_MAPPING flag, it'll be very
easy to grep this information by simply grepping the flag.
It'll also make life easier when we want to e.g. pass in zap_flags into the
callers like unmap_mapping_pages() (instead of adding new booleans besides the
even_cows parameter).
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/linux/mm.h | 19 ++++++++++++++++++-
mm/memory.c | 31 ++++++++-----------------------
2 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5d977e484095..7ed4352ec84f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1620,13 +1620,30 @@ static inline bool can_do_mlock(void) { return false; }
extern int user_shm_lock(size_t, struct user_struct *);
extern void user_shm_unlock(size_t, struct user_struct *);
+/* Whether to check page->mapping when zapping */
+#define ZAP_FLAG_CHECK_MAPPING BIT(0)
+
/*
* Parameter block passed down to zap_pte_range in exceptional cases.
*/
struct zap_details {
- struct address_space *check_mapping; /* Check page->mapping if set */
+ struct address_space *zap_mapping; /* Check page->mapping if set */
+ unsigned long zap_flags; /* Special flags for zapping */
};
+/* Return true if skip zapping this page, false otherwise */
+static inline bool
+zap_check_mapping_skip(struct zap_details *details, struct page *page)
+{
+ if (!details || !page)
+ return false;
+
+ if (!(details->zap_flags & ZAP_FLAG_CHECK_MAPPING))
+ return false;
+
+ return details->zap_mapping != page_rmapping(page);
+}
+
struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
pte_t pte);
struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,
diff --git a/mm/memory.c b/mm/memory.c
index 70d57c39380d..20a8ba05c334 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1220,16 +1220,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
struct page *page;
page = vm_normal_page(vma, addr, ptent);
- if (unlikely(details) && page) {
- /*
- * unmap_shared_mapping_pages() wants to
- * invalidate cache without truncating:
- * unmap shared but keep private pages.
- */
- if (details->check_mapping &&
- details->check_mapping != page_rmapping(page))
- continue;
- }
+ if (unlikely(zap_check_mapping_skip(details, page)))
+ continue;
ptent = ptep_get_and_clear_full(mm, addr, pte,
tlb->fullmm);
tlb_remove_tlb_entry(tlb, pte, addr);
@@ -1261,17 +1253,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
if (is_device_private_entry(entry)) {
struct page *page = device_private_entry_to_page(entry);
- if (unlikely(details && details->check_mapping)) {
- /*
- * unmap_shared_mapping_pages() wants to
- * invalidate cache without truncating:
- * unmap shared but keep private pages.
- */
- if (details->check_mapping !=
- page_rmapping(page))
- continue;
- }
-
+ if (unlikely(zap_check_mapping_skip(details, page)))
+ continue;
pte_clear_not_present_full(mm, addr, pte, tlb->fullmm);
rss[mm_counter(page)]--;
page_remove_rmap(page, false);
@@ -3176,9 +3159,11 @@ void unmap_mapping_pages(struct address_space *mapping, pgoff_t start,
pgoff_t nr, bool even_cows)
{
pgoff_t first_index = start, last_index = start + nr - 1;
- struct zap_details details = { };
+ struct zap_details details = { .zap_mapping = mapping };
+
+ if (!even_cows)
+ details.zap_flags |= ZAP_FLAG_CHECK_MAPPING;
- details.check_mapping = even_cows ? NULL : mapping;
if (last_index < first_index)
last_index = ULONG_MAX;
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] mm: Introduce ZAP_FLAG_SKIP_SWAP
2020-12-08 2:50 [PATCH 0/3] mm: Some rework on zap_details Peter Xu
2020-12-08 2:50 ` [PATCH 1/3] mm: Drop first_index/last_index in zap_details Peter Xu
2020-12-08 2:50 ` [PATCH 2/3] mm: Introduce zap_details.zap_flags Peter Xu
@ 2020-12-08 2:50 ` Peter Xu
2020-12-15 20:39 ` [PATCH 0/3] mm: Some rework on zap_details Peter Xu
3 siblings, 0 replies; 5+ messages in thread
From: Peter Xu @ 2020-12-08 2:50 UTC (permalink / raw)
To: linux-kernel, linux-mm
Cc: Andrea Arcangeli, peterx, Kirill A . Shutemov, Andrew Morton
Firstly, the comment in zap_pte_range() is misleading because it checks against
details rather than check_mappings, so it's against what the code did.
Meanwhile, it's confusing too on not explaining why passing in the details
pointer would mean to skip all swap entries. New user of zap_details could
very possibly miss this fact if they don't read deep until zap_pte_range()
because there's no comment at zap_details talking about it at all, so swap
entries could be errornously skipped without being noticed.
This partly reverts 3e8715fdc03e ("mm: drop zap_details::check_swap_entries"),
but introduce ZAP_FLAG_SKIP_SWAP flag, which means the opposite of previous
"details" parameter: the caller should explicitly set this to skip swap
entries, otherwise swap entries will always be considered (which is still the
major case here).
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/linux/mm.h | 12 ++++++++++++
mm/memory.c | 8 +++++---
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7ed4352ec84f..16631ee5eb9d 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1622,6 +1622,8 @@ extern void user_shm_unlock(size_t, struct user_struct *);
/* Whether to check page->mapping when zapping */
#define ZAP_FLAG_CHECK_MAPPING BIT(0)
+/* Whether to skip zapping swap entries */
+#define ZAP_FLAG_SKIP_SWAP BIT(1)
/*
* Parameter block passed down to zap_pte_range in exceptional cases.
@@ -1644,6 +1646,16 @@ zap_check_mapping_skip(struct zap_details *details, struct page *page)
return details->zap_mapping != page_rmapping(page);
}
+/* Return true if skip swap entries, false otherwise */
+static inline bool
+zap_skip_swap(struct zap_details *details)
+{
+ if (!details)
+ return false;
+
+ return details->zap_flags & ZAP_FLAG_SKIP_SWAP;
+}
+
struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
pte_t pte);
struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,
diff --git a/mm/memory.c b/mm/memory.c
index 20a8ba05c334..c9945f3c374d 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1262,8 +1262,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
continue;
}
- /* If details->check_mapping, we leave swap entries. */
- if (unlikely(details))
+ if (unlikely(zap_skip_swap(details)))
continue;
if (!non_swap_entry(entry))
@@ -3159,7 +3158,10 @@ void unmap_mapping_pages(struct address_space *mapping, pgoff_t start,
pgoff_t nr, bool even_cows)
{
pgoff_t first_index = start, last_index = start + nr - 1;
- struct zap_details details = { .zap_mapping = mapping };
+ struct zap_details details = {
+ .zap_mapping = mapping,
+ .zap_flags = ZAP_FLAG_SKIP_SWAP,
+ };
if (!even_cows)
details.zap_flags |= ZAP_FLAG_CHECK_MAPPING;
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 0/3] mm: Some rework on zap_details
2020-12-08 2:50 [PATCH 0/3] mm: Some rework on zap_details Peter Xu
` (2 preceding siblings ...)
2020-12-08 2:50 ` [PATCH 3/3] mm: Introduce ZAP_FLAG_SKIP_SWAP Peter Xu
@ 2020-12-15 20:39 ` Peter Xu
3 siblings, 0 replies; 5+ messages in thread
From: Peter Xu @ 2020-12-15 20:39 UTC (permalink / raw)
To: linux-kernel, linux-mm
Cc: Andrea Arcangeli, Kirill A . Shutemov, Andrew Morton
On Mon, Dec 07, 2020 at 09:50:19PM -0500, Peter Xu wrote:
> Posted this small series out to rework zap_details a bit, before adding
> something new in. Hopefully it makes things slighly clearer.
>
> Smoke test only. Please have a look, thanks.
>
> Peter Xu (3):
> mm: Drop first_index/last_index in zap_details
> mm: Introduce zap_details.zap_flags
> mm: Introduce ZAP_FLAG_SKIP_SWAP
Some more information on "before adding something new in" - the new flag as a
reference but not yet posted anywhere...
https://github.com/xzpeter/linux/commit/102790511a1a25b7be841fb2059a8c8d8f1a87b2
I still think this small series worths to be reviewed/merged even before that
new flag, because patch 1 should definitely be something good to have, patch 2
prepares for patch 3 and the new bit (which can be seen as optional), but patch
3 should help make things clearer rather than using non-null "details" pointer
to show "whether we should skip swap entries", which can be easily misused IMHO
when someone accidentally replaced one "details==NULL" with some valid pointer.
Of course I can repost these series with the larger one when time comes too,
but I'd still like to at least get a NO early if there is...
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-15 20:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-08 2:50 [PATCH 0/3] mm: Some rework on zap_details Peter Xu
2020-12-08 2:50 ` [PATCH 1/3] mm: Drop first_index/last_index in zap_details Peter Xu
2020-12-08 2:50 ` [PATCH 2/3] mm: Introduce zap_details.zap_flags Peter Xu
2020-12-08 2:50 ` [PATCH 3/3] mm: Introduce ZAP_FLAG_SKIP_SWAP Peter Xu
2020-12-15 20:39 ` [PATCH 0/3] mm: Some rework on zap_details Peter Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).