2018-11-19 10:18:15

by David Hildenbrand

Subject: [PATCH v1 5/8] hv_balloon: mark inflated pages PG_offline

Mark inflated and never onlined pages PG_offline, to tell the world that
the content is stale and should not be dumped.

Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: Kairui Song <[email protected]>
Cc: Vitaly Kuznetsov <[email protected]>
Cc: Andrew Morton <[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]>
---
drivers/hv/hv_balloon.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 211f3fe3a038..47719862e57f 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -681,8 +681,13 @@ static struct notifier_block hv_memory_nb = {
/* Check if the particular page is backed and can be onlined and online it. */
static void hv_page_online_one(struct hv_hotadd_state *has, struct page *pg)
{
- if (!has_pfn_is_backed(has, page_to_pfn(pg)))
+ if (!has_pfn_is_backed(has, page_to_pfn(pg))) {
+ if (!PageOffline(pg))
+ __SetPageOffline(pg);
return;
+ }
+ if (PageOffline(pg))
+ __ClearPageOffline(pg);

/* This frame is currently backed; online the page. */
__online_page_set_limits(pg);
@@ -1201,6 +1206,7 @@ static void free_balloon_pages(struct hv_dynmem_device *dm,

for (i = 0; i < num_pages; i++) {
pg = pfn_to_page(i + start_frame);
+ __ClearPageOffline(pg);
__free_page(pg);
dm->num_pages_ballooned--;
}
@@ -1213,7 +1219,7 @@ static unsigned int alloc_balloon_pages(struct hv_dynmem_device *dm,
struct dm_balloon_response *bl_resp,
int alloc_unit)
{
- unsigned int i = 0;
+ unsigned int i, j;
struct page *pg;

if (num_pages < alloc_unit)
@@ -1245,6 +1251,10 @@ static unsigned int alloc_balloon_pages(struct hv_dynmem_device *dm,
if (alloc_unit != 1)
split_page(pg, get_order(alloc_unit << PAGE_SHIFT));

+ /* mark all pages offline */
+ for (j = 0; j < (1 << get_order(alloc_unit << PAGE_SHIFT)); j++)
+ __SetPageOffline(pg + j);
+
bl_resp->range_count++;
bl_resp->range_array[i].finfo.start_page =
page_to_pfn(pg);
--
2.17.2

2018-11-20 08:46:41

by Pankaj Gupta

Subject: Re: [PATCH v1 5/8] hv_balloon: mark inflated pages PG_offline

Hi David,

>
> Mark inflated and never onlined pages PG_offline, to tell the world that
> the content is stale and should not be dumped.
>
> Cc: "K. Y. Srinivasan" <[email protected]>
> Cc: Haiyang Zhang <[email protected]>
> Cc: Stephen Hemminger <[email protected]>
> Cc: Kairui Song <[email protected]>
> Cc: Vitaly Kuznetsov <[email protected]>
> Cc: Andrew Morton <[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]>
> ---
> drivers/hv/hv_balloon.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 211f3fe3a038..47719862e57f 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -681,8 +681,13 @@ static struct notifier_block hv_memory_nb = {
> /* Check if the particular page is backed and can be onlined and online it.
> */
> static void hv_page_online_one(struct hv_hotadd_state *has, struct page *pg)
> {
> - if (!has_pfn_is_backed(has, page_to_pfn(pg)))
> + if (!has_pfn_is_backed(has, page_to_pfn(pg))) {
> + if (!PageOffline(pg))
> + __SetPageOffline(pg);
> return;
> + }
> + if (PageOffline(pg))
> + __ClearPageOffline(pg);
>
> /* This frame is currently backed; online the page. */
> __online_page_set_limits(pg);
> @@ -1201,6 +1206,7 @@ static void free_balloon_pages(struct hv_dynmem_device
> *dm,
>
> for (i = 0; i < num_pages; i++) {
> pg = pfn_to_page(i + start_frame);
> + __ClearPageOffline(pg);

Just thinking, do we need to care for clearing PageOffline flag before freeing
a balloon'd page?

Thanks,
Pankaj

> __free_page(pg);
> dm->num_pages_ballooned--;
> }
> @@ -1213,7 +1219,7 @@ static unsigned int alloc_balloon_pages(struct
> hv_dynmem_device *dm,
> struct dm_balloon_response *bl_resp,
> int alloc_unit)
> {
> - unsigned int i = 0;
> + unsigned int i, j;
> struct page *pg;
>
> if (num_pages < alloc_unit)
> @@ -1245,6 +1251,10 @@ static unsigned int alloc_balloon_pages(struct
> hv_dynmem_device *dm,
> if (alloc_unit != 1)
> split_page(pg, get_order(alloc_unit << PAGE_SHIFT));
>
> + /* mark all pages offline */
> + for (j = 0; j < (1 << get_order(alloc_unit << PAGE_SHIFT)); j++)
> + __SetPageOffline(pg + j);
> +
> bl_resp->range_count++;
> bl_resp->range_array[i].finfo.start_page =
> page_to_pfn(pg);
> --
> 2.17.2
>
>

2018-11-20 12:34:21

by David Hildenbrand

Subject: Re: [PATCH v1 5/8] hv_balloon: mark inflated pages PG_offline

On 20.11.18 09:45, Pankaj Gupta wrote:
>
> Hi David,
>
>>
>> Mark inflated and never onlined pages PG_offline, to tell the world that
>> the content is stale and should not be dumped.
>>
>> Cc: "K. Y. Srinivasan" <[email protected]>
>> Cc: Haiyang Zhang <[email protected]>
>> Cc: Stephen Hemminger <[email protected]>
>> Cc: Kairui Song <[email protected]>
>> Cc: Vitaly Kuznetsov <[email protected]>
>> Cc: Andrew Morton <[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]>
>> ---
>> drivers/hv/hv_balloon.c | 14 ++++++++++++--
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
>> index 211f3fe3a038..47719862e57f 100644
>> --- a/drivers/hv/hv_balloon.c
>> +++ b/drivers/hv/hv_balloon.c
>> @@ -681,8 +681,13 @@ static struct notifier_block hv_memory_nb = {
>> /* Check if the particular page is backed and can be onlined and online it.
>> */
>> static void hv_page_online_one(struct hv_hotadd_state *has, struct page *pg)
>> {
>> - if (!has_pfn_is_backed(has, page_to_pfn(pg)))
>> + if (!has_pfn_is_backed(has, page_to_pfn(pg))) {
>> + if (!PageOffline(pg))
>> + __SetPageOffline(pg);
>> return;
>> + }
>> + if (PageOffline(pg))
>> + __ClearPageOffline(pg);
>>
>> /* This frame is currently backed; online the page. */
>> __online_page_set_limits(pg);
>> @@ -1201,6 +1206,7 @@ static void free_balloon_pages(struct hv_dynmem_device
>> *dm,
>>
>> for (i = 0; i < num_pages; i++) {
>> pg = pfn_to_page(i + start_frame);
>> + __ClearPageOffline(pg);
>
> Just thinking, do we need to care for clearing PageOffline flag before freeing
> a balloon'd page?

Yes we have to otherwise the code will crash when trying to set PageBuddy.

(only one page type at a time may be set right now, and it makes sense.
A page that is offline cannot e.g. be a buddy page)

So PageOffline is completely managed by the page owner.

>
> Thanks,
> Pankaj


--

Thanks,

David / dhildenb

2018-11-20 14:35:15

by Pankaj Gupta

Subject: Re: [PATCH v1 5/8] hv_balloon: mark inflated pages PG_offline

> >> ---
> >> drivers/hv/hv_balloon.c | 14 ++++++++++++--
> >> 1 file changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> >> index 211f3fe3a038..47719862e57f 100644
> >> --- a/drivers/hv/hv_balloon.c
> >> +++ b/drivers/hv/hv_balloon.c
> >> @@ -681,8 +681,13 @@ static struct notifier_block hv_memory_nb = {
> >> /* Check if the particular page is backed and can be onlined and online
> >> it.
> >> */
> >> static void hv_page_online_one(struct hv_hotadd_state *has, struct page
> >> *pg)
> >> {
> >> - if (!has_pfn_is_backed(has, page_to_pfn(pg)))
> >> + if (!has_pfn_is_backed(has, page_to_pfn(pg))) {
> >> + if (!PageOffline(pg))
> >> + __SetPageOffline(pg);
> >> return;
> >> + }
> >> + if (PageOffline(pg))
> >> + __ClearPageOffline(pg);
> >>
> >> /* This frame is currently backed; online the page. */
> >> __online_page_set_limits(pg);
> >> @@ -1201,6 +1206,7 @@ static void free_balloon_pages(struct
> >> hv_dynmem_device
> >> *dm,
> >>
> >> for (i = 0; i < num_pages; i++) {
> >> pg = pfn_to_page(i + start_frame);
> >> + __ClearPageOffline(pg);
> >
> > Just thinking, do we need to care for clearing PageOffline flag before
> > freeing
> > a balloon'd page?
>
> Yes we have to otherwise the code will crash when trying to set PageBuddy.
>
> (only one page type at a time may be set right now, and it makes sense.
> A page that is offline cannot e.g. be a buddy page)

o.k
>
> So PageOffline is completely managed by the page owner.

Makes sense. Thanks for explaining.

Pankaj
>
> >
> > Thanks,
> > Pankaj
>
>
> --
>
> Thanks,
>
> David / dhildenb
>

2018-11-21 01:11:22

by Pankaj Gupta

Subject: Re: [PATCH v1 5/8] hv_balloon: mark inflated pages PG_offline

> > >> ---
> > >> drivers/hv/hv_balloon.c | 14 ++++++++++++--
> > >> 1 file changed, 12 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> > >> index 211f3fe3a038..47719862e57f 100644
> > >> --- a/drivers/hv/hv_balloon.c
> > >> +++ b/drivers/hv/hv_balloon.c
> > >> @@ -681,8 +681,13 @@ static struct notifier_block hv_memory_nb = {
> > >> /* Check if the particular page is backed and can be onlined and online
> > >> it.
> > >> */
> > >> static void hv_page_online_one(struct hv_hotadd_state *has, struct page
> > >> *pg)
> > >> {
> > >> - if (!has_pfn_is_backed(has, page_to_pfn(pg)))
> > >> + if (!has_pfn_is_backed(has, page_to_pfn(pg))) {
> > >> + if (!PageOffline(pg))
> > >> + __SetPageOffline(pg);
> > >> return;
> > >> + }
> > >> + if (PageOffline(pg))
> > >> + __ClearPageOffline(pg);
> > >>
> > >> /* This frame is currently backed; online the page. */
> > >> __online_page_set_limits(pg);
> > >> @@ -1201,6 +1206,7 @@ static void free_balloon_pages(struct
> > >> hv_dynmem_device
> > >> *dm,
> > >>
> > >> for (i = 0; i < num_pages; i++) {
> > >> pg = pfn_to_page(i + start_frame);
> > >> + __ClearPageOffline(pg);
> > >
> > > Just thinking, do we need to care for clearing PageOffline flag before
> > > freeing
> > > a balloon'd page?
> >
> > Yes we have to otherwise the code will crash when trying to set PageBuddy.
> >
> > (only one page type at a time may be set right now, and it makes sense.
> > A page that is offline cannot e.g. be a buddy page)
>
> o.k
> >
> > So PageOffline is completely managed by the page owner.
>
> Makes sense. Thanks for explaining.

Looks good to me.

Acked-by: Pankaj gupta <[email protected]>