Linux Virtualization - Aug 2017 - [PATCH v13 4/5] mm: support reporting free page blocks

On 08/04/2017 03:53 PM, Michal Hocko wrote:
> On Fri 04-08-17 00:02:01, Michael S. Tsirkin wrote:
>> On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote:
>>> On Thursday, August 3, 2017 9:51 PM, Michal Hocko:
>>>> As I've said earlier. Start simple optimize incrementally
with some numbers to
>>>> justify a more subtle code.
>>>> --
>>> OK. Let's start with the simple implementation as you
suggested.
>>>
>>> Best,
>>> Wei
>> The tricky part is when you need to drop the lock and
>> then restart because the device is busy. Would it maybe
>> make sense to rotate the list so that new head
>> will consist of pages not yet sent to device?
> No, I this should be strictly non-modifying API.

Just get the context here for discussion:

     spin_lock_irqsave(&zone->lock, flags);
     ...
     visit(opaque2, pfn, 1<<order);
     spin_unlock_irqrestore(&zone->lock, flags);

The concern is that the callback may cause the lock be
taken too long.


I think here we can have two options:
- Option 1: Put a Note for the callback: the callback function
     should not block and it should finish as soon as possible.
     (when implementing an interrupt handler, we also have
     such similar rules in mind, right?).

For our use case, the callback just puts the reported page
block to the ring, then returns. If the ring is full as the host
is busy, then I think it should skip this one, and just return.
Because:
     A. This is an optimization feature, losing a couple of free
          pages to report isn't that important;
     B. In reality, I think it's uncommon to see this ring getting
         full (I didn't observe ring full in the tests), since the host
         (consumer) is notified to take out the page block right
         after it is added.

- Option 2: Put the callback function outside the lock
     What's input into the callback is just a pfn, and the callback
     won't access the corresponding pages. So, I still think it won't
     be an issue no matter what status of the pages is after they
     are reported (even they doesn't exit due to hot-remove).


What would you guys think?

Best,
Wei

On Fri 04-08-17 16:15:24, Wei Wang wrote:
> On 08/04/2017 03:53 PM, Michal Hocko wrote:
> >On Fri 04-08-17 00:02:01, Michael S. Tsirkin wrote:
> >>On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote:
> >>>On Thursday, August 3, 2017 9:51 PM, Michal Hocko:
> >>>>As I've said earlier. Start simple optimize
incrementally with some numbers to
> >>>>justify a more subtle code.
> >>>>--
> >>>OK. Let's start with the simple implementation as you
suggested.
> >>>
> >>>Best,
> >>>Wei
> >>The tricky part is when you need to drop the lock and
> >>then restart because the device is busy. Would it maybe
> >>make sense to rotate the list so that new head
> >>will consist of pages not yet sent to device?
> >No, I this should be strictly non-modifying API.
> 
> 
> Just get the context here for discussion:
> 
>     spin_lock_irqsave(&zone->lock, flags);
>     ...
>     visit(opaque2, pfn, 1<<order);
>     spin_unlock_irqrestore(&zone->lock, flags);
> 
> The concern is that the callback may cause the lock be
> taken too long.
> 
> 
> I think here we can have two options:
> - Option 1: Put a Note for the callback: the callback function
>     should not block and it should finish as soon as possible.
>     (when implementing an interrupt handler, we also have
>     such similar rules in mind, right?).
absolutely
> For our use case, the callback just puts the reported page
> block to the ring, then returns. If the ring is full as the host
> is busy, then I think it should skip this one, and just return.
> Because:
>     A. This is an optimization feature, losing a couple of free
>          pages to report isn't that important;
>     B. In reality, I think it's uncommon to see this ring getting
>         full (I didn't observe ring full in the tests), since the host
>         (consumer) is notified to take out the page block right
>         after it is added.
I thought you only updated a pre allocated bitmat... Anyway, I cannot
comment on this part much as I am not familiar with your usecase.
 
> - Option 2: Put the callback function outside the lock
>     What's input into the callback is just a pfn, and the callback
>     won't access the corresponding pages. So, I still think it
won't
>     be an issue no matter what status of the pages is after they
>     are reported (even they doesn't exit due to hot-remove).
This would make the API implementation more complex and I am not yet
convinced we really need that.

-- 
Michal Hocko
SUSE Labs

On 08/04/2017 04:24 PM, Michal Hocko wrote:
>
>> For our use case, the callback just puts the reported page
>> block to the ring, then returns. If the ring is full as the host
>> is busy, then I think it should skip this one, and just return.
>> Because:
>>      A. This is an optimization feature, losing a couple of free
>>           pages to report isn't that important;
>>      B. In reality, I think it's uncommon to see this ring getting
>>          full (I didn't observe ring full in the tests), since the
host
>>          (consumer) is notified to take out the page block right
>>          after it is added.
> I thought you only updated a pre allocated bitmat... Anyway, I cannot
> comment on this part much as I am not familiar with your usecase.
>   
Actually the bitmap is in the hypervisor (host). The callback puts the
(pfn,size) on a ring which is shared with the hypervisor, then the
hypervisor takes that info from the ring and updates that bitmap.


Best,
Wei