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

[Nadav Amit]

> On Apr 19, 2019, at 3:07 PM, Michael S. Tsirkin <mst@xxxxxxxxxx> 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@xxxxxxxxxx>
>> Cc: Jason Wang <jasowang@xxxxxxxxxx>
>> Cc: linux-mm@xxxxxxxxx
>> Cc: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
>> Reviewed-by: Xavier Deguillard <xdeguillard@xxxxxxxxxx>
>> Signed-off-by: Nadav Amit <namit@xxxxxxxxxx>
>> ---
>> 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?

I am ok either way, but please be consistent.

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

>> +	}
>> +	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.

>> + *
>> + * 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.

> 
>> +		n_pages++;
>> +	}
>> +	spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
>> +	return n_pages;
>> +}
>> +EXPORT_SYMBOL_GPL(balloon_page_list_enqueue);
>> +
>> +/**
>> + * balloon_page_list_dequeue() - removes pages from balloon's page list and
>> + *				 returns a list of the pages.
>> + * @b_dev_info: balloon device decriptor where we will grab a page from.
>> + * @pages: pointer to the list of pages that would be returned to the caller.
>> + * @n_req_pages: number of requested pages.
>> + *
>> + * Driver must call it to properly de-allocate a previous enlisted balloon pages
>> + * before definetively releasing it back to the guest system. This function
>> + * tries to remove @n_req_pages from the ballooned pages and return it to the
>> + * caller in the @pages list.
>> + *
>> + * Note that this function may fail to dequeue some pages temporarily empty due
>> + * to compaction isolated pages.
>> + *
>> + * Return: number of pages that were added to the @pages list.
>> + */
>> +size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info,
>> +				 struct list_head *pages, int n_req_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, &b_dev_info->pages, lru) {
>> +		/*
>> +		 * Block others from accessing the 'page' while we get around
>> +		 * establishing additional references and preparing the 'page'
>> +		 * to be released by the balloon driver.
>> +		 */
>> +		if (!trylock_page(page))
>> +			continue;
>> +
>> +		if (IS_ENABLED(CONFIG_BALLOON_COMPACTION) &&
>> +		    PageIsolated(page)) {
>> +			/* raced with isolation */
>> +			unlock_page(page);
>> +			continue;
>> +		}
>> +		balloon_page_delete(page);
>> +		__count_vm_event(BALLOON_DEFLATE);
>> +		unlock_page(page);
>> +		list_add(&page->lru, pages);
>> +		if (++n_pages >= n_req_pages)
>> +			break;
>> +	}
>> +	spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
>> +
>> +	return n_pages;
>> +}
>> +EXPORT_SYMBOL_GPL(balloon_page_list_dequeue);
>> +
>> /*
>>  * balloon_page_alloc - allocates a new page for insertion into the balloon
>>  *			  page list.
>> @@ -43,17 +143,9 @@ void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
>> {
>> 	unsigned long flags;
>> 
>> -	/*
>> -	 * 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.
>> -	 */
>> -	BUG_ON(!trylock_page(page));
>> 	spin_lock_irqsave(&b_dev_info->pages_lock, flags);
>> -	balloon_page_insert(b_dev_info, page);
>> -	__count_vm_event(BALLOON_INFLATE);
>> +	balloon_page_enqueue_one(b_dev_info, page);
> 
> We used to bug on failure to lock page, now we
> silently ignore this error. Why?

Thatâs a mistake. Iâll add a BUG_ON() if balloon_page_enqueue_one() fails.