mm/kdump: allow to exclude pages that are logically offline

Rename PG_balloon to PG_offline. This is an indicator that the page is logically offline, the content stale and that it should not be touched (e.g. a hypervisor would have to allocate backing storage in order for the guest to dump an unused page). We can then e.g. exclude such pages from dumps.

In following patches, we will make use of this bit also in other balloon drivers. While at it, document PGTABLE.

Quoted 6 lines · David Hildenbrand

Rename PG_balloon to PG_offline. This is an indicator that the page is logically offline, the content stale and that it should not be touched (e.g. a hypervisor would have to allocate backing storage in order for the guest to dump an unused page). We can then e.g. exclude such pages from dumps.

Quoted 2 lines

In following patches, we will make use of this bit also in other balloon drivers. While at it, document PGTABLE.

Thank you for documenting PGTABLE. I didn't realise I also had this document to update when I added PGTABLE.

Documentation/admin-guide/mm/pagemap.rst number of times a page is mapped. · line 82

Quoted 8 lines

+++ b/Documentation/admin-guide/mm/pagemap.rst

Documentation/admin-guide/mm/pagemap.rst +2 -0

@@ -78,6 +78,8 @@ number of times a page is mapped.
     23. BALLOON
     24. ZERO_PAGE
     25. IDLE
+    26. PGTABLE
+    27. OFFLINE

So the offline *user* bit is new ... even though the *kernel* bit just renames the balloon bit. I'm not sure how I feel about this. I'm going to think about it some more. Could you share your decision process with us?

I have no objection to renaming the balloon bit inside the kernel; I think that's a wise idea. I'm just not sure whether we should rename the user balloon bit rather than adding a new bit.

Quoted 7 lines · Matthew Wilcox

Quoted 6 lines · David Hildenbrand

Rename PG_balloon to PG_offline. This is an indicator that the page is logically offline, the content stale and that it should not be touched (e.g. a hypervisor would have to allocate backing storage in order for the guest to dump an unused page). We can then e.g. exclude such pages from dumps.

Quoted 5 lines

Quoted 2 lines

In following patches, we will make use of this bit also in other balloon drivers. While at it, document PGTABLE.

Thank you for documenting PGTABLE. I didn't realise I also had this document to update when I added PGTABLE.

Thank you for looking into this :)

Quoted 13 lines

Documentation/admin-guide/mm/pagemap.rst number of times a page is mapped. · line 82

Quoted 8 lines

+++ b/Documentation/admin-guide/mm/pagemap.rst

Documentation/admin-guide/mm/pagemap.rst +2 -0

@@ -78,6 +78,8 @@ number of times a page is mapped.
     23. BALLOON
     24. ZERO_PAGE
     25. IDLE
+    26. PGTABLE
+    27. OFFLINE

So the offline *user* bit is new ... even though the *kernel* bit just renames the balloon bit. I'm not sure how I feel about this. I'm going to think about it some more. Could you share your decision process with us?

BALLOON was/is documented as

"23 - BALLOON
balloon compaction page
"

and only includes all virtio-ballon pages after the non-lru migration feature has been implemented for ballooned pages. Since then, this flag does basically no longer stands for what it actually was supposed to do.

To not break uapi I decided to not rename it but instead to add a new flag.

Quoted 5 lines

I have no objection to renaming the balloon bit inside the kernel; I think that's a wise idea. I'm just not sure whether we should rename the user balloon bit rather than adding a new bit.

Can we rename without breaking uapi?

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 36 lines · David Hildenbrand

Quoted 7 lines · Matthew Wilcox

Quoted 6 lines · David Hildenbrand

Rename PG_balloon to PG_offline. This is an indicator that the page is logically offline, the content stale and that it should not be touched (e.g. a hypervisor would have to allocate backing storage in order for the guest to dump an unused page). We can then e.g. exclude such pages from dumps.

Quoted 5 lines

Quoted 2 lines

In following patches, we will make use of this bit also in other balloon drivers. While at it, document PGTABLE.

Thank you for documenting PGTABLE. I didn't realise I also had this document to update when I added PGTABLE.

Thank you for looking into this :)

Quoted 13 lines

Documentation/admin-guide/mm/pagemap.rst number of times a page is mapped. · line 82

Quoted 8 lines

+++ b/Documentation/admin-guide/mm/pagemap.rst

Documentation/admin-guide/mm/pagemap.rst +2 -0

@@ -78,6 +78,8 @@ number of times a page is mapped.
     23. BALLOON
     24. ZERO_PAGE
     25. IDLE
+    26. PGTABLE
+    27. OFFLINE

So the offline *user* bit is new ... even though the *kernel* bit just renames the balloon bit. I'm not sure how I feel about this. I'm going to think about it some more. Could you share your decision process with us?

BALLOON was/is documented as

"23 - BALLOON
balloon compaction page
"

Quoted 3 lines

and only includes all virtio-ballon pages after the non-lru migration feature has been implemented for ballooned pages. Since then, this flag does basically no longer stands for what it actually was supposed to do.

Perhaps I missing something, but how the user should interpret "23" when he reads /proc/kpageflags?

Quoted 2 lines

To not break uapi I decided to not rename it but instead to add a new flag.

I've got a feeling that uapi was anyway changed for the BALLON flag meaning.

Quoted 37 lines · Mike Rapoport

Quoted 36 lines · David Hildenbrand

Quoted 7 lines · Matthew Wilcox

Quoted 6 lines · David Hildenbrand

Rename PG_balloon to PG_offline. This is an indicator that the page is logically offline, the content stale and that it should not be touched (e.g. a hypervisor would have to allocate backing storage in order for the guest to dump an unused page). We can then e.g. exclude such pages from dumps.

Quoted 5 lines

Quoted 2 lines

