linux-kernel - Re: [PATCH 3/6] mm/balloon_compaction: list interfaces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 6 Feb 2019 20:26:42 -0500
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Nadav Amit <namit@...are.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Julien Freche <jfreche@...are.com>,
        Jason Wang <jasowang@...hat.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH 3/6] mm/balloon_compaction: list interfaces

On Thu, Feb 07, 2019 at 12:43:51AM +0000, Nadav Amit wrote:
> > On Feb 6, 2019, at 4:32 PM, Michael S. Tsirkin <mst@...hat.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.
> 

No, that's not a blocker.

-- 
MST