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

[Nadav Amit <namit@vmware.com>]

> 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!