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

[David Vrabel <david.vrabel@citrix.com>]

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