Re: [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel

[Michal Hocko]

On Mon 26-04-21 13:18:14, Vasily Averin wrote:
> msg_queue and shmid_kernel are quite small objects, no need to use
> kvmalloc for them.

Both of them are 256B on most 64b systems.

> Previously these objects was allocated via ipc_alloc/ipc_rcu_alloc(),
> common function for several ipc objects. It had kvmalloc call inside().
> Later, this function went away and was finally replaced by direct
> kvmalloc call, and now we can use more suitable kmalloc/kfree for them.

This describes the history of the code but it doesn't really explain why
kvmalloc is a bad decision here. I would go with the following:
"
Using kvmalloc for sub page size objects is suboptimal because kmalloc
can easily fallback into vmalloc under memory pressure and smaller
objects would fragment memory. Therefore replace kvmalloc by a simple
kmalloc.
"
> 
> Reported-by: Alexey Dobriyan <adobriyan@xxxxxxxxx>
> Signed-off-by: Vasily Averin <vvs@xxxxxxxxxxxxx>

With a clarified changelog, feel free to add
Acked-by: Michal Hocko <mhocko@xxxxxxxx>

Thanks!

> ---
>  ipc/msg.c | 6 +++---
>  ipc/shm.c | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/ipc/msg.c b/ipc/msg.c
> index 87898cb..79c6625 100644
> --- a/ipc/msg.c
> +++ b/ipc/msg.c
> @@ -130,7 +130,7 @@ static void msg_rcu_free(struct rcu_head *head)
>  	struct msg_queue *msq = container_of(p, struct msg_queue, q_perm);
>  
>  	security_msg_queue_free(&msq->q_perm);
> -	kvfree(msq);
> +	kfree(msq);
>  }
>  
>  /**
> @@ -147,7 +147,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params)
>  	key_t key = params->key;
>  	int msgflg = params->flg;
>  
> -	msq = kvmalloc(sizeof(*msq), GFP_KERNEL_ACCOUNT);
> +	msq = kmalloc(sizeof(*msq), GFP_KERNEL_ACCOUNT);
>  	if (unlikely(!msq))
>  		return -ENOMEM;
>  
> @@ -157,7 +157,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params)
>  	msq->q_perm.security = NULL;
>  	retval = security_msg_queue_alloc(&msq->q_perm);
>  	if (retval) {
> -		kvfree(msq);
> +		kfree(msq);
>  		return retval;
>  	}
>  
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 7632d72..85da060 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -222,7 +222,7 @@ static void shm_rcu_free(struct rcu_head *head)
>  	struct shmid_kernel *shp = container_of(ptr, struct shmid_kernel,
>  							shm_perm);
>  	security_shm_free(&shp->shm_perm);
> -	kvfree(shp);
> +	kfree(shp);
>  }
>  
>  static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s)
> @@ -619,7 +619,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
>  			ns->shm_tot + numpages > ns->shm_ctlall)
>  		return -ENOSPC;
>  
> -	shp = kvmalloc(sizeof(*shp), GFP_KERNEL_ACCOUNT);
> +	shp = kmalloc(sizeof(*shp), GFP_KERNEL_ACCOUNT);
>  	if (unlikely(!shp))
>  		return -ENOMEM;
>  
> @@ -630,7 +630,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
>  	shp->shm_perm.security = NULL;
>  	error = security_shm_alloc(&shp->shm_perm);
>  	if (error) {
> -		kvfree(shp);
> +		kfree(shp);
>  		return error;
>  	}
>  
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs