Re: [RFC PATCH 1/2] Scaling msgmni to the system memory

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 11 Dec 2007 16:38:46 +0100
Nadia.Derbey@bull.net wrote:

> [PATCH 01/02]
> 
> This patch computes msg_ctlmni to make it scale with system memory.
> msg_ctlmni is now set to make the message queues occupy 1/32 of the available
> memory.
> 
> Some cleaning has also been done in the MSGXXX constants:
>   . MSGPOOL: the msgctl man page says it's not used, but it also defines it as
>              a size in bytes (the code expresses it in Kbytes).
>   . MSGSEG definition has been removed since it used only once in msgctl().
> 

The objective seems reasonable.

> 
> ===================================================================
> --- linux-2.6.24-rc4.orig/include/linux/msg.h	2007-12-11 11:57:53.000000000 +0100
> +++ linux-2.6.24-rc4/include/linux/msg.h	2007-12-11 12:10:01.000000000 +0100
> @@ -49,17 +49,17 @@ struct msginfo {
>  	unsigned short  msgseg; 
>  };
>  
> +#define MSG_MEM_SCALE 32    /* Scaling factor to compute msgmni */
> +
>  #define MSGMNI    16   /* <= IPCMNI */     /* max # of msg queue identifiers */
>  #define MSGMAX  8192   /* <= INT_MAX */   /* max size of message (bytes) */
>  #define MSGMNB 16384   /* <= INT_MAX */   /* default max size of a message queue */
>  
>  /* unused */
> -#define MSGPOOL (MSGMNI*MSGMNB/1024)  /* size in kilobytes of message pool */
> +#define MSGPOOL (MSGMNI * MSGMNB) /* size in bytes of message pool */
>  #define MSGTQL  MSGMNB            /* number of system message headers */
>  #define MSGMAP  MSGMNB            /* number of entries in message map */
>  #define MSGSSZ  16                /* message segment size */
> -#define __MSGSEG ((MSGPOOL*1024)/ MSGSSZ) /* max no. of segments */
> -#define MSGSEG (__MSGSEG <= 0xffff ? __MSGSEG : 0xffff)
>  
>  #ifdef __KERNEL__
>  #include <linux/list.h>
> Index: linux-2.6.24-rc4/ipc/msg.c
> ===================================================================
> --- linux-2.6.24-rc4.orig/ipc/msg.c	2007-12-11 11:57:58.000000000 +0100
> +++ linux-2.6.24-rc4/ipc/msg.c	2007-12-11 12:12:32.000000000 +0100
> @@ -27,6 +27,7 @@
>  #include <linux/msg.h>
>  #include <linux/spinlock.h>
>  #include <linux/init.h>
> +#include <linux/mm.h>
>  #include <linux/proc_fs.h>
>  #include <linux/list.h>
>  #include <linux/security.h>
> @@ -81,10 +82,25 @@ static int sysvipc_msg_proc_show(struct 
>  
>  static void __msg_init_ns(struct ipc_namespace *ns, struct ipc_ids *ids)
>  {
> +	struct sysinfo i;
> +	unsigned long allowed;
> +
>  	ns->ids[IPC_MSG_IDS] = ids;
>  	ns->msg_ctlmax = MSGMAX;
>  	ns->msg_ctlmnb = MSGMNB;
> -	ns->msg_ctlmni = MSGMNI;
> +
> +	/*
> +	 * Scale msgmni with the available memory size: the memory dedicated
> +	 * to msg queues should occupy 1/32 of the available memory:
> +	 * up to 8MB       : msgmni = 16 (MSGMNI)
> +	 * 4 GB            : msgmni = 8K
> +	 * more than 16 GB : msgmni = 32K (IPCMNI)
> +	 */
> +	si_meminfo(&i);
> +	allowed = ((i.totalram / MSG_MEM_SCALE) * i.mem_unit) / MSGMNB;
> +	ns->msg_ctlmni = min((unsigned long) IPCMNI,
> +			max((unsigned long) MSGMNI, allowed));

The space after the (typecast) isn't useful IMO.

Please use min_t rather than the open-coded casts.

Even better would be to sort out the types so that neither casts nor min_t
are needed.

What about highmem machines?  For those we usually want to scale data
structures according to the amount of direct-addressable memory (ie:
lowmem) rather than acording to total physical memory.  I haven't a clue
how this consideration would be addressed when ipc-namespaces is taken into
consideration.

I'd suggest the addition of a printk telling people what value the kernel
calculated.

We should ensure that the calculated value is never _less_ than what the
kernel was previously giving - to avoid breaking existing things.

It's a bit of a concern that a change like this can cause an application to
work OK on machine A but then fail when it is taken over to (smaller)
machine B.