In following patches, we will make use of this bit also in other balloon drivers. While at it, document PGTABLE.

Thank you for documenting PGTABLE. I didn't realise I also had this document to update when I added PGTABLE.

Thank you for looking into this :)

Quoted 13 lines

Documentation/admin-guide/mm/pagemap.rst number of times a page is mapped. · line 82

Quoted 8 lines

+++ b/Documentation/admin-guide/mm/pagemap.rst

Documentation/admin-guide/mm/pagemap.rst +2 -0

@@ -78,6 +78,8 @@ number of times a page is mapped.
     23. BALLOON
     24. ZERO_PAGE
     25. IDLE
+    26. PGTABLE
+    27. OFFLINE

So the offline *user* bit is new ... even though the *kernel* bit just renames the balloon bit. I'm not sure how I feel about this. I'm going to think about it some more. Could you share your decision process with us?

BALLOON was/is documented as

"23 - BALLOON
balloon compaction page
"

Quoted 6 lines

Quoted 3 lines

and only includes all virtio-ballon pages after the non-lru migration feature has been implemented for ballooned pages. Since then, this flag does basically no longer stands for what it actually was supposed to do.

Perhaps I missing something, but how the user should interpret "23" when he reads /proc/kpageflags?

Looking at the history in more detail:

commit 09316c09dde33aae14f34489d9e3d243ec0d5938

Author: Konstantin Khlebnikov <[email protected]>
Date:   Thu Oct 9 15:29:32 2014 -0700

mm/balloon_compaction: add vmstat counters and kpageflags bit

Always mark pages with PageBalloon even if balloon compaction is
disabled
and expose this mark in /proc/kpageflags as KPF_BALLOON.

So KPF_BALLOON was exposed when virtio-balloon pages were always marked with PG_balloon. So the documentation is actually wrong ("balloon page" vs. "balloon compaction page").

I have no idea who actually used that information. I suspect this was just some debugging aid.

Quoted 5 lines

Quoted 2 lines

To not break uapi I decided to not rename it but instead to add a new flag.

I've got a feeling that uapi was anyway changed for the BALLON flag meaning.

Yes. If we *replace* KPF_BALLOON by KPF_OFFLINE

a) Some applications might no longer compile (I guess that's ok)
b) Some old applications will treat KPF_OFFLINE like KPF_BALLOON (which should at least for virtio-balloon usage until now be fine - it is just more generic)

Quoted 37 lines · Mike Rapoport

Quoted 36 lines · David Hildenbrand

Quoted 7 lines · Matthew Wilcox

Quoted 6 lines · David Hildenbrand

Rename PG_balloon to PG_offline. This is an indicator that the page is logically offline, the content stale and that it should not be touched (e.g. a hypervisor would have to allocate backing storage in order for the guest to dump an unused page). We can then e.g. exclude such pages from dumps.

Quoted 5 lines

Quoted 2 lines

In following patches, we will make use of this bit also in other balloon drivers. While at it, document PGTABLE.

Thank you for documenting PGTABLE. I didn't realise I also had this document to update when I added PGTABLE.

Thank you for looking into this :)

Quoted 13 lines

Documentation/admin-guide/mm/pagemap.rst number of times a page is mapped. · line 82

Quoted 8 lines

+++ b/Documentation/admin-guide/mm/pagemap.rst

Documentation/admin-guide/mm/pagemap.rst +2 -0

@@ -78,6 +78,8 @@ number of times a page is mapped.
     23. BALLOON
     24. ZERO_PAGE
     25. IDLE
+    26. PGTABLE
+    27. OFFLINE

So the offline *user* bit is new ... even though the *kernel* bit just renames the balloon bit. I'm not sure how I feel about this. I'm going to think about it some more. Could you share your decision process with us?

BALLOON was/is documented as

"23 - BALLOON
balloon compaction page
"

Quoted 6 lines

Quoted 3 lines

and only includes all virtio-ballon pages after the non-lru migration feature has been implemented for ballooned pages. Since then, this flag does basically no longer stands for what it actually was supposed to do.

Perhaps I missing something, but how the user should interpret "23" when he reads /proc/kpageflags?

Looking at the history in more detail:

commit 09316c09dde33aae14f34489d9e3d243ec0d5938

Author: Konstantin Khlebnikov <[email protected]>
Date:   Thu Oct 9 15:29:32 2014 -0700

mm/balloon_compaction: add vmstat counters and kpageflags bit

Always mark pages with PageBalloon even if balloon compaction is
disabled
and expose this mark in /proc/kpageflags as KPF_BALLOON.

So KPF_BALLOON was exposed when virtio-balloon pages were always marked with PG_balloon. So the documentation is actually wrong ("balloon page" vs. "balloon compaction page").

I have no idea who actually used that information. I suspect this was just some debugging aid.

Quoted 5 lines

Quoted 2 lines

To not break uapi I decided to not rename it but instead to add a new flag.

I've got a feeling that uapi was anyway changed for the BALLON flag meaning.

Yes. If we *replace* KPF_BALLOON by KPF_OFFLINE

a) Some applications might no longer compile (I guess that's ok)
b) Some old applications will treat KPF_OFFLINE like KPF_BALLOON (which should at least for virtio-balloon usage until now be fine - it is just more generic)

Quoted 36 lines · David Hildenbrand

Quoted 7 lines · Matthew Wilcox

Quoted 6 lines · David Hildenbrand

Rename PG_balloon to PG_offline. This is an indicator that the page is logically offline, the content stale and that it should not be touched (e.g. a hypervisor would have to allocate backing storage in order for the guest to dump an unused page). We can then e.g. exclude such pages from dumps.

Quoted 5 lines

Quoted 2 lines

In following patches, we will make use of this bit also in other balloon drivers. While at it, document PGTABLE.

