[alsa-devel] [PATCH 1/2] ALSA: Replace snd_malloc_pages() and snd_free_pages() with standard helpers, take#2

Michal Hocko mhocko at suse.com
Wed Mar 27 12:47:30 CET 2019


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 at sakamocchi.jp>
> Signed-off-by: Takashi Iwai <tiwai at 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));
>  }
>  
>  
> -- 
> 2.16.4

-- 
Michal Hocko
SUSE Labs


More information about the Alsa-devel mailing list