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
| ||
|
Date: Thu, 7 Feb 2019 00:43:51 +0000 From: Nadav Amit <namit@...are.com> To: "Michael S. Tsirkin" <mst@...hat.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 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.
Powered by blists - more mailing lists