Re: [PATCH 3/6] drm/i915: use vmap in shmem_pin_map
On Tue, Sep 22, 2020 at 04:39:06PM +0200, Christoph Hellwig wrote: > On Tue, Sep 22, 2020 at 12:21:44PM +0100, Matthew Wilcox wrote: > > Actually, vfree() will work today; I cc'd you on a documentation update > > to make it clear that this is permitted. > > vfree calls __free_pages, the i915 and a lot of other code calls > put_page. They are mostly the same, but not quite and everytime I > look into that mess I'm more confused than before. > > Can someone in the know write sensible documentation on when to use > __free_page(s) vs put_page? I started on that, and then I found a bug that's been lurking for 12 years, so that delayed the documentation somewhat. The short answer is that __free_pages() lets you free non-compound high-order pages while put_page() can only free order-0 and compound pages. I would really like to overhaul our memory allocation APIs: current new __get_free_page(s) alloc_page(s) free_page(s)free_page(s) alloc_page(s) get_free_page(s) __free_pagesput_page_order Then put_page() and put_page_order() are more obviously friends. But I cannot imagine a world in which Linus says yes to that upheaval. He's previous expressed dislike of the get_free_page() family of APIs, and thinks all those callers should just use kmalloc(). Maybe we can make that transition happen, now that kmalloc() aligns larger allocations.
Re: [PATCH 3/6] drm/i915: use vmap in shmem_pin_map
On Tue, Sep 22, 2020 at 12:21:44PM +0100, Matthew Wilcox wrote: > Actually, vfree() will work today; I cc'd you on a documentation update > to make it clear that this is permitted. vfree calls __free_pages, the i915 and a lot of other code calls put_page. They are mostly the same, but not quite and everytime I look into that mess I'm more confused than before. Can someone in the know write sensible documentation on when to use __free_page(s) vs put_page?
Re: [PATCH 3/6] drm/i915: use vmap in shmem_pin_map
On Tue, Sep 22, 2020 at 08:22:49AM +0200, Christoph Hellwig wrote: > On Mon, Sep 21, 2020 at 08:11:57PM +0100, Matthew Wilcox wrote: > > This is awkward. I'd like it if we had a vfree() variant which called > > put_page() instead of __free_pages(). I'd like it even more if we > > used release_pages() instead of our own loop that called put_page(). > > Note that we don't need a new vfree variant, we can do this manually if > we want, take a look at kernel/dma/remap.c. But I thought this code > intentionally doesn't want to do that to avoid locking in the memory > for the pages array. Maybe the i915 maintainers can clarify. Actually, vfree() will work today; I cc'd you on a documentation update to make it clear that this is permitted. >From my current experience with the i915 shmem code, I think that the i915 maintainers are experts at graphics, and are unfamiliar with the MM. There are a number of places where they do things the hard way.
Re: [PATCH 3/6] drm/i915: use vmap in shmem_pin_map
On Mon, Sep 21, 2020 at 08:11:57PM +0100, Matthew Wilcox wrote: > This is awkward. I'd like it if we had a vfree() variant which called > put_page() instead of __free_pages(). I'd like it even more if we > used release_pages() instead of our own loop that called put_page(). Note that we don't need a new vfree variant, we can do this manually if we want, take a look at kernel/dma/remap.c. But I thought this code intentionally doesn't want to do that to avoid locking in the memory for the pages array. Maybe the i915 maintainers can clarify.
Re: [PATCH 3/6] drm/i915: use vmap in shmem_pin_map
On Fri, Sep 18, 2020 at 06:37:21PM +0200, Christoph Hellwig wrote:
> void shmem_unpin_map(struct file *file, void *ptr)
> {
> + long i = shmem_npages(file);
> +
> mapping_clear_unevictable(file->f_mapping);
> - __shmem_unpin_map(file, ptr, shmem_npte(file));
> + vunmap(ptr);
> +
> + for (i = 0; i < shmem_npages(file); i++) {
> + struct page *page;
> +
> + page = shmem_read_mapping_page_gfp(file->f_mapping, i,
> +GFP_KERNEL);
> + if (!WARN_ON(IS_ERR(page))) {
> + put_page(page);
> + put_page(page);
> + }
> + }
> }
This is awkward. I'd like it if we had a vfree() variant which called
put_page() instead of __free_pages(). I'd like it even more if we
used release_pages() instead of our own loop that called put_page().
Perhaps something like this ...
+++ b/mm/vmalloc.c
@@ -2262,7 +2262,7 @@ static void __vunmap(const void *addr, int deallocate_page
s)
vm_remove_mappings(area, deallocate_pages);
- if (deallocate_pages) {
+ if (deallocate_pages == 1) {
int i;
for (i = 0; i < area->nr_pages; i++) {
@@ -2271,8 +2271,12 @@ static void __vunmap(const void *addr, int
deallocate_pages)
BUG_ON(!page);
__free_pages(page, 0);
}
- atomic_long_sub(area->nr_pages, &nr_vmalloc_pages);
+ } else if (deallocate_pages == 2) {
+ release_pages(area->pages, area->nr_pages);
+ }
+ if (deallocate_pages) {
+ atomic_long_sub(area->nr_pages, &nr_vmalloc_pages);
kvfree(area->pages);
}
@@ -2369,6 +2373,14 @@ void vunmap(const void *addr)
}
EXPORT_SYMBOL(vunmap);
+void vunmap_put_pages(const void *addr)
+{
+ BUG_ON(in_interrupt());
+ might_sleep();
+ if (addr)
+ __vunmap(addr, 2);
+}
+
/**
* vmap - map an array of pages into virtually contiguous space
* @pages: array of page pointers
only with kernel-doc and so on. I bet somebody has a better idea for a name.