Thank you for documenting PGTABLE. I didn't realise I also had this document to update when I added PGTABLE.

Thank you for looking into this :)

Quoted 13 lines

Documentation/admin-guide/mm/pagemap.rst number of times a page is mapped. · line 82

Quoted 8 lines

+++ b/Documentation/admin-guide/mm/pagemap.rst

Documentation/admin-guide/mm/pagemap.rst +2 -0

@@ -78,6 +78,8 @@ number of times a page is mapped.
     23. BALLOON
     24. ZERO_PAGE
     25. IDLE
+    26. PGTABLE
+    27. OFFLINE

So the offline *user* bit is new ... even though the *kernel* bit just renames the balloon bit. I'm not sure how I feel about this. I'm going to think about it some more. Could you share your decision process with us?

BALLOON was/is documented as

"23 - BALLOON
balloon compaction page
"

Quoted 3 lines

and only includes all virtio-ballon pages after the non-lru migration feature has been implemented for ballooned pages. Since then, this flag does basically no longer stands for what it actually was supposed to do.

Perhaps I missing something, but how the user should interpret "23" when he reads /proc/kpageflags?

Quoted 2 lines

To not break uapi I decided to not rename it but instead to add a new flag.

I've got a feeling that uapi was anyway changed for the BALLON flag meaning.

Quoted 7 lines · Matthew Wilcox

Quoted 6 lines · David Hildenbrand

Rename PG_balloon to PG_offline. This is an indicator that the page is logically offline, the content stale and that it should not be touched (e.g. a hypervisor would have to allocate backing storage in order for the guest to dump an unused page). We can then e.g. exclude such pages from dumps.

Quoted 5 lines

Quoted 2 lines

In following patches, we will make use of this bit also in other balloon drivers. While at it, document PGTABLE.

Thank you for documenting PGTABLE. I didn't realise I also had this document to update when I added PGTABLE.

Thank you for looking into this :)

Quoted 13 lines

Documentation/admin-guide/mm/pagemap.rst number of times a page is mapped. · line 82

Quoted 8 lines

+++ b/Documentation/admin-guide/mm/pagemap.rst

Documentation/admin-guide/mm/pagemap.rst +2 -0

@@ -78,6 +78,8 @@ number of times a page is mapped.
     23. BALLOON
     24. ZERO_PAGE
     25. IDLE
+    26. PGTABLE
+    27. OFFLINE

So the offline *user* bit is new ... even though the *kernel* bit just renames the balloon bit. I'm not sure how I feel about this. I'm going to think about it some more. Could you share your decision process with us?

BALLOON was/is documented as

"23 - BALLOON
balloon compaction page
"

and only includes all virtio-ballon pages after the non-lru migration feature has been implemented for ballooned pages. Since then, this flag does basically no longer stands for what it actually was supposed to do.

To not break uapi I decided to not rename it but instead to add a new flag.

Quoted 5 lines

I have no objection to renaming the balloon bit inside the kernel; I think that's a wise idea. I'm just not sure whether we should rename the user balloon bit rather than adding a new bit.

Can we rename without breaking uapi?

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 6 lines · David Hildenbrand

Rename PG_balloon to PG_offline. This is an indicator that the page is logically offline, the content stale and that it should not be touched (e.g. a hypervisor would have to allocate backing storage in order for the guest to dump an unused page). We can then e.g. exclude such pages from dumps.

Quoted 2 lines

In following patches, we will make use of this bit also in other balloon drivers. While at it, document PGTABLE.

Thank you for documenting PGTABLE. I didn't realise I also had this document to update when I added PGTABLE.

Documentation/admin-guide/mm/pagemap.rst number of times a page is mapped. · line 82

Quoted 8 lines

+++ b/Documentation/admin-guide/mm/pagemap.rst

Documentation/admin-guide/mm/pagemap.rst +2 -0

@@ -78,6 +78,8 @@ number of times a page is mapped.
     23. BALLOON
     24. ZERO_PAGE
     25. IDLE
+    26. PGTABLE
+    27. OFFLINE

So the offline *user* bit is new ... even though the *kernel* bit just renames the balloon bit. I'm not sure how I feel about this. I'm going to think about it some more. Could you share your decision process with us?

I have no objection to renaming the balloon bit inside the kernel; I think that's a wise idea. I'm just not sure whether we should rename the user balloon bit rather than adding a new bit.

Rename PG_balloon to PG_offline. This is an indicator that the page is logically offline, the content stale and that it should not be touched (e.g. a hypervisor would have to allocate backing storage in order for the guest to dump an unused page). We can then e.g. exclude such pages from dumps.

In following patches, we will make use of this bit also in other balloon drivers. While at it, document PGTABLE.

Let's export PG_offline via PAGE_OFFLINE_MAPCOUNT_VALUE, so makedumpfile can directly skip pages that are logically offline and the content therefore stale.

Cc Andrew Morton <[email protected]>

Cc Dave Young <[email protected]>

Cc "Kirill A. Shutemov" <[email protected]>

Cc Baoquan He <[email protected]>

Cc Omar Sandoval <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc Michal Hocko <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 kernel/crash_core.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/crash_core.c +2 -0

index 933cb3e45b98..093c9f917ed0 100644
@@ -464,6 +464,8 @@ static int __init crash_save_vmcoreinfo_init(void)
 	VMCOREINFO_NUMBER(PAGE_BUDDY_MAPCOUNT_VALUE);
 #ifdef CONFIG_HUGETLB_PAGE
 	VMCOREINFO_NUMBER(HUGETLB_PAGE_DTOR);
+#define PAGE_OFFLINE_MAPCOUNT_VALUE	(~PG_offline)
+	VMCOREINFO_NUMBER(PAGE_OFFLINE_MAPCOUNT_VALUE);
 #endif
 
 	arch_crash_save_vmcoreinfo();

