Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
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>,
        Julien Freche <jfreche@vmware.com>,
        Pv-drivers <Pv-drivers@vmware.com>,
        Jason Wang <jasowang@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "virtualization@lists.linux-foundation.org" 
        <virtualization@lists.linux-foundation.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>
Subject: Re: [PATCH v3 1/4] mm/balloon_compaction: list interfaces
Thread-Topic: [PATCH v3 1/4] mm/balloon_compaction: list interfaces
Thread-Index: AQHU+mwdinHS3UiOtEKcnmatxdvta6ZLVFgAgAHrSAA=
Date:   Thu, 25 Apr 2019 19:08:17 +0000
Message-ID: <77127A20-8B02-4C1F-A746-6378A160D12B@vmware.com>
References: <20190423234531.29371-1-namit@vmware.com>
 <20190423234531.29371-2-namit@vmware.com>
 <20190424092829-mutt-send-email-mst@kernel.org>
In-Reply-To: <20190424092829-mutt-send-email-mst@kernel.org>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: vmware.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-ID: <6DCBC4D19BB59F4481A64E7CC85E0804@namprd05.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 77370d01-82c2-4c89-81c8-08d6c9b16023
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Apr 2019 19:08:17.6759
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR05MB4662
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> On Apr 24, 2019, at 6:49 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>=20
> On Tue, Apr 23, 2019 at 04:45:28PM -0700, 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.
>>=20
>> 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>
>=20
>=20
> Looks good overall. Two minor comments below.
>=20
>=20
>> ---
>> include/linux/balloon_compaction.h |   4 +
>> mm/balloon_compaction.c            | 144 +++++++++++++++++++++--------
>> 2 files changed, 110 insertions(+), 38 deletions(-)
>>=20
>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_=
compaction.h
>> index f111c780ef1d..430b6047cef7 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, size_t n_req_pages);
>>=20
>> 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..a2995002edc2 100644
>> --- a/mm/balloon_compaction.c
>> +++ b/mm/balloon_compaction.c
>> @@ -10,6 +10,105 @@
>> #include <linux/export.h>
>> #include <linux/balloon_compaction.h>
>>=20
>> +static void balloon_page_enqueue_one(struct balloon_dev_info *b_dev_inf=
o,
>> +				     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 we are not, the=
n
>> +	 * memory corruption is possible and we should stop execution.
>> +	 */
>> +	BUG_ON(!trylock_page(page));
>> +	list_del(&page->lru);
>> +	balloon_page_insert(b_dev_info, page);
>> +	unlock_page(page);
>> +	__count_vm_event(BALLOON_INFLATE);
>> +}
>> +
>> +/**
>> + * balloon_page_list_enqueue() - inserts a list of pages into the ballo=
on page
>> + *				 list.
>> + * @b_dev_info: balloon device descriptor where we will insert a new pa=
ge to
>> + * @pages: pages to enqueue - allocated using balloon_page_alloc.
>> + *
>> + * Driver must call it to properly enqueue a balloon pages before defin=
itively
>> + * removing it from the guest system.
>> + *
>> + * 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 =3D 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);
>> +		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 c=
aller.
>> + * @n_req_pages: number of requested pages.
>> + *
>> + * Driver must call this function to properly de-allocate a previous en=
listed
>> + * balloon pages before definetively releasing it back to the guest sys=
tem.
>> + * This function tries to remove @n_req_pages from the ballooned pages =
and
>> + * return them to the caller in the @pages list.
>> + *
>> + * Note that this function may fail to dequeue some pages temporarily e=
mpty 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, size_t n_req_pages)
>> +{
>> +	struct page *page, *tmp;
>> +	unsigned long flags;
>> +	size_t n_pages =3D 0;
>> +
>> +	spin_lock_irqsave(&b_dev_info->pages_lock, flags);
>> +	list_for_each_entry_safe(page, tmp, &b_dev_info->pages, lru) {
>> +		if (n_pages =3D=3D n_req_pages)
>> +			break;
>> +
>> +		/*
>> +		 * Block others from accessing the 'page' while we get around
>=20
> should be "get around to" - same as in other places
>=20
>=20
>> +		 * 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);
>=20
> I'm not sure whether this list_add must be under the page lock,
> but enqueue does list_del under page lock, so I think it's
> a good idea to keep dequeue consistent, operating in the
> reverse order of enqueue.
>=20
>> +		++n_pages;
>> +	}
>> +	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 ball=
oon
>>  *			  page list.
>> @@ -43,17 +142,9 @@ void balloon_page_enqueue(struct balloon_dev_info *b=
_dev_info,
>> {
>> 	unsigned long flags;
>>=20
>> -	/*
>> -	 * 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);
>> 	spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
>> -	unlock_page(page);
>> }
>> EXPORT_SYMBOL_GPL(balloon_page_enqueue);
>>=20
>> @@ -70,36 +161,13 @@ EXPORT_SYMBOL_GPL(balloon_page_enqueue);
>>  */
>> struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info)
>> {
>> -	struct page *page, *tmp;
>> 	unsigned long flags;
>> -	bool dequeued_page;
>> +	LIST_HEAD(pages);
>> +	int n_pages;
>>=20
>> -	dequeued_page =3D false;
>> -	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)) {
>> -#ifdef CONFIG_BALLOON_COMPACTION
>> -			if (PageIsolated(page)) {
>> -				/* raced with isolation */
>> -				unlock_page(page);
>> -				continue;
>> -			}
>> -#endif
>> -			balloon_page_delete(page);
>> -			__count_vm_event(BALLOON_DEFLATE);
>> -			unlock_page(page);
>> -			dequeued_page =3D true;
>> -			break;
>> -		}
>> -	}
>> -	spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
>> +	n_pages =3D balloon_page_list_dequeue(b_dev_info, &pages, 1);
>>=20
>> -	if (!dequeued_page) {
>> +	if (n_pages !=3D 1) {
>> 		/*
>> 		 * If we are unable to dequeue a balloon page because the page
>> 		 * list is empty and there is no isolated pages, then something
>> @@ -112,9 +180,9 @@ struct page *balloon_page_dequeue(struct balloon_dev=
_info *b_dev_info)
>> 			     !b_dev_info->isolated_pages))
>> 			BUG();
>> 		spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
>> -		page =3D NULL;
>> +		return NULL;
>> 	}
>> -	return page;
>> +	return list_first_entry(&pages, struct page, lru);
>> }
>> EXPORT_SYMBOL_GPL(balloon_page_dequeue);
>>=20
>> --=20
>> 2.19.1
>=20
>=20
> With above addressed:
>=20
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thank you, Michael.

I addressed your feedback and I will send another version shortly.