[PATCH] drivers: xen-selfballoon: consider slab pages

[PATCH] drivers: xen-selfballoon: consider slab pages

[Bob Liu]

Currently the goal_page in xen-selfballon doesn't consider much about pages used
in kernel space.
A typical usage is slab pages, without consider slab pages the goal_page result
may be too rough and lead extra memory pressure to guest os.

Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
 drivers/xen/xen-selfballoon.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/xen/xen-selfballoon.c b/drivers/xen/xen-selfballoon.c
index 21e18c1..4814759 100644
--- a/drivers/xen/xen-selfballoon.c
+++ b/drivers/xen/xen-selfballoon.c
@@ -191,6 +191,8 @@ static void selfballoon_process(struct work_struct *work)
 		tgt_pages = cur_pages; /* default is no change */
 		goal_pages = vm_memory_committed() +
 				totalreserve_pages +
+				global_page_state(NR_SLAB_RECLAIMABLE) +
+				global_page_state(NR_SLAB_UNRECLAIMABLE) +
 				MB2PAGES(selfballoon_reserved_mb);
 #ifdef CONFIG_FRONTSWAP
 		/* allow space for frontswap pages to be repatriated */
-- 
1.7.10.4

Re: [PATCH] drivers: xen-selfballoon: consider slab pages

[Konrad Rzeszutek Wilk]

On Mon, Nov 04, 2013 at 08:39:24PM +0800, Bob Liu wrote:
> Currently the goal_page in xen-selfballon doesn't consider much about pages used
> in kernel space.
> A typical usage is slab pages, without consider slab pages the goal_page result
> may be too rough and lead extra memory pressure to guest os.

Does  "lead extra memory pressure" mean OOM?

> 
> Signed-off-by: Bob Liu <bob.liu@oracle.com>
> ---
>  drivers/xen/xen-selfballoon.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/xen/xen-selfballoon.c b/drivers/xen/xen-selfballoon.c
> index 21e18c1..4814759 100644
> --- a/drivers/xen/xen-selfballoon.c
> +++ b/drivers/xen/xen-selfballoon.c
> @@ -191,6 +191,8 @@ static void selfballoon_process(struct work_struct *work)
>  		tgt_pages = cur_pages; /* default is no change */
>  		goal_pages = vm_memory_committed() +
>  				totalreserve_pages +
> +				global_page_state(NR_SLAB_RECLAIMABLE) +
> +				global_page_state(NR_SLAB_UNRECLAIMABLE) +
>  				MB2PAGES(selfballoon_reserved_mb);
>  #ifdef CONFIG_FRONTSWAP
>  		/* allow space for frontswap pages to be repatriated */
> -- 
> 1.7.10.4
> 

Re: [Xen-devel] [PATCH] drivers: xen-selfballoon: consider slab pages

[David Vrabel]

On 04/11/13 12:39, Bob Liu wrote:
> Currently the goal_page in xen-selfballon doesn't consider much about pages used
> in kernel space.
> A typical usage is slab pages, without consider slab pages the goal_page result
> may be too rough and lead extra memory pressure to guest os.

Can you provide some real world figures where the calculatation got it
wrong? What was the resultant behavior?  Swap death? OOM killer?

> Signed-off-by: Bob Liu <bob.liu@oracle.com>
> ---
>  drivers/xen/xen-selfballoon.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/xen/xen-selfballoon.c b/drivers/xen/xen-selfballoon.c
> index 21e18c1..4814759 100644
> --- a/drivers/xen/xen-selfballoon.c
> +++ b/drivers/xen/xen-selfballoon.c
> @@ -191,6 +191,8 @@ static void selfballoon_process(struct work_struct *work)
>  		tgt_pages = cur_pages; /* default is no change */
>  		goal_pages = vm_memory_committed() +
>  				totalreserve_pages +
> +				global_page_state(NR_SLAB_RECLAIMABLE) +

Does SLAB_RECLAIMABLE want to be included here?  Unless I'm
misunderstanding here, SLAB_RECLAIMABLE is effectively free.

> +				global_page_state(NR_SLAB_UNRECLAIMABLE) +

This bit looks fine to me.

>  				MB2PAGES(selfballoon_reserved_mb);
>  #ifdef CONFIG_FRONTSWAP
>  		/* allow space for frontswap pages to be repatriated */

David

Re: [Xen-devel] [PATCH] drivers: xen-selfballoon: consider slab pages

[Bob Liu]

