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

[Bob Liu <bob.liu@oracle.com>]

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