From: Nadav Amit <namit@vmware.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Arnd Bergmann <arnd@arndb.de>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Julien Freche <jfreche@vmware.com>,
Jason Wang <jasowang@redhat.com>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"virtualization@lists.linux-foundation.org"
<virtualization@lists.linux-foundation.org>
Subject: Re: [PATCH 3/6] mm/balloon_compaction: list interfaces
Date: Thu, 7 Feb 2019 00:43:51 +0000 [thread overview]
Message-ID: <0DFA5F3F-8358-4268-83C7-9937C5F0CFFF@vmware.com> (raw)
In-Reply-To: <20190206191936-mutt-send-email-mst@kernel.org>
> On Feb 6, 2019, at 4:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Feb 06, 2019 at 03:57:03PM -0800, 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.
>>
[Snip]
First, thanks for the quick feedback.
>> +
>> +/**
>> + * 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.
>> + */
>> +void balloon_page_list_enqueue(struct balloon_dev_info *b_dev_info,
>> + struct list_head *pages)
>> +{
>> + struct page *page, *tmp;
>> + unsigned long flags;
>> +
>> + 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);
>> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
>
> As this is scanning pages one by one anyway, it will be useful
> to have this return the # of pages enqueued.
Sure.
>
>> +}
>> +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.
>> + */
>> +int balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info,
>> + struct list_head *pages, int n_req_pages)
>
> Are we sure this int never overflows? Why not just use u64
> or size_t straight away?
size_t it is.
>
>> +{
>> + struct page *page, *tmp;
>> + unsigned long flags;
>> + int 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);
>> +
>
> This looks quite reasonable. In fact virtio can be reworked to use
> this too and then the original one can be dropped.
>
> Have the time?
Obviously not, but I am willing to make the time. What I cannot “make" is an
approval to send patches for other hypervisors. Let me run a quick check
with our FOSS people here.
Anyhow, I hope it would not prevent the patches from getting to the next
release.
next prev parent reply other threads:[~2019-02-07 0:43 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-06 23:57 [PATCH 0/6] vmw_balloon: 64-bit limit support, compaction, shrinker Nadav Amit
2019-02-06 23:57 ` [PATCH 1/6] vmw_balloon: remove the version number Nadav Amit
2019-02-06 23:57 ` [PATCH 2/6] vmw_balloon: support 64-bit memory limit Nadav Amit
2019-02-08 11:13 ` Greg Kroah-Hartman
2019-02-08 20:05 ` Nadav Amit
2019-02-06 23:57 ` [PATCH 3/6] mm/balloon_compaction: list interfaces Nadav Amit
2019-02-07 0:32 ` Michael S. Tsirkin
2019-02-07 0:43 ` Nadav Amit [this message]
2019-02-07 1:26 ` Michael S. Tsirkin
2019-02-06 23:57 ` [PATCH 4/6] vmw_balloon: compaction support Nadav Amit
2019-02-06 23:57 ` [PATCH 5/6] vmw_balloon: add memory shrinker Nadav Amit
2019-02-06 23:57 ` [PATCH 6/6] vmw_balloon: split refused pages Nadav Amit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0DFA5F3F-8358-4268-83C7-9937C5F0CFFF@vmware.com \
--to=namit@vmware.com \
--cc=arnd@arndb.de \
--cc=gregkh@linuxfoundation.org \
--cc=jasowang@redhat.com \
--cc=jfreche@vmware.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mst@redhat.com \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).