Signature · 1 line

2.17.2

Hi David,

Quoted 3 lines · David Hildenbrand

Let's export PG_offline via PAGE_OFFLINE_MAPCOUNT_VALUE, so makedumpfile can directly skip pages that are logically offline and the content therefore stale.

It would be good to copy some background info from cover letter to the patch description so that we can get better understanding why this is needed now.

BTW, Lianbo is working on a documentation of the vmcoreinfo exported fields. Ccing him so that he is aware of this.

Also cc Boris, although I do not want the doc changes blocks this he might have different opinion :)

Hi David,

Quoted 3 lines · David Hildenbrand

Let's export PG_offline via PAGE_OFFLINE_MAPCOUNT_VALUE, so makedumpfile can directly skip pages that are logically offline and the content therefore stale.

It would be good to copy some background info from cover letter to the patch description so that we can get better understanding why this is needed now.

BTW, Lianbo is working on a documentation of the vmcoreinfo exported fields. Ccing him so that he is aware of this.

Also cc Boris, although I do not want the doc changes blocks this he might have different opinion :)

It would be good to copy some background info from cover letter to the patch description so that we can get better understanding why this is needed now.

BTW, Lianbo is working on a documentation of the vmcoreinfo exported fields. Ccing him so that he is aware of this.

Also cc Boris, although I do not want the doc changes blocks this he might have different opinion :)

Quoted 4 lines · Dave Young

It would be good to copy some background info from cover letter to the patch description so that we can get better understanding why this is needed now.

Quoted 5 lines

BTW, Lianbo is working on a documentation of the vmcoreinfo exported fields. Ccing him so that he is aware of this.

Also cc Boris, although I do not want the doc changes blocks this he might have different opinion :)

Yeah, my initial reaction is that exporting an mm-internal flag to userspace is a no-no.

What would be better, IMHO, is having a general method of telling the kdump kernel - maybe ranges of physical addresses - which to skip.

Because the moment there's a set of pages which do not have PG_offline set but kdump would still like to skip, this breaks.

Sorry to say, but that is the current practice without which makedumpfile would not be able to work at all. (exclude user pages, exclude page cache, exclude buddy pages). Let's not reinvent the wheel here. This is how dumping works forever.

I don't see how there should be "set of pages which do not have PG_offline".

Quoted 4 lines · David Hildenbrand

Sorry to say, but that is the current practice without which makedumpfile would not be able to work at all. (exclude user pages, exclude page cache, exclude buddy pages). Let's not reinvent the wheel here. This is how dumping works forever.

Sorry, but "we've always done this in the past" doesn't make it better.

Quoted 3 lines

I don't see how there should be "set of pages which do not have PG_offline".

It doesn't have to be a set of pages. Think a (mmconfig perhaps) region which the kdump kernel should completely skip because poking in it in the kdump kernel, causes all kinds of havoc like machine checks. etc. We've had and still have one issue like that.

But let me clarify my note: I don't want to be discussing with you the design of makedumpfile and how it should or should not work - that ship has already sailed. Apparently there are valid reasons to do it this way.

I was *simply* stating that it feels wrong to export mm flags like that.

But as I said already, that is mm guys' call and looking at how we're already exporting a bunch of stuff in the vmcoreinfo - including other mm flags - I guess one more flag doesn't matter anymore.

Just saying that "I'm not the first to do it, don't hit me with a stick" :)

Indeed. And we still have without makedumpfile. I think you are aware of this, but I'll explain it just for consistency: PG_hwpoison

At some point we detect a HW error and mask a page as PG_hwpoison.

makedumpfile knows how to treat that flag and can exclude it from the dump (== not access it). No crash.

kdump itself has no clue about old "struct pages". Especially:
a) Where they are located in memory (e.g. SPARSE)
b) What their format is ("where are the flags")
c) What the meaning of flags is ("what does bit X mean")

In order to know such information, we would have to do parsing of quite some information inside the kernel in kdump. Basically what makedumpfile does just now. Is this feasible? I don't think so.

So we would need another approach to communicate such information as you said. I can't think of any, but if anybody reading this has an idea, please speak up. I am interested.

The *only* way right now we would have to handle such scenarios:

1. While dumping memory and we get a machine check, fake reading a zero page instead of crashing.
2. While dumping memory and we get a fault, fake reading a zero page instead of crashing.

Indeed, and the basic design is to export these flags. (let's say "unfortunately", being able to handle such stuff in kdump directly would be the dream).

Just saying that "I'm not the first to do it, don't hit me with a stick" :)

Indeed. And we still have without makedumpfile. I think you are aware of this, but I'll explain it just for consistency: PG_hwpoison

At some point we detect a HW error and mask a page as PG_hwpoison.

makedumpfile knows how to treat that flag and can exclude it from the dump (== not access it). No crash.

kdump itself has no clue about old "struct pages". Especially:
a) Where they are located in memory (e.g. SPARSE)
b) What their format is ("where are the flags")
c) What the meaning of flags is ("what does bit X mean")

In order to know such information, we would have to do parsing of quite some information inside the kernel in kdump. Basically what makedumpfile does just now. Is this feasible? I don't think so.

So we would need another approach to communicate such information as you said. I can't think of any, but if anybody reading this has an idea, please speak up. I am interested.

The *only* way right now we would have to handle such scenarios:

1. While dumping memory and we get a machine check, fake reading a zero page instead of crashing.
2. While dumping memory and we get a fault, fake reading a zero page instead of crashing.

