Re: [PATCH v2 1/4] mm/balloon_compaction: list interfaces

Nadav Amit
2019-04-19T22:59:05Z

> On Apr 19, 2019, at 3:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Apr 19, 2019 at 10:34:04PM +0000, Nadav Amit wrote:
>>> On Apr 19, 2019, at 3:07 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Thu, Mar 28, 2019 at 01:07:15AM +0000, Nadav Amit wrote:
>>>> Introduce interfaces for ballooning enqueueing and dequeueing of a list
>>>> of pages. These interfaces reduce the overhead of storing and restoring
>>>> IRQs by batching the operations. In addition they do not panic if the
>>>> list of pages is empty.
>>>>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>> Cc: linux-mm@kvack.org
>>>> Cc: virtualization@lists.linux-foundation.org
>>>> Reviewed-by: Xavier Deguillard <xdeguillard@vmware.com>
>>>> Signed-off-by: Nadav Amit <namit@vmware.com>
>>>> ---
>>>> include/linux/balloon_compaction.h | 4 +
>>>> mm/balloon_compaction.c | 145 +++++++++++++++++++++--------
>>>> 2 files changed, 111 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
>>>> index f111c780ef1d..1da79edadb69 100644
>>>> --- a/include/linux/balloon_compaction.h
>>>> +++ b/include/linux/balloon_compaction.h
>>>> @@ -64,6 +64,10 @@ extern struct page *balloon_page_alloc(void);
>>>> extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
>>>> struct page *page);
>>>> extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info);
>>>> +extern size_t balloon_page_list_enqueue(struct balloon_dev_info *b_dev_info,
>>>> + struct list_head *pages);
>>>> +extern size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info,
>>>> + struct list_head *pages, int n_req_pages);
>>>
>>> Why size_t I wonder? It can never be > n_req_pages which is int.
>>> Callers also seem to assume int.
>>
>> Only because on the previous iteration
>> ( https://lkml.org/lkml/2019/2/6/912 ) you said:
>>
>>> Are we sure this int never overflows? Why not just use u64
>>> or size_t straight away?
>
> And the answer is because n_req_pages is an int too?
>
>> I am ok either way, but please be consistent.
>
> I guess n_req_pages should be size_t too then?

Yes. I will change it.

>
>>>> static inline void balloon_devinfo_init(struct balloon_dev_info *balloon)
>>>> {
>>>
>>>
>>>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
>>>> index ef858d547e2d..88d5d9a01072 100644
>>>> --- a/mm/balloon_compaction.c
>>>> +++ b/mm/balloon_compaction.c
>>>> @@ -10,6 +10,106 @@
>>>> #include <linux/export.h>
>>>> #include <linux/balloon_compaction.h>
>>>>
>>>> +static int balloon_page_enqueue_one(struct balloon_dev_info *b_dev_info,
>>>> + struct page *page)
>>>> +{
>>>> + /*
>>>> + * Block others from accessing the 'page' when we get around to
>>>> + * establishing additional references. We should be the only one
>>>> + * holding a reference to the 'page' at this point.
>>>> + */
>>>> + if (!trylock_page(page)) {
>>>> + WARN_ONCE(1, "balloon inflation failed to enqueue page\n");
>>>> + return -EFAULT;
>>>
>>> Looks like all callers bug on a failure. So let's just do it here,
>>> and then make this void?
>>
>> As you noted below, actually balloon_page_list_enqueue() does not do
>> anything when an error occurs. I really prefer to avoid adding BUG_ON() -
>> I always get pushed back on such things. Yes, this might lead to memory
>> leak, but there is no reason to crash the system.
>
> Need to audit callers to make sure they don't misbehave in worse ways.
>
> I think in this case this indicates that someone is using the page so if
> one keeps going and adds it into balloon this will lead to corruption down the road.
>
> If you can change the caller code such that it's just a leak,
> then a warning is more appropriate. Or even do not warn at all.

Yes, you are right (and I was wrong) - this is indeed much more than a
memory leak. I’ll see if it is easy to handle this case (I am not sure), but
I think the warning should stay.

>>>> + }
>>>> + list_del(&page->lru);
>>>> + balloon_page_insert(b_dev_info, page);
>>>> + unlock_page(page);
>>>> + __count_vm_event(BALLOON_INFLATE);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * balloon_page_list_enqueue() - inserts a list of pages into the balloon page
>>>> + * list.
>>>> + * @b_dev_info: balloon device descriptor where we will insert a new page to
>>>> + * @pages: pages to enqueue - allocated using balloon_page_alloc.
>>>> + *
>>>> + * Driver must call it to properly enqueue a balloon pages before definitively
>>>> + * removing it from the guest system.
>>>
>>> A bunch of grammar error here. Pls fix for clarify.
>>> Also - document that nothing must lock the pages? More assumptions?
>>> What is "it" in this context? All pages? And what does removing from
>>> guest mean? Really adding to the balloon?
>>
>> I pretty much copy-pasted this description from balloon_page_enqueue(). I
>> see that you edited this message in the past at least couple of times (e.g.,
>> c7cdff0e86471 “virtio_balloon: fix deadlock on OOM”) and left it as is.
>>
>> So maybe all of the comments in this file need a rework, but I don’t think
>> this patch-set needs to do it.
>
> I see.
> That one dealt with one page so "it" was the page. This one deals with
> many pages so you can't just copy it over without changes.
> Makes it look like "it" refers to driver or guest.

I will fix “it”. ;-)

>
>>>> + *
>>>> + * Return: number of pages that were enqueued.
>>>> + */
>>>> +size_t balloon_page_list_enqueue(struct balloon_dev_info *b_dev_info,
>>>> + struct list_head *pages)
>>>> +{
>>>> + struct page *page, *tmp;
>>>> + unsigned long flags;
>>>> + size_t n_pages = 0;
>>>> +
>>>> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
>>>> + list_for_each_entry_safe(page, tmp, pages, lru) {
>>>> + balloon_page_enqueue_one(b_dev_info, page);
>>>
>>> Do we want to do something about an error here?
>>
>> Hmm… This is really something that should never happen, but I still prefer
>> to avoid BUG_ON(), as I said before. I will just not count the page.
>
> Callers can BUG then if they want. That is fine but you then
> need to change the callers to do it.

Ok, I’ll pay more attention this time. Thanks!