| Message ID | 20201207133024.16621-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: fix using ZONE_DEVICE memory for foreign mappings | expand |
On Mon, Dec 7, 2020 at 8:30 AM Juergen Gross <jgross@suse.com> wrote: > > Commit 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated > memory") introduced usage of ZONE_DEVICE memory for foreign memory > mappings. > > Unfortunately this collides with using page->lru for Xen backend > private page caches. > > Fix that by using page->zone_device_data instead. > > Fixes: 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated memory") > Signed-off-by: Juergen Gross <jgross@suse.com> Would it make sense to add BUG_ON(is_zone_device_page(page)) and the opposite as appropriate to cache_enq? Either way: Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
On 12/7/20 8:30 AM, Juergen Gross wrote: > --- a/drivers/xen/unpopulated-alloc.c > +++ b/drivers/xen/unpopulated-alloc.c > @@ -12,7 +12,7 @@ > #include <xen/xen.h> > > static DEFINE_MUTEX(list_lock); > -static LIST_HEAD(page_list); > +static struct page *page_list; next_page or next_allocated_page? Either way Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
On 07.12.20 21:48, Jason Andryuk wrote: > On Mon, Dec 7, 2020 at 8:30 AM Juergen Gross <jgross@suse.com> wrote: >> >> Commit 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated >> memory") introduced usage of ZONE_DEVICE memory for foreign memory >> mappings. >> >> Unfortunately this collides with using page->lru for Xen backend >> private page caches. >> >> Fix that by using page->zone_device_data instead. >> >> Fixes: 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated memory") >> Signed-off-by: Juergen Gross <jgross@suse.com> > > Would it make sense to add BUG_ON(is_zone_device_page(page)) and the > opposite as appropriate to cache_enq? No, I don't think so. At least in the CONFIG_ZONE_DEVICE case the initial list in a PV dom0 is populated from extra memory (basically the same, but not marked as zone device memory explicitly). Juergen
On Tue, Dec 08, 2020 at 07:45:00AM +0100, Jürgen Groß wrote: > On 07.12.20 21:48, Jason Andryuk wrote: > > On Mon, Dec 7, 2020 at 8:30 AM Juergen Gross <jgross@suse.com> wrote: > > > > > > Commit 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated > > > memory") introduced usage of ZONE_DEVICE memory for foreign memory > > > mappings. > > > > > > Unfortunately this collides with using page->lru for Xen backend > > > private page caches. > > > > > > Fix that by using page->zone_device_data instead. > > > > > > Fixes: 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated memory") > > > Signed-off-by: Juergen Gross <jgross@suse.com> > > > > Would it make sense to add BUG_ON(is_zone_device_page(page)) and the > > opposite as appropriate to cache_enq? > > No, I don't think so. At least in the CONFIG_ZONE_DEVICE case the > initial list in a PV dom0 is populated from extra memory (basically > the same, but not marked as zone device memory explicitly). I assume it's fine for us to then use page->zone_device_data even if the page is not explicitly marked as ZONE_DEVICE memory? If that's fine, add my: Acked-by: Roger Pau Monné <roger.pau@citrix.com> To both patches, and thank you very much for fixing this! Roger.
On 10.12.20 12:14, Roger Pau Monné wrote: > On Tue, Dec 08, 2020 at 07:45:00AM +0100, Jürgen Groß wrote: >> On 07.12.20 21:48, Jason Andryuk wrote: >>> On Mon, Dec 7, 2020 at 8:30 AM Juergen Gross <jgross@suse.com> wrote: >>>> >>>> Commit 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated >>>> memory") introduced usage of ZONE_DEVICE memory for foreign memory >>>> mappings. >>>> >>>> Unfortunately this collides with using page->lru for Xen backend >>>> private page caches. >>>> >>>> Fix that by using page->zone_device_data instead. >>>> >>>> Fixes: 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated memory") >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> >>> Would it make sense to add BUG_ON(is_zone_device_page(page)) and the >>> opposite as appropriate to cache_enq? >> >> No, I don't think so. At least in the CONFIG_ZONE_DEVICE case the >> initial list in a PV dom0 is populated from extra memory (basically >> the same, but not marked as zone device memory explicitly). > > I assume it's fine for us to then use page->zone_device_data even if > the page is not explicitly marked as ZONE_DEVICE memory? I think so, yes, as we are owner of that page and we were fine to use lru, too. > > If that's fine, add my: > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> > > To both patches, and thank you very much for fixing this! UR welcome. :-) Juergen
On Thu, Dec 10, 2020 at 6:40 AM Jürgen Groß <jgross@suse.com> wrote: > > On 10.12.20 12:14, Roger Pau Monné wrote: > > On Tue, Dec 08, 2020 at 07:45:00AM +0100, Jürgen Groß wrote: > >> On 07.12.20 21:48, Jason Andryuk wrote: > >>> On Mon, Dec 7, 2020 at 8:30 AM Juergen Gross <jgross@suse.com> wrote: > >>>> > >>>> Commit 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated > >>>> memory") introduced usage of ZONE_DEVICE memory for foreign memory > >>>> mappings. > >>>> > >>>> Unfortunately this collides with using page->lru for Xen backend > >>>> private page caches. > >>>> > >>>> Fix that by using page->zone_device_data instead. > >>>> > >>>> Fixes: 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated memory") > >>>> Signed-off-by: Juergen Gross <jgross@suse.com> > >>> > >>> Would it make sense to add BUG_ON(is_zone_device_page(page)) and the > >>> opposite as appropriate to cache_enq? > >> > >> No, I don't think so. At least in the CONFIG_ZONE_DEVICE case the > >> initial list in a PV dom0 is populated from extra memory (basically > >> the same, but not marked as zone device memory explicitly). > > > > I assume it's fine for us to then use page->zone_device_data even if > > the page is not explicitly marked as ZONE_DEVICE memory? > > I think so, yes, as we are owner of that page and we were fine to use > lru, too. I think memremap_pages or devm_memremap_pages (which calls memremap_pages) is how you mark memory as ZONE_DEVICE. i.e. they are explicitly marked. memremap_pages memmap_init_zone_device (with ZONE_DEVICE) __init_single_page set_page_links set_page_zone grep only finds a few uses of ZONE_DEVICE in the whole tree. Regards, Jason
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c index e2e42912f241..696663a439fe 100644 --- a/drivers/xen/grant-table.c +++ b/drivers/xen/grant-table.c @@ -813,10 +813,63 @@ int gnttab_alloc_pages(int nr_pages, struct page **pages) } EXPORT_SYMBOL_GPL(gnttab_alloc_pages); +#ifdef CONFIG_XEN_UNPOPULATED_ALLOC +static inline void cache_init(struct gnttab_page_cache *cache) +{ + cache->pages = NULL; +} + +static inline bool cache_empty(struct gnttab_page_cache *cache) +{ + return !cache->pages; +} + +static inline struct page *cache_deq(struct gnttab_page_cache *cache) +{ + struct page *page; + + page = cache->pages; + cache->pages = page->zone_device_data; + + return page; +} + +static inline void cache_enq(struct gnttab_page_cache *cache, struct page *page) +{ + page->zone_device_data = cache->pages; + cache->pages = page; +} +#else +static inline void cache_init(struct gnttab_page_cache *cache) +{ + INIT_LIST_HEAD(&cache->pages); +} + +static inline bool cache_empty(struct gnttab_page_cache *cache) +{ + return list_empty(&cache->pages); +} + +static inline struct page *cache_deq(struct gnttab_page_cache *cache) +{ + struct page *page; + + page = list_first_entry(&cache->pages, struct page, lru); + list_del(&page[0]->lru); + + return page; +} + +static inline void cache_enq(struct gnttab_page_cache *cache, struct page *page) +{ + list_add(&page->lru, &cache->pages); +} +#endif + void gnttab_page_cache_init(struct gnttab_page_cache *cache) { spin_lock_init(&cache->lock); - INIT_LIST_HEAD(&cache->pages); + cache_init(cache); cache->num_pages = 0; } EXPORT_SYMBOL_GPL(gnttab_page_cache_init); @@ -827,13 +880,12 @@ int gnttab_page_cache_get(struct gnttab_page_cache *cache, struct page **page) spin_lock_irqsave(&cache->lock, flags); - if (list_empty(&cache->pages)) { + if (cache_empty(cache)) { spin_unlock_irqrestore(&cache->lock, flags); return gnttab_alloc_pages(1, page); } - page[0] = list_first_entry(&cache->pages, struct page, lru); - list_del(&page[0]->lru); + page[0] = cache_deq(cache); cache->num_pages--; spin_unlock_irqrestore(&cache->lock, flags); @@ -851,7 +903,7 @@ void gnttab_page_cache_put(struct gnttab_page_cache *cache, struct page **page, spin_lock_irqsave(&cache->lock, flags); for (i = 0; i < num; i++) - list_add(&page[i]->lru, &cache->pages); + cache_enq(cache, page[i]); cache->num_pages += num; spin_unlock_irqrestore(&cache->lock, flags); @@ -867,8 +919,7 @@ void gnttab_page_cache_shrink(struct gnttab_page_cache *cache, unsigned int num) spin_lock_irqsave(&cache->lock, flags); while (cache->num_pages > num) { - page[i] = list_first_entry(&cache->pages, struct page, lru); - list_del(&page[i]->lru); + page[i] = cache_deq(cache); cache->num_pages--; if (++i == ARRAY_SIZE(page)) { spin_unlock_irqrestore(&cache->lock, flags); diff --git a/drivers/xen/unpopulated-alloc.c b/drivers/xen/unpopulated-alloc.c index 8c512ea550bb..7762c1bb23cb 100644 --- a/drivers/xen/unpopulated-alloc.c +++ b/drivers/xen/unpopulated-alloc.c @@ -12,7 +12,7 @@ #include <xen/xen.h> static DEFINE_MUTEX(list_lock); -static LIST_HEAD(page_list); +static struct page *page_list; static unsigned int list_count; static int fill_list(unsigned int nr_pages) @@ -84,7 +84,8 @@ static int fill_list(unsigned int nr_pages) struct page *pg = virt_to_page(vaddr + PAGE_SIZE * i); BUG_ON(!virt_addr_valid(vaddr + PAGE_SIZE * i)); - list_add(&pg->lru, &page_list); + pg->zone_device_data = page_list; + page_list = pg; list_count++; } @@ -118,12 +119,10 @@ int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages) } for (i = 0; i < nr_pages; i++) { - struct page *pg = list_first_entry_or_null(&page_list, - struct page, - lru); + struct page *pg = page_list; BUG_ON(!pg); - list_del(&pg->lru); + page_list = pg->zone_device_data; list_count--; pages[i] = pg; @@ -134,7 +133,8 @@ int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages) unsigned int j; for (j = 0; j <= i; j++) { - list_add(&pages[j]->lru, &page_list); + pages[j]->zone_device_data = page_list; + page_list = pages[j]; list_count++; } goto out; @@ -160,7 +160,8 @@ void xen_free_unpopulated_pages(unsigned int nr_pages, struct page **pages) mutex_lock(&list_lock); for (i = 0; i < nr_pages; i++) { - list_add(&pages[i]->lru, &page_list); + pages[i]->zone_device_data = page_list; + page_list = pages[i]; list_count++; } mutex_unlock(&list_lock); @@ -189,7 +190,8 @@ static int __init init(void) struct page *pg = pfn_to_page(xen_extra_mem[i].start_pfn + j); - list_add(&pg->lru, &page_list); + pg->zone_device_data = page_list; + page_list = pg; list_count++; } } diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h index c6ef8ffc1a09..b9c937b3a149 100644 --- a/include/xen/grant_table.h +++ b/include/xen/grant_table.h @@ -200,7 +200,11 @@ void gnttab_free_pages(int nr_pages, struct page **pages); struct gnttab_page_cache { spinlock_t lock; +#ifdef CONFIG_XEN_UNPOPULATED_ALLOC + struct page *pages; +#else struct list_head pages; +#endif unsigned int num_pages; };
Commit 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated memory") introduced usage of ZONE_DEVICE memory for foreign memory mappings. Unfortunately this collides with using page->lru for Xen backend private page caches. Fix that by using page->zone_device_data instead. Fixes: 9e2369c06c8a18 ("xen: add helpers to allocate unpopulated memory") Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/grant-table.c | 65 +++++++++++++++++++++++++++++---- drivers/xen/unpopulated-alloc.c | 20 +++++----- include/xen/grant_table.h | 4 ++ 3 files changed, 73 insertions(+), 16 deletions(-)