Indeed, and the basic design is to export these flags. (let's say "unfortunately", being able to handle such stuff in kdump directly would be the dream).

Quoted 4 lines · David Hildenbrand

Sorry to say, but that is the current practice without which makedumpfile would not be able to work at all. (exclude user pages, exclude page cache, exclude buddy pages). Let's not reinvent the wheel here. This is how dumping works forever.

Sorry, but "we've always done this in the past" doesn't make it better.

Quoted 3 lines

I don't see how there should be "set of pages which do not have PG_offline".

It doesn't have to be a set of pages. Think a (mmconfig perhaps) region which the kdump kernel should completely skip because poking in it in the kdump kernel, causes all kinds of havoc like machine checks. etc. We've had and still have one issue like that.

But let me clarify my note: I don't want to be discussing with you the design of makedumpfile and how it should or should not work - that ship has already sailed. Apparently there are valid reasons to do it this way.

I was *simply* stating that it feels wrong to export mm flags like that.

But as I said already, that is mm guys' call and looking at how we're already exporting a bunch of stuff in the vmcoreinfo - including other mm flags - I guess one more flag doesn't matter anymore.

Quoted 4 lines · David Hildenbrand

Sorry to say, but that is the current practice without which makedumpfile would not be able to work at all. (exclude user pages, exclude page cache, exclude buddy pages). Let's not reinvent the wheel here. This is how dumping works forever.

Sorry, but "we've always done this in the past" doesn't make it better.

Quoted 2 lines

I don't see how there should be "set of pages which do not have PG_offline".

It doesn't have to be a set of pages. Think a (mmconfig perhaps) region which the kdump kernel should completely skip because poking in it in the kdump kernel, causes all kinds of havoc like machine checks. etc. We've had and still have one issue like that.

But let me clarify my note: I don't want to be discussing with you the design of makedumpfile and how it should or should not work - that ship has already sailed. Apparently there are valid reasons to do it this way.

I was *simply* stating that it feels wrong to export mm flags like that.

But as I said already, that is mm guys' call and looking at how we're already exporting a bunch of stuff in the vmcoreinfo - including other mm flags - I guess one more flag doesn't matter anymore.

I am not familiar with kexec to judge this particular patch but we cannot simply define any range for these pages (same as for hwpoison ones) because they can be almost anywhere in the available memory range. Then there can be countless of them. There is no other way to rule them out but to check the page state.

I am not really sure what is the concern here exactly. Kdump is so closly tight to the specific kernel version that the api exported specifically for its purpose cannot be seriously considered an ABI. Kdump has to adopt all the time.

I am not familiar with kexec to judge this particular patch but we cannot simply define any range for these pages (same as for hwpoison ones) because they can be almost anywhere in the available memory range. Then there can be countless of them. There is no other way to rule them out but to check the page state.

I am not really sure what is the concern here exactly. Kdump is so closly tight to the specific kernel version that the api exported specifically for its purpose cannot be seriously considered an ABI. Kdump has to adopt all the time.

Quoted 4 lines · David Hildenbrand

Sorry to say, but that is the current practice without which makedumpfile would not be able to work at all. (exclude user pages, exclude page cache, exclude buddy pages). Let's not reinvent the wheel here. This is how dumping works forever.

Sorry, but "we've always done this in the past" doesn't make it better.

Quoted 2 lines

I don't see how there should be "set of pages which do not have PG_offline".

It doesn't have to be a set of pages. Think a (mmconfig perhaps) region which the kdump kernel should completely skip because poking in it in the kdump kernel, causes all kinds of havoc like machine checks. etc. We've had and still have one issue like that.

But let me clarify my note: I don't want to be discussing with you the design of makedumpfile and how it should or should not work - that ship has already sailed. Apparently there are valid reasons to do it this way.

I was *simply* stating that it feels wrong to export mm flags like that.

But as I said already, that is mm guys' call and looking at how we're already exporting a bunch of stuff in the vmcoreinfo - including other mm flags - I guess one more flag doesn't matter anymore.

Sorry to say, but that is the current practice without which makedumpfile would not be able to work at all. (exclude user pages, exclude page cache, exclude buddy pages). Let's not reinvent the wheel here. This is how dumping works forever.

I don't see how there should be "set of pages which do not have PG_offline".

Quoted 4 lines · Dave Young

It would be good to copy some background info from cover letter to the patch description so that we can get better understanding why this is needed now.

Quoted 5 lines

BTW, Lianbo is working on a documentation of the vmcoreinfo exported fields. Ccing him so that he is aware of this.

Also cc Boris, although I do not want the doc changes blocks this he might have different opinion :)

Yeah, my initial reaction is that exporting an mm-internal flag to userspace is a no-no.

What would be better, IMHO, is having a general method of telling the kdump kernel - maybe ranges of physical addresses - which to skip.

Because the moment there's a set of pages which do not have PG_offline set but kdump would still like to skip, this breaks.

It would be good to copy some background info from cover letter to the patch description so that we can get better understanding why this is needed now.

BTW, Lianbo is working on a documentation of the vmcoreinfo exported fields. Ccing him so that he is aware of this.

Also cc Boris, although I do not want the doc changes blocks this he might have different opinion :)

Let's export PG_offline via PAGE_OFFLINE_MAPCOUNT_VALUE, so makedumpfile can directly skip pages that are logically offline and the content therefore stale.

Cc Andrew Morton <[email protected]>

Cc Dave Young <[email protected]>

Cc "Kirill A. Shutemov" <[email protected]>

Cc Baoquan He <[email protected]>

Cc Omar Sandoval <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc Michal Hocko <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 kernel/crash_core.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/crash_core.c +2 -0

index 933cb3e45b98..093c9f917ed0 100644
@@ -464,6 +464,8 @@ static int __init crash_save_vmcoreinfo_init(void)
 	VMCOREINFO_NUMBER(PAGE_BUDDY_MAPCOUNT_VALUE);
 #ifdef CONFIG_HUGETLB_PAGE
 	VMCOREINFO_NUMBER(HUGETLB_PAGE_DTOR);
+#define PAGE_OFFLINE_MAPCOUNT_VALUE	(~PG_offline)
+	VMCOREINFO_NUMBER(PAGE_OFFLINE_MAPCOUNT_VALUE);
 #endif
 
 	arch_crash_save_vmcoreinfo();

Signature · 1 line

2.17.2

The content of pages that are marked PG_offline is not of interest (e.g. inflated by a balloon driver), let's skip these pages.

Cc "Rafael J. Wysocki" <[email protected]>

kernel/power/snapshot.c +4 -1

index b0308a2c6000..01db1d13481a 100644
@@ -1222,7 +1222,7 @@ static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
 	BUG_ON(!PageHighMem(page));
 
 	if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page) ||
-	    PageReserved(page))
+	    PageReserved(page) || PageOffline(page))
 		return NULL;
 
 	if (page_is_guard(page))

@@ -1286,6 +1286,9 @@ static struct page *saveable_page(struct zone *zone, unsigned long pfn)
 	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
 		return NULL;
 
+	if (PageOffline(page))
+		return NULL;
+
 	if (PageReserved(page)
 	    && (!kernel_page_present(page) || pfn_is_nosave(pfn)))
 		return NULL;

The content of pages that are marked PG_offline is not of interest (e.g. inflated by a balloon driver), let's skip these pages.

Cc "Rafael J. Wysocki" <[email protected]>

kernel/power/snapshot.c +4 -1

index b0308a2c6000..01db1d13481a 100644
@@ -1222,7 +1222,7 @@ static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
 	BUG_ON(!PageHighMem(page));
 
 	if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page) ||
-	    PageReserved(page))
+	    PageReserved(page) || PageOffline(page))
 		return NULL;
 
 	if (page_is_guard(page))

@@ -1286,6 +1286,9 @@ static struct page *saveable_page(struct zone *zone, unsigned long pfn)
 	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
 		return NULL;
 
+	if (PageOffline(page))
+		return NULL;
+
 	if (PageReserved(page)
 	    && (!kernel_page_present(page) || pfn_is_nosave(pfn)))
 		return NULL;

kernel/power/snapshot.c +1 -1

index b0308a2c6000..01db1d13481a 100644
@@ -1222,7 +1222,7 @@ static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
 	BUG_ON(!PageHighMem(page));
 
 	if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page) ||
-	    PageReserved(page))
+	    PageReserved(page) || PageOffline(page))
 		return NULL;
 
 	if (page_is_guard(page))

On Wed 14-11-18 22:17:04, David Hildenbrand wrote:
[...]

Quoted 14 lines

kernel/power/snapshot.c +1 -1

index b0308a2c6000..01db1d13481a 100644
@@ -1222,7 +1222,7 @@ static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
 	BUG_ON(!PageHighMem(page));
 
 	if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page) ||
-	    PageReserved(page))
+	    PageReserved(page) || PageOffline(page))
 		return NULL;
 
 	if (page_is_guard(page))

kernel/power/snapshot.c static struct page *saveable_page(struct zone *zone, unsigned long pfn) · line 1294

Quoted 9 lines

kernel/power/snapshot.c +3 -0

@@ -1286,6 +1286,9 @@ static struct page *saveable_page(struct zone *zone, unsigned long pfn)
 	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
 		return NULL;
 
+	if (PageOffline(page))
+		return NULL;
+
 	if (PageReserved(page)
 	    && (!kernel_page_present(page) || pfn_is_nosave(pfn)))
 		return NULL;

Btw. now that you are touching this file could you also make s@pfn_to_page@pfn_to_online_page@ please? We really do not want to touch offline pfn ranges in general. A separate patch for that of course.

Thanks!

On Wed 14-11-18 22:17:04, David Hildenbrand wrote:
[...]

Quoted 14 lines

kernel/power/snapshot.c +1 -1

index b0308a2c6000..01db1d13481a 100644
@@ -1222,7 +1222,7 @@ static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
 	BUG_ON(!PageHighMem(page));
 
 	if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page) ||
-	    PageReserved(page))
+	    PageReserved(page) || PageOffline(page))
 		return NULL;
 
 	if (page_is_guard(page))

kernel/power/snapshot.c static struct page *saveable_page(struct zone *zone, unsigned long pfn) · line 1294

Quoted 9 lines

kernel/power/snapshot.c +3 -0

@@ -1286,6 +1286,9 @@ static struct page *saveable_page(struct zone *zone, unsigned long pfn)
 	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
 		return NULL;
 
+	if (PageOffline(page))
+		return NULL;
+
 	if (PageReserved(page)
 	    && (!kernel_page_present(page) || pfn_is_nosave(pfn)))
 		return NULL;

Btw. now that you are touching this file could you also make s@pfn_to_page@pfn_to_online_page@ please? We really do not want to touch offline pfn ranges in general. A separate patch for that of course.

Thanks!

kernel/power/snapshot.c +1 -1

index b0308a2c6000..01db1d13481a 100644
@@ -1222,7 +1222,7 @@ static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
 	BUG_ON(!PageHighMem(page));
 
 	if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page) ||
-	    PageReserved(page))
+	    PageReserved(page) || PageOffline(page))
 		return NULL;
 
 	if (page_is_guard(page))

Subject: [PATCH RFC 0/6] mm/kdump: allow to exclude pages that are logically offline

Right now, pages inflated as part of a balloon driver will be dumped by dump tools like makedumpfile. While XEN is able to check in the crash kernel whether a certain pfn is actuall backed by memory in the hypervisor (see xen_oldmem_pfn_is_ram) and optimize this case, dumps of virtio-balloon and hv-balloon inflated memory will essentially result in zero pages getting allocated by the hypervisor and the dump getting filled with this data.

From: David Hildenbrand
Sent: November 14, 2018 at 9:16:58 PM GMT

Quoted 10 lines

Subject: [PATCH RFC 0/6] mm/kdump: allow to exclude pages that are logically offline

Right now, pages inflated as part of a balloon driver will be dumped by dump tools like makedumpfile. While XEN is able to check in the crash kernel whether a certain pfn is actuall backed by memory in the hypervisor (see xen_oldmem_pfn_is_ram) and optimize this case, dumps of virtio-balloon and hv-balloon inflated memory will essentially result in zero pages getting allocated by the hypervisor and the dump getting filled with this data.

Is there any reason that VMware balloon driver is not mentioned?

Subject: Re: [PATCH RFC 0/6] mm/kdump: allow to exclude pages that are logically offline

Quoted 13 lines · Nadav Amit

From: David Hildenbrand
Sent: November 14, 2018 at 9:16:58 PM GMT

Quoted 10 lines

Subject: [PATCH RFC 0/6] mm/kdump: allow to exclude pages that are logically offline

Right now, pages inflated as part of a balloon driver will be dumped by dump tools like makedumpfile. While XEN is able to check in the crash kernel whether a certain pfn is actuall backed by memory in the hypervisor (see xen_oldmem_pfn_is_ram) and optimize this case, dumps of virtio-balloon and hv-balloon inflated memory will essentially result in zero pages getting allocated by the hypervisor and the dump getting filled with this data.

Quoted 1 line

Is there any reason that VMware balloon driver is not mentioned?

Definitely ...

... not ;) . I haven't looked at vmware's balloon driver yet (I only saw that there was quite some activity recently). I guess it should have similar problems. (I mean reading and dumping data nobody cares about is certainly not desired)

Can you share if something like this is also desired for vmware's implementation? (I tagged this as RFC to get some more feedback)

It should in theory be as simple as adding a handful of _SetPageOffline()/_ClearPageOffline() at the right spots.

Quoted 8 lines

From: David Hildenbrand
Sent: November 14, 2018 at 11:05:38 PM GMT
Subject: Re: [PATCH RFC 0/6] mm/kdump: allow to exclude pages that are logically offline

Can you share if something like this is also desired for vmware's implementation? (I tagged this as RFC to get some more feedback)

Quoted 2 lines

It should in theory be as simple as adding a handful of _SetPageOffline()/_ClearPageOffline() at the right spots.

Thanks, I was just suspecting it is personal ;-)

I would obviously prefer that your changes would be done on top of those that were skipped. This patch-set sounds very reasonable to me, but I prefer that Julien (cc’d) would also give his opinion.

Quoted 8 lines

From: David Hildenbrand
Sent: November 14, 2018 at 11:05:38 PM GMT
Subject: Re: [PATCH RFC 0/6] mm/kdump: allow to exclude pages that are logically offline

Can you share if something like this is also desired for vmware's implementation? (I tagged this as RFC to get some more feedback)

Quoted 2 lines

It should in theory be as simple as adding a handful of _SetPageOffline()/_ClearPageOffline() at the right spots.

Thanks, I was just suspecting it is personal ;-)

I would obviously prefer that your changes would be done on top of those that were skipped. This patch-set sounds very reasonable to me, but I prefer that Julien (cc’d) would also give his opinion.

Subject: Re: [PATCH RFC 0/6] mm/kdump: allow to exclude pages that are logically offline

Quoted 13 lines · Nadav Amit

From: David Hildenbrand
Sent: November 14, 2018 at 9:16:58 PM GMT

Quoted 10 lines

Subject: [PATCH RFC 0/6] mm/kdump: allow to exclude pages that are logically offline

Right now, pages inflated as part of a balloon driver will be dumped by dump tools like makedumpfile. While XEN is able to check in the crash kernel whether a certain pfn is actuall backed by memory in the hypervisor (see xen_oldmem_pfn_is_ram) and optimize this case, dumps of virtio-balloon and hv-balloon inflated memory will essentially result in zero pages getting allocated by the hypervisor and the dump getting filled with this data.

Quoted 1 line

Is there any reason that VMware balloon driver is not mentioned?

Definitely ...

... not ;) . I haven't looked at vmware's balloon driver yet (I only saw that there was quite some activity recently). I guess it should have similar problems. (I mean reading and dumping data nobody cares about is certainly not desired)

Can you share if something like this is also desired for vmware's implementation? (I tagged this as RFC to get some more feedback)

It should in theory be as simple as adding a handful of _SetPageOffline()/_ClearPageOffline() at the right spots.

From: David Hildenbrand
Sent: November 14, 2018 at 9:16:58 PM GMT

Quoted 10 lines

Subject: [PATCH RFC 0/6] mm/kdump: allow to exclude pages that are logically offline

Right now, pages inflated as part of a balloon driver will be dumped by dump tools like makedumpfile. While XEN is able to check in the crash kernel whether a certain pfn is actuall backed by memory in the hypervisor (see xen_oldmem_pfn_is_ram) and optimize this case, dumps of virtio-balloon and hv-balloon inflated memory will essentially result in zero pages getting allocated by the hypervisor and the dump getting filled with this data.

Is there any reason that VMware balloon driver is not mentioned?

Subject: [PATCH RFC 0/6] mm/kdump: allow to exclude pages that are logically offline

Right now, pages inflated as part of a balloon driver will be dumped by dump tools like makedumpfile. While XEN is able to check in the crash kernel whether a certain pfn is actuall backed by memory in the hypervisor (see xen_oldmem_pfn_is_ram) and optimize this case, dumps of virtio-balloon and hv-balloon inflated memory will essentially result in zero pages getting allocated by the hypervisor and the dump getting filled with this data.

Right now, pages inflated as part of a balloon driver will be dumped by dump tools like makedumpfile. While XEN is able to check in the crash kernel whether a certain pfn is actuall backed by memory in the hypervisor (see xen_oldmem_pfn_is_ram) and optimize this case, dumps of virtio-balloon and hv-balloon inflated memory will essentially result in zero pages getting allocated by the hypervisor and the dump getting filled with this data.

The allocation and reading of zero pages can directly be avoided if a dumping tool could know which pages only contain stale information not to be dumped.

Also for XEN, calling into the kernel and asking the hypervisor if a pfn is backed can be avoided if the duming tool would skip such pages right from the beginning.

Dumping tools have no idea whether a given page is part of a balloon driver and shall not be dumped. Esp. PG_reserved cannot be used for that purpose as all memory allocated during early boot is also PG_reserved, see discussion at [1]. So some other way of indication is required and a new page flag is frowned upon.

We have PG_balloon (MAPCOUNT value), which is essentially unused now. I suggest renaming it to something more generic (PG_offline) to mark pages as logically offline. This flag can than e.g. also be used by virtio-mem in the future to mark subsections as offline. Or by other code that wants to put pages logically offline (e.g. later maybe poisoned pages that shall no longer be used).

This series converts PG_balloon to PG_offline, allows dumping tools to query the value to detect such pages and marks pages in the hv-balloon and XEN balloon properly as PG_offline. Note that virtio-balloon already set pages to PG_balloon (and now PG_offline).

Please note that this is also helpful for a problem we were seeing under
Hyper-V: Dumping logically offline memory (pages kept fake offline while onlining a section via online_page_callback) would under some condicions result in a kernel panic when dumping them.

As I don't have access to neither XEN nor Hyper-V installation, this was not tested yet (and a makedumpfile change will be required to skip dumping these pages).

[1] https://lkml.org/lkml/2018/7/20/566

David Hildenbrand (6):
mm: balloon: update comment about isolation/migration/compaction
mm: convert PG_balloon to PG_offline
kexec: export PG_offline to VMCOREINFO
xen/balloon: mark inflated pages PG_offline
hv_balloon: mark inflated pages PG_offline
PM / Hibernate: exclude all PageOffline() pages

 Documentation/admin-guide/mm/pagemap.rst |  6 +++++
 drivers/hv/hv_balloon.c                  | 14 ++++++++--
 drivers/xen/balloon.c                    |  3 +++
 fs/proc/page.c                           |  4 +--
 include/linux/balloon_compaction.h       | 34 +++++++++---------------
 include/linux/page-flags.h               | 11 +++++---
 include/uapi/linux/kernel-page-flags.h   |  1 +
 kernel/crash_core.c                      |  2 ++
 kernel/power/snapshot.c                  |  5 +++-
 tools/vm/page-types.c                    |  1 +
 10 files changed, 51 insertions(+), 30 deletions(-)

Right now, pages inflated as part of a balloon driver will be dumped by dump tools like makedumpfile. While XEN is able to check in the crash kernel whether a certain pfn is actuall backed by memory in the hypervisor (see xen_oldmem_pfn_is_ram) and optimize this case, dumps of virtio-balloon and hv-balloon inflated memory will essentially result in zero pages getting allocated by the hypervisor and the dump getting filled with this data.

The allocation and reading of zero pages can directly be avoided if a dumping tool could know which pages only contain stale information not to be dumped.

Also for XEN, calling into the kernel and asking the hypervisor if a pfn is backed can be avoided if the duming tool would skip such pages right from the beginning.

Dumping tools have no idea whether a given page is part of a balloon driver and shall not be dumped. Esp. PG_reserved cannot be used for that purpose as all memory allocated during early boot is also PG_reserved, see discussion at [1]. So some other way of indication is required and a new page flag is frowned upon.

We have PG_balloon (MAPCOUNT value), which is essentially unused now. I suggest renaming it to something more generic (PG_offline) to mark pages as logically offline. This flag can than e.g. also be used by virtio-mem in the future to mark subsections as offline. Or by other code that wants to put pages logically offline (e.g. later maybe poisoned pages that shall no longer be used).

This series converts PG_balloon to PG_offline, allows dumping tools to query the value to detect such pages and marks pages in the hv-balloon and XEN balloon properly as PG_offline. Note that virtio-balloon already set pages to PG_balloon (and now PG_offline).

Please note that this is also helpful for a problem we were seeing under
Hyper-V: Dumping logically offline memory (pages kept fake offline while onlining a section via online_page_callback) would under some condicions result in a kernel panic when dumping them.

As I don't have access to neither XEN nor Hyper-V installation, this was not tested yet (and a makedumpfile change will be required to skip dumping these pages).

[1] https://lkml.org/lkml/2018/7/20/566

David Hildenbrand (6):
mm: balloon: update comment about isolation/migration/compaction
mm: convert PG_balloon to PG_offline
kexec: export PG_offline to VMCOREINFO
xen/balloon: mark inflated pages PG_offline
hv_balloon: mark inflated pages PG_offline
PM / Hibernate: exclude all PageOffline() pages

 Documentation/admin-guide/mm/pagemap.rst |  6 +++++
 drivers/hv/hv_balloon.c                  | 14 ++++++++--
 drivers/xen/balloon.c                    |  3 +++
 fs/proc/page.c                           |  4 +--
 include/linux/balloon_compaction.h       | 34 +++++++++---------------
 include/linux/page-flags.h               | 11 +++++---
 include/uapi/linux/kernel-page-flags.h   |  1 +
 kernel/crash_core.c                      |  2 ++
 kernel/power/snapshot.c                  |  5 +++-
 tools/vm/page-types.c                    |  1 +
 10 files changed, 51 insertions(+), 30 deletions(-)