Re: [RFC][Patch v8 0/7] KVM: Guest Free Page Hinting

[Nitesh Narayan Lal]

On 2/18/19 4:04 PM, David Hildenbrand wrote:
> On 18.02.19 21:40, Nitesh Narayan Lal wrote:
>> On 2/18/19 3:31 PM, Michael S. Tsirkin wrote:
>>> On Mon, Feb 18, 2019 at 09:04:57PM +0100, David Hildenbrand wrote:
>>>>>>>>> So I'm fine with a simple implementation but the interface needs to
>>>>>>>>> allow the hypervisor to process hints in parallel while guest is
>>>>>>>>> running.  We can then fix any issues on hypervisor without breaking
>>>>>>>>> guests.
>>>>>>>> Yes, I am fine with defining an interface that theoretically let's us
>>>>>>>> change the implementation in the guest later.
>>>>>>>> I consider this even a
>>>>>>>> prerequisite. IMHO the interface shouldn't be different, it will be
>>>>>>>> exactly the same.
>>>>>>>>
>>>>>>>> It is just "who" calls the batch freeing and waits for it. And as I
>>>>>>>> outlined here, doing it without additional threads at least avoids us
>>>>>>>> for now having to think about dynamic data structures and that we can
>>>>>>>> sometimes not report "because the thread is still busy reporting or
>>>>>>>> wasn't scheduled yet".
>>>>>>> Sorry I wasn't clear. I think we need ability to change the
>>>>>>> implementation in the *host* later. IOW don't rely on
>>>>>>> host being synchronous.
>>>>>>>
>>>>>>>
>>>>>> I actually misread it :) . In any way, there has to be a mechanism to
>>>>>> synchronize.
>>>>>>
>>>>>> If we are going via a bare hypercall (like s390x, like what Alexander
>>>>>> proposes), it is going to be a synchronous interface either way. Just a
>>>>>> bare hypercall, there will not really be any blocking on the guest side.
>>>>> It bothers me that we are now tied to interface being synchronous. We
>>>>> won't be able to fix it if there's an issue as that would break guests.
>>>> I assume with "fix it" you mean "fix kfree taking longer on every X call"?
>>>>
>>>> Yes, as I initially wrote, this mimics s390x. That might be good (we
>>>> know it has been working for years) and bad (we are inheriting the same
>>>> problem class, if it exists). And being synchronous is part of the
>>>> approach for now.
>>> BTW on s390 are these hypercalls handled by Linux?
>>>
>>>> I tend to focus on the first part (we don't know anything besides it is
>>>> working) while you focus on the second part (there could be a potential
>>>> problem). Having a real problem at hand would be great, then we would
>>>> know what exactly we actually have to fix. But read below.
>>> If we end up doing a hypercall per THP, maybe we could at least
>>> not block with interrupts disabled? Poll in guest until
>>> hypervisor reports its done?  That would already be an
>>> improvement IMHO. E.g. perf within guest will point you
>>> in the right direction and towards disabling hinting.
>>>
>>>
>>>>>> Via virtio, I guess it is waiting for a response to a requests, right?
>>>>> For the buffer to be used, yes. And it could mean putting some pages
>>>>> aside until hypervisor is done with them. Then you don't need timers or
>>>>> tricks like this, you can get an interrupt and start using the memory.
>>>> I am very open to such an approach as long as we can make it work and it
>>>> is not too complicated. (-> simple)
>>>>
>>>> This would mean for example
>>>>
>>>> 1. Collect entries to be reported per VCPU in a buffer. Say magic number
>>>> 256/512.
>>>>
>>>> 2. Once the buffer is full, do crazy "take pages out of the balloon
>>>> action" and report them to the hypervisor via virtio. Let the VCPU
>>>> continue. This will require some memory to store the request. Small
>>>> hickup for the VCPU to kick of the reporting to the hypervisor.
>>>>
>>>> 3. On interrupt/response, go over the response and put the pages back to
>>>> the buddy.
>>>>
>>>> (assuming that reporting a bulk of frees is better than reporting every
>>>> single free obviously)
>>>>
>>>> This could allow nice things like "when OOM gets trigger, see if pages
>>>> are currently being reported and wait until they have been put back to
>>>> the buddy, return "new pages available", so in a real "low on memory"
>>>> scenario, no OOM killer would get involved. This could address the issue
>>>> Wei had with reporting when low on memory.
>>>>
>>>> Is that something you have in mind?
>>> Yes that seems more future proof I think.
>>>
>>>> I assume we would have to allocate
>>>> memory when crafting the new requests. This is the only reason I tend to
>>>> prefer a synchronous interface for now. But if allocation is not a
>>>> problem, great.
>>> There are two main ways to avoid allocation:
>>> 1. do not add extra data on top of each chunk passed
>> If I am not wrong then this is close to what we have right now.
> Yes, minus the kthread(s) and eventually with some sort of memory
> allocation for the request. Once you're asynchronous via a notification
> mechanisnm, there is no real need for a thread anymore, hopefully.
Whether we should go with kthread or without it, I would like to do some
performance comparison before commenting on this.
>
>> One issue I see right now is that I am polling while host is freeing the
>> memory.
>> In the next version I could tie the logic which returns pages to the
>> buddy and resets the per cpu array index value to 0 with the callback.
>> (i.e.., it happens once we receive an response from the host)
> The question is, what happens when freeing pages and the array is not
> ready to be reused yet. In that case, you want to somehow continue
> freeing pages without busy waiting or eventually not reporting pages.
This is what happens right now.
Having kthread or not should not effect this behavior.
When the array is full the current approach simply skips collecting the
free pages.
>
> The callback should put the pages back to the buddy and free the request
> eventually to have a fully asynchronous mechanism.
>
>> Other change which I am testing right now is to only capture 'MAX_ORDER
> I am not sure if this is an arbitrary number we came up with here. We
> should really play with different orders to find a hot spot. I wouldn't
> consider this high priority, though. Getting the whole concept right to
> be able to deal with any magic number we come up should be the ultimate
> goal. (stuff that only works with huge pages I consider not future
> proof, especially regarding fragmented guests which can happen easily)
Its quite possible that when we are only capturing MAX_ORDER - 1 and run
a specific workload we don't get any memory back until we re-run the
program and buddy finally starts merging of pages of order MAX_ORDER -1.
This is why I think we may make this configurable from compile time and
keep capturing MAX_ORDER - 1 so that we don't end up breaking anything.
>
-- 
Regards
Nitesh

Attachment: signature.asc
Description: OpenPGP digital signature