Re: [PATCH v13 4/5] mm: support reporting free page blocks

[Michal Hocko]

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