[alsa-devel] [PATCH 0/2] Replace with alloc_pages_exact(), take#2
Hi,
this is a renewed attempt to replace __get_free_pages() usages with alloc_pages_exact() in sound tree. At this time, the incorrectly leftover of __GFP_COMP flag is removed altogether.
thanks,
Takashi
===
Takashi Iwai (2): ALSA: Replace snd_malloc_pages() and snd_free_pages() with standard helpers, take#2 ALSA: us122l: Use alloc_pages_exact()
include/sound/memalloc.h | 4 ---- sound/core/memalloc.c | 53 +++-------------------------------------- sound/core/pcm.c | 14 +++++------ sound/usb/usx2y/usX2Yhwdep.c | 3 ++- sound/usb/usx2y/usb_stream.c | 20 +++++++--------- sound/usb/usx2y/usbusx2y.c | 3 ++- sound/usb/usx2y/usx2yhwdeppcm.c | 6 +++-- 7 files changed, 26 insertions(+), 77 deletions(-)
snd_malloc_pages() and snd_free_pages() are merely thin wrappers of the standard page allocator / free functions. Even the arguments are compatible with some standard helpers, so there is little merit of keeping these wrappers.
This patch replaces the all existing callers of snd_malloc_pages() and snd_free_pages() with the direct calls of the standard helper functions. In this version, we use a recently introduced one, alloc_pages_exact(), which suits better than the old snd_malloc_pages() implementation for our purposes. Then we can avoid the waste of pages by alignment to power-of-two.
Since alloc_pages_exact() does split pages, we need no longer __GFP_COMP flag; or better to say, we must not pass __GFP_COMP to alloc_pages_exact(). So the former unconditional addition of __GFP_COMP flag in snd_malloc_pages() is dropped, as well as in most other places.
Reviewed-by: Takashi Sakamoto o-takashi@sakamocchi.jp Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/memalloc.h | 4 ---- sound/core/memalloc.c | 53 +++-------------------------------------- sound/core/pcm.c | 14 +++++------ sound/usb/usx2y/usX2Yhwdep.c | 3 ++- sound/usb/usx2y/usbusx2y.c | 3 ++- sound/usb/usx2y/usx2yhwdeppcm.c | 6 +++-- 6 files changed, 18 insertions(+), 65 deletions(-)
diff --git a/include/sound/memalloc.h b/include/sound/memalloc.h index 1ac0dd82a916..4c6f3b5a7cff 100644 --- a/include/sound/memalloc.h +++ b/include/sound/memalloc.h @@ -151,9 +151,5 @@ int snd_dma_alloc_pages_fallback(int type, struct device *dev, size_t size, struct snd_dma_buffer *dmab); void snd_dma_free_pages(struct snd_dma_buffer *dmab);
-/* basic memory allocation functions */ -void *snd_malloc_pages(size_t size, gfp_t gfp_flags); -void snd_free_pages(void *ptr, size_t size); - #endif /* __SOUND_MEMALLOC_H */
diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c index eb974235c92b..9f48e1d3a257 100644 --- a/sound/core/memalloc.c +++ b/sound/core/memalloc.c @@ -30,53 +30,6 @@ #endif #include <sound/memalloc.h>
-/* - * - * Generic memory allocators - * - */ - -/** - * snd_malloc_pages - allocate pages with the given size - * @size: the size to allocate in bytes - * @gfp_flags: the allocation conditions, GFP_XXX - * - * Allocates the physically contiguous pages with the given size. - * - * Return: The pointer of the buffer, or %NULL if no enough memory. - */ -void *snd_malloc_pages(size_t size, gfp_t gfp_flags) -{ - int pg; - - if (WARN_ON(!size)) - return NULL; - if (WARN_ON(!gfp_flags)) - return NULL; - gfp_flags |= __GFP_COMP; /* compound page lets parts be mapped */ - pg = get_order(size); - return (void *) __get_free_pages(gfp_flags, pg); -} -EXPORT_SYMBOL(snd_malloc_pages); - -/** - * snd_free_pages - release the pages - * @ptr: the buffer pointer to release - * @size: the allocated buffer size - * - * Releases the buffer allocated via snd_malloc_pages(). - */ -void snd_free_pages(void *ptr, size_t size) -{ - int pg; - - if (ptr == NULL) - return; - pg = get_order(size); - free_pages((unsigned long) ptr, pg); -} -EXPORT_SYMBOL(snd_free_pages); - /* * * Bus-specific memory allocators @@ -190,8 +143,8 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size, dmab->bytes = 0; switch (type) { case SNDRV_DMA_TYPE_CONTINUOUS: - dmab->area = snd_malloc_pages(size, - (__force gfp_t)(unsigned long)device); + dmab->area = alloc_pages_exact(size, + (__force gfp_t)(unsigned long)device); dmab->addr = 0; break; #ifdef CONFIG_HAS_DMA @@ -275,7 +228,7 @@ void snd_dma_free_pages(struct snd_dma_buffer *dmab) { switch (dmab->dev.type) { case SNDRV_DMA_TYPE_CONTINUOUS: - snd_free_pages(dmab->area, dmab->bytes); + free_pages_exact(dmab->area, dmab->bytes); break; #ifdef CONFIG_HAS_DMA #ifdef CONFIG_GENERIC_ALLOCATOR diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 7b63aee124af..998e477522fd 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -959,22 +959,22 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream, return -ENOMEM;
size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status)); - runtime->status = snd_malloc_pages(size, GFP_KERNEL); + runtime->status = alloc_pages_exact(size, GFP_KERNEL); if (runtime->status == NULL) { kfree(runtime); return -ENOMEM; } - memset((void*)runtime->status, 0, size); + memset(runtime->status, 0, size);
size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_control)); - runtime->control = snd_malloc_pages(size, GFP_KERNEL); + runtime->control = alloc_pages_exact(size, GFP_KERNEL); if (runtime->control == NULL) { - snd_free_pages((void*)runtime->status, + free_pages_exact(runtime->status, PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status))); kfree(runtime); return -ENOMEM; } - memset((void*)runtime->control, 0, size); + memset(runtime->control, 0, size);
init_waitqueue_head(&runtime->sleep); init_waitqueue_head(&runtime->tsleep); @@ -1000,9 +1000,9 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream) runtime = substream->runtime; if (runtime->private_free != NULL) runtime->private_free(runtime); - snd_free_pages((void*)runtime->status, + free_pages_exact(runtime->status, PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status))); - snd_free_pages((void*)runtime->control, + free_pages_exact(runtime->control, PAGE_ALIGN(sizeof(struct snd_pcm_mmap_control))); kfree(runtime->hw_constraints.rules); /* Avoid concurrent access to runtime via PCM timer interface */ diff --git a/sound/usb/usx2y/usX2Yhwdep.c b/sound/usb/usx2y/usX2Yhwdep.c index c1dd9a7b48df..bfe1108416cf 100644 --- a/sound/usb/usx2y/usX2Yhwdep.c +++ b/sound/usb/usx2y/usX2Yhwdep.c @@ -75,7 +75,8 @@ static int snd_us428ctls_mmap(struct snd_hwdep * hw, struct file *filp, struct v
if (!us428->us428ctls_sharedmem) { init_waitqueue_head(&us428->us428ctls_wait_queue_head); - if(!(us428->us428ctls_sharedmem = snd_malloc_pages(sizeof(struct us428ctls_sharedmem), GFP_KERNEL))) + us428->us428ctls_sharedmem = alloc_pages_exact(sizeof(struct us428ctls_sharedmem), GFP_KERNEL); + if (!us428->us428ctls_sharedmem) return -ENOMEM; memset(us428->us428ctls_sharedmem, -1, sizeof(struct us428ctls_sharedmem)); us428->us428ctls_sharedmem->CtlSnapShotLast = -2; diff --git a/sound/usb/usx2y/usbusx2y.c b/sound/usb/usx2y/usbusx2y.c index da4a5a541512..9f7bbed2c0f0 100644 --- a/sound/usb/usx2y/usbusx2y.c +++ b/sound/usb/usx2y/usbusx2y.c @@ -437,7 +437,8 @@ static void snd_usX2Y_card_private_free(struct snd_card *card) kfree(usX2Y(card)->In04Buf); usb_free_urb(usX2Y(card)->In04urb); if (usX2Y(card)->us428ctls_sharedmem) - snd_free_pages(usX2Y(card)->us428ctls_sharedmem, sizeof(*usX2Y(card)->us428ctls_sharedmem)); + free_pages_exact(usX2Y(card)->us428ctls_sharedmem, + sizeof(*usX2Y(card)->us428ctls_sharedmem)); if (usX2Y(card)->card_index >= 0 && usX2Y(card)->card_index < SNDRV_CARDS) snd_usX2Y_card_used[usX2Y(card)->card_index] = 0; } diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c b/sound/usb/usx2y/usx2yhwdeppcm.c index 714cf50d4a4c..ace8185c3f6d 100644 --- a/sound/usb/usx2y/usx2yhwdeppcm.c +++ b/sound/usb/usx2y/usx2yhwdeppcm.c @@ -488,7 +488,9 @@ static int snd_usX2Y_usbpcm_prepare(struct snd_pcm_substream *substream) snd_printdd("snd_usX2Y_pcm_prepare(%p)\n", substream);
if (NULL == usX2Y->hwdep_pcm_shm) { - if (NULL == (usX2Y->hwdep_pcm_shm = snd_malloc_pages(sizeof(struct snd_usX2Y_hwdep_pcm_shm), GFP_KERNEL))) + usX2Y->hwdep_pcm_shm = alloc_pages_exact(sizeof(struct snd_usX2Y_hwdep_pcm_shm), + GFP_KERNEL); + if (!usX2Y->hwdep_pcm_shm) return -ENOMEM; memset(usX2Y->hwdep_pcm_shm, 0, sizeof(struct snd_usX2Y_hwdep_pcm_shm)); } @@ -700,7 +702,7 @@ static void snd_usX2Y_hwdep_pcm_private_free(struct snd_hwdep *hwdep) { struct usX2Ydev *usX2Y = hwdep->private_data; if (NULL != usX2Y->hwdep_pcm_shm) - snd_free_pages(usX2Y->hwdep_pcm_shm, sizeof(struct snd_usX2Y_hwdep_pcm_shm)); + free_pages_exact(usX2Y->hwdep_pcm_shm, sizeof(struct snd_usX2Y_hwdep_pcm_shm)); }
On Wed 27-03-19 10:29:58, Takashi Iwai wrote:
snd_malloc_pages() and snd_free_pages() are merely thin wrappers of the standard page allocator / free functions. Even the arguments are compatible with some standard helpers, so there is little merit of keeping these wrappers.
This patch replaces the all existing callers of snd_malloc_pages() and snd_free_pages() with the direct calls of the standard helper functions. In this version, we use a recently introduced one, alloc_pages_exact(), which suits better than the old snd_malloc_pages() implementation for our purposes. Then we can avoid the waste of pages by alignment to power-of-two.
Since alloc_pages_exact() does split pages, we need no longer __GFP_COMP flag; or better to say, we must not pass __GFP_COMP to alloc_pages_exact(). So the former unconditional addition of __GFP_COMP flag in snd_malloc_pages() is dropped, as well as in most other places.
I haven't checked all the callers but the replacement makes sense to me. I am just wondering whether there is any hard requireemet to have all those requests to be physically contiguous or kvmalloc would suit them better.
Reviewed-by: Takashi Sakamoto o-takashi@sakamocchi.jp Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/memalloc.h | 4 ---- sound/core/memalloc.c | 53 +++-------------------------------------- sound/core/pcm.c | 14 +++++------ sound/usb/usx2y/usX2Yhwdep.c | 3 ++- sound/usb/usx2y/usbusx2y.c | 3 ++- sound/usb/usx2y/usx2yhwdeppcm.c | 6 +++-- 6 files changed, 18 insertions(+), 65 deletions(-)
diff --git a/include/sound/memalloc.h b/include/sound/memalloc.h index 1ac0dd82a916..4c6f3b5a7cff 100644 --- a/include/sound/memalloc.h +++ b/include/sound/memalloc.h @@ -151,9 +151,5 @@ int snd_dma_alloc_pages_fallback(int type, struct device *dev, size_t size, struct snd_dma_buffer *dmab); void snd_dma_free_pages(struct snd_dma_buffer *dmab);
-/* basic memory allocation functions */ -void *snd_malloc_pages(size_t size, gfp_t gfp_flags); -void snd_free_pages(void *ptr, size_t size);
#endif /* __SOUND_MEMALLOC_H */
diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c index eb974235c92b..9f48e1d3a257 100644 --- a/sound/core/memalloc.c +++ b/sound/core/memalloc.c @@ -30,53 +30,6 @@ #endif #include <sound/memalloc.h>
-/*
- Generic memory allocators
- */
-/**
- snd_malloc_pages - allocate pages with the given size
- @size: the size to allocate in bytes
- @gfp_flags: the allocation conditions, GFP_XXX
- Allocates the physically contiguous pages with the given size.
- Return: The pointer of the buffer, or %NULL if no enough memory.
- */
-void *snd_malloc_pages(size_t size, gfp_t gfp_flags) -{
- int pg;
- if (WARN_ON(!size))
return NULL;
- if (WARN_ON(!gfp_flags))
return NULL;
- gfp_flags |= __GFP_COMP; /* compound page lets parts be mapped */
- pg = get_order(size);
- return (void *) __get_free_pages(gfp_flags, pg);
-} -EXPORT_SYMBOL(snd_malloc_pages);
-/**
- snd_free_pages - release the pages
- @ptr: the buffer pointer to release
- @size: the allocated buffer size
- Releases the buffer allocated via snd_malloc_pages().
- */
-void snd_free_pages(void *ptr, size_t size) -{
- int pg;
- if (ptr == NULL)
return;
- pg = get_order(size);
- free_pages((unsigned long) ptr, pg);
-} -EXPORT_SYMBOL(snd_free_pages);
/*
- Bus-specific memory allocators
@@ -190,8 +143,8 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size, dmab->bytes = 0; switch (type) { case SNDRV_DMA_TYPE_CONTINUOUS:
dmab->area = snd_malloc_pages(size,
(__force gfp_t)(unsigned long)device);
dmab->area = alloc_pages_exact(size,
dmab->addr = 0; break;(__force gfp_t)(unsigned long)device);
#ifdef CONFIG_HAS_DMA @@ -275,7 +228,7 @@ void snd_dma_free_pages(struct snd_dma_buffer *dmab) { switch (dmab->dev.type) { case SNDRV_DMA_TYPE_CONTINUOUS:
snd_free_pages(dmab->area, dmab->bytes);
break;free_pages_exact(dmab->area, dmab->bytes);
#ifdef CONFIG_HAS_DMA #ifdef CONFIG_GENERIC_ALLOCATOR diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 7b63aee124af..998e477522fd 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -959,22 +959,22 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream, return -ENOMEM;
size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status));
- runtime->status = snd_malloc_pages(size, GFP_KERNEL);
- runtime->status = alloc_pages_exact(size, GFP_KERNEL); if (runtime->status == NULL) { kfree(runtime); return -ENOMEM; }
- memset((void*)runtime->status, 0, size);
memset(runtime->status, 0, size);
size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_control));
- runtime->control = snd_malloc_pages(size, GFP_KERNEL);
- runtime->control = alloc_pages_exact(size, GFP_KERNEL); if (runtime->control == NULL) {
snd_free_pages((void*)runtime->status,
kfree(runtime); return -ENOMEM; }free_pages_exact(runtime->status, PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status)));
- memset((void*)runtime->control, 0, size);
memset(runtime->control, 0, size);
init_waitqueue_head(&runtime->sleep); init_waitqueue_head(&runtime->tsleep);
@@ -1000,9 +1000,9 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream) runtime = substream->runtime; if (runtime->private_free != NULL) runtime->private_free(runtime);
- snd_free_pages((void*)runtime->status,
- free_pages_exact(runtime->status, PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status)));
- snd_free_pages((void*)runtime->control,
- free_pages_exact(runtime->control, PAGE_ALIGN(sizeof(struct snd_pcm_mmap_control))); kfree(runtime->hw_constraints.rules); /* Avoid concurrent access to runtime via PCM timer interface */
diff --git a/sound/usb/usx2y/usX2Yhwdep.c b/sound/usb/usx2y/usX2Yhwdep.c index c1dd9a7b48df..bfe1108416cf 100644 --- a/sound/usb/usx2y/usX2Yhwdep.c +++ b/sound/usb/usx2y/usX2Yhwdep.c @@ -75,7 +75,8 @@ static int snd_us428ctls_mmap(struct snd_hwdep * hw, struct file *filp, struct v
if (!us428->us428ctls_sharedmem) { init_waitqueue_head(&us428->us428ctls_wait_queue_head);
if(!(us428->us428ctls_sharedmem = snd_malloc_pages(sizeof(struct us428ctls_sharedmem), GFP_KERNEL)))
us428->us428ctls_sharedmem = alloc_pages_exact(sizeof(struct us428ctls_sharedmem), GFP_KERNEL);
memset(us428->us428ctls_sharedmem, -1, sizeof(struct us428ctls_sharedmem)); us428->us428ctls_sharedmem->CtlSnapShotLast = -2;if (!us428->us428ctls_sharedmem) return -ENOMEM;
diff --git a/sound/usb/usx2y/usbusx2y.c b/sound/usb/usx2y/usbusx2y.c index da4a5a541512..9f7bbed2c0f0 100644 --- a/sound/usb/usx2y/usbusx2y.c +++ b/sound/usb/usx2y/usbusx2y.c @@ -437,7 +437,8 @@ static void snd_usX2Y_card_private_free(struct snd_card *card) kfree(usX2Y(card)->In04Buf); usb_free_urb(usX2Y(card)->In04urb); if (usX2Y(card)->us428ctls_sharedmem)
snd_free_pages(usX2Y(card)->us428ctls_sharedmem, sizeof(*usX2Y(card)->us428ctls_sharedmem));
free_pages_exact(usX2Y(card)->us428ctls_sharedmem,
if (usX2Y(card)->card_index >= 0 && usX2Y(card)->card_index < SNDRV_CARDS) snd_usX2Y_card_used[usX2Y(card)->card_index] = 0;sizeof(*usX2Y(card)->us428ctls_sharedmem));
} diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c b/sound/usb/usx2y/usx2yhwdeppcm.c index 714cf50d4a4c..ace8185c3f6d 100644 --- a/sound/usb/usx2y/usx2yhwdeppcm.c +++ b/sound/usb/usx2y/usx2yhwdeppcm.c @@ -488,7 +488,9 @@ static int snd_usX2Y_usbpcm_prepare(struct snd_pcm_substream *substream) snd_printdd("snd_usX2Y_pcm_prepare(%p)\n", substream);
if (NULL == usX2Y->hwdep_pcm_shm) {
if (NULL == (usX2Y->hwdep_pcm_shm = snd_malloc_pages(sizeof(struct snd_usX2Y_hwdep_pcm_shm), GFP_KERNEL)))
usX2Y->hwdep_pcm_shm = alloc_pages_exact(sizeof(struct snd_usX2Y_hwdep_pcm_shm),
GFP_KERNEL);
memset(usX2Y->hwdep_pcm_shm, 0, sizeof(struct snd_usX2Y_hwdep_pcm_shm)); }if (!usX2Y->hwdep_pcm_shm) return -ENOMEM;
@@ -700,7 +702,7 @@ static void snd_usX2Y_hwdep_pcm_private_free(struct snd_hwdep *hwdep) { struct usX2Ydev *usX2Y = hwdep->private_data; if (NULL != usX2Y->hwdep_pcm_shm)
snd_free_pages(usX2Y->hwdep_pcm_shm, sizeof(struct snd_usX2Y_hwdep_pcm_shm));
free_pages_exact(usX2Y->hwdep_pcm_shm, sizeof(struct snd_usX2Y_hwdep_pcm_shm));
}
-- 2.16.4
On Wed, 27 Mar 2019 12:47:30 +0100, Michal Hocko wrote:
On Wed 27-03-19 10:29:58, Takashi Iwai wrote:
snd_malloc_pages() and snd_free_pages() are merely thin wrappers of the standard page allocator / free functions. Even the arguments are compatible with some standard helpers, so there is little merit of keeping these wrappers.
This patch replaces the all existing callers of snd_malloc_pages() and snd_free_pages() with the direct calls of the standard helper functions. In this version, we use a recently introduced one, alloc_pages_exact(), which suits better than the old snd_malloc_pages() implementation for our purposes. Then we can avoid the waste of pages by alignment to power-of-two.
Since alloc_pages_exact() does split pages, we need no longer __GFP_COMP flag; or better to say, we must not pass __GFP_COMP to alloc_pages_exact(). So the former unconditional addition of __GFP_COMP flag in snd_malloc_pages() is dropped, as well as in most other places.
I haven't checked all the callers but the replacement makes sense to me. I am just wondering whether there is any hard requireemet to have all those requests to be physically contiguous or kvmalloc would suit them better.
Most of callers are supposed to allocate buffers that can be also mmapped to user-space, so kvmalloc() won't work. So, the explicit page allocations are on purpose.
thanks,
Takashi
On Wed 27-03-19 14:51:17, Takashi Iwai wrote:
On Wed, 27 Mar 2019 12:47:30 +0100, Michal Hocko wrote:
On Wed 27-03-19 10:29:58, Takashi Iwai wrote:
snd_malloc_pages() and snd_free_pages() are merely thin wrappers of the standard page allocator / free functions. Even the arguments are compatible with some standard helpers, so there is little merit of keeping these wrappers.
This patch replaces the all existing callers of snd_malloc_pages() and snd_free_pages() with the direct calls of the standard helper functions. In this version, we use a recently introduced one, alloc_pages_exact(), which suits better than the old snd_malloc_pages() implementation for our purposes. Then we can avoid the waste of pages by alignment to power-of-two.
Since alloc_pages_exact() does split pages, we need no longer __GFP_COMP flag; or better to say, we must not pass __GFP_COMP to alloc_pages_exact(). So the former unconditional addition of __GFP_COMP flag in snd_malloc_pages() is dropped, as well as in most other places.
I haven't checked all the callers but the replacement makes sense to me. I am just wondering whether there is any hard requireemet to have all those requests to be physically contiguous or kvmalloc would suit them better.
Most of callers are supposed to allocate buffers that can be also mmapped to user-space, so kvmalloc() won't work. So, the explicit page allocations are on purpose.
OK, I see. There is a facility to remap vmalloc pages to userspace (remap_vmalloc_range_partial) but I can see why you do not want to change the code around.
From the snd_malloc_pages->alloc_pages_exact point of view (I haven't
checked all the callers) feel free to add Acked-by: Michal Hocko mhocko@suse.com
On Wed, 27 Mar 2019 15:19:51 +0100, Michal Hocko wrote:
On Wed 27-03-19 14:51:17, Takashi Iwai wrote:
On Wed, 27 Mar 2019 12:47:30 +0100, Michal Hocko wrote:
On Wed 27-03-19 10:29:58, Takashi Iwai wrote:
snd_malloc_pages() and snd_free_pages() are merely thin wrappers of the standard page allocator / free functions. Even the arguments are compatible with some standard helpers, so there is little merit of keeping these wrappers.
This patch replaces the all existing callers of snd_malloc_pages() and snd_free_pages() with the direct calls of the standard helper functions. In this version, we use a recently introduced one, alloc_pages_exact(), which suits better than the old snd_malloc_pages() implementation for our purposes. Then we can avoid the waste of pages by alignment to power-of-two.
Since alloc_pages_exact() does split pages, we need no longer __GFP_COMP flag; or better to say, we must not pass __GFP_COMP to alloc_pages_exact(). So the former unconditional addition of __GFP_COMP flag in snd_malloc_pages() is dropped, as well as in most other places.
I haven't checked all the callers but the replacement makes sense to me. I am just wondering whether there is any hard requireemet to have all those requests to be physically contiguous or kvmalloc would suit them better.
Most of callers are supposed to allocate buffers that can be also mmapped to user-space, so kvmalloc() won't work. So, the explicit page allocations are on purpose.
OK, I see. There is a facility to remap vmalloc pages to userspace (remap_vmalloc_range_partial) but I can see why you do not want to change the code around.
Thanks, it's good to know the remap_vmalloc_range*(). Currently we're mapping vmalloced buffer via vm fault calls.
From the snd_malloc_pages->alloc_pages_exact point of view (I haven't checked all the callers) feel free to add Acked-by: Michal Hocko mhocko@suse.com
Added.
thanks,
Takashi
alloc_pages_exact() is more suitable choice for allocating the sound buffers, as it doesn't need to align with power-of-two. Along with the conversion, we can drop __GFP_COMP as well.
The patch also replace the error messages to be more explicit.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/usx2y/usb_stream.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/sound/usb/usx2y/usb_stream.c b/sound/usb/usx2y/usb_stream.c index 221adf68bd0c..51d73111263a 100644 --- a/sound/usb/usx2y/usb_stream.c +++ b/sound/usb/usx2y/usb_stream.c @@ -155,9 +155,9 @@ void usb_stream_free(struct usb_stream_kernel *sk) if (!s) return;
- free_pages((unsigned long)sk->write_page, get_order(s->write_size)); + free_pages_exact(sk->write_page, s->write_size); sk->write_page = NULL; - free_pages((unsigned long)s, get_order(s->read_size)); + free_pages_exact(s, s->read_size); sk->s = NULL; }
@@ -172,7 +172,6 @@ struct usb_stream *usb_stream_new(struct usb_stream_kernel *sk, int read_size = sizeof(struct usb_stream); int write_size; int usb_frames = dev->speed == USB_SPEED_HIGH ? 8000 : 1000; - int pg;
in_pipe = usb_rcvisocpipe(dev, in_endpoint); out_pipe = usb_sndisocpipe(dev, out_endpoint); @@ -202,11 +201,10 @@ struct usb_stream *usb_stream_new(struct usb_stream_kernel *sk, goto out; }
- pg = get_order(read_size); - sk->s = (void *) __get_free_pages(GFP_KERNEL|__GFP_COMP|__GFP_ZERO| - __GFP_NOWARN, pg); + sk->s = alloc_pages_exact(read_size, + GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN); if (!sk->s) { - snd_printk(KERN_WARNING "couldn't __get_free_pages()\n"); + pr_warn("us122l: couldn't allocate read buffer\n"); goto out; } sk->s->cfg.version = USB_STREAM_INTERFACE_VERSION; @@ -221,13 +219,11 @@ struct usb_stream *usb_stream_new(struct usb_stream_kernel *sk, sk->s->period_size = frame_size * period_frames;
sk->s->write_size = write_size; - pg = get_order(write_size);
- sk->write_page = - (void *)__get_free_pages(GFP_KERNEL|__GFP_COMP|__GFP_ZERO| - __GFP_NOWARN, pg); + sk->write_page = alloc_pages_exact(write_size, + GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN); if (!sk->write_page) { - snd_printk(KERN_WARNING "couldn't __get_free_pages()\n"); + pr_warn("us122l: couldn't allocate write buffer\n"); usb_stream_free(sk); return NULL; }
participants (2)
-
Michal Hocko
-
Takashi Iwai