On 11/05/2013 01:22 AM, David Vrabel wrote:
> On 04/11/13 12:39, Bob Liu wrote:
>> Currently the goal_page in xen-selfballon doesn't consider much about pages used
>> in kernel space.
>> A typical usage is slab pages, without consider slab pages the goal_page result
>> may be too rough and lead extra memory pressure to guest os.
> 
> Can you provide some real world figures where the calculatation got it
> wrong? What was the resultant behavior?  Swap death? OOM killer?
> 

Sorry, I didn't run any testing I just think it's unreasonable while
reading the source code.

vm_memory_committed() only calculate pages which mapped to process
address space, but the kernel itself(like block, fs and network
subsystem) may occupy some memory. And it's possible that those
subsystem may occupy a significant amount of memory in some situation.

I'm afraid if we don't consider those kernel memory while calculating
goal_pages, guest memory will be set lower than guest really needs.

>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>> ---
>>  drivers/xen/xen-selfballoon.c |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/xen/xen-selfballoon.c b/drivers/xen/xen-selfballoon.c
>> index 21e18c1..4814759 100644
>> --- a/drivers/xen/xen-selfballoon.c
>> +++ b/drivers/xen/xen-selfballoon.c
>> @@ -191,6 +191,8 @@ static void selfballoon_process(struct work_struct *work)
>>  		tgt_pages = cur_pages; /* default is no change */
>>  		goal_pages = vm_memory_committed() +
>>  				totalreserve_pages +
>> +				global_page_state(NR_SLAB_RECLAIMABLE) +
> 
> Does SLAB_RECLAIMABLE want to be included here?  Unless I'm
> misunderstanding here, SLAB_RECLAIMABLE is effectively free.
> 

SLAB_RECLAIMABLE isn't effectively free, it means the slab page is in
used but can be reclaimed(freed) during memory pressure.

>> +				global_page_state(NR_SLAB_UNRECLAIMABLE) +
> 
> This bit looks fine to me.
> 
>>  				MB2PAGES(selfballoon_reserved_mb);
>>  #ifdef CONFIG_FRONTSWAP
>>  		/* allow space for frontswap pages to be repatriated */
> 
> David
> 

Thanks for your review.

-- 
Regards,
-Bob

Re: [Xen-devel] [PATCH] drivers: xen-selfballoon: consider slab pages

[David Vrabel]

On 05/11/13 03:30, Bob Liu wrote:
> 
> On 11/05/2013 01:22 AM, David Vrabel wrote:
>> On 04/11/13 12:39, Bob Liu wrote:
>>> Currently the goal_page in xen-selfballon doesn't consider much about pages used
>>> in kernel space.
>>> A typical usage is slab pages, without consider slab pages the goal_page result
>>> may be too rough and lead extra memory pressure to guest os.
>>
>> Can you provide some real world figures where the calculatation got it
>> wrong? What was the resultant behavior?  Swap death? OOM killer?
>>
> 
> Sorry, I didn't run any testing I just think it's unreasonable while
> reading the source code.

I'm not keen on changes to heuristics without testing with real
workloads and a demonstration that it is better in practice.

> vm_memory_committed() only calculate pages which mapped to process
> address space, but the kernel itself(like block, fs and network
> subsystem) may occupy some memory. And it's possible that those
> subsystem may occupy a significant amount of memory in some situation.
> 
> I'm afraid if we don't consider those kernel memory while calculating
> goal_pages, guest memory will be set lower than guest really needs.
> 
>>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>>> ---
>>>  drivers/xen/xen-selfballoon.c |    2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/xen/xen-selfballoon.c b/drivers/xen/xen-selfballoon.c
>>> index 21e18c1..4814759 100644
>>> --- a/drivers/xen/xen-selfballoon.c
>>> +++ b/drivers/xen/xen-selfballoon.c
>>> @@ -191,6 +191,8 @@ static void selfballoon_process(struct work_struct *work)
>>>  		tgt_pages = cur_pages; /* default is no change */
>>>  		goal_pages = vm_memory_committed() +
>>>  				totalreserve_pages +
>>> +				global_page_state(NR_SLAB_RECLAIMABLE) +
>>
>> Does SLAB_RECLAIMABLE want to be included here?  Unless I'm
>> misunderstanding here, SLAB_RECLAIMABLE is effectively free.
>>
> 
> SLAB_RECLAIMABLE isn't effectively free, it means the slab page is in
> used but can be reclaimed(freed) during memory pressure.

Similarly to the pages used for the page cache, I think the intention is
for the selfballoon_reserved_mb value to include this.

>>> +				global_page_state(NR_SLAB_UNRECLAIMABLE) +
>>
>> This bit looks fine to me.
>>
>>>  				MB2PAGES(selfballoon_reserved_mb);
>>>  #ifdef CONFIG_FRONTSWAP
>>>  		/* allow space for frontswap pages to be repatriated */

David