Re: [RFC -mm 5/6] sysv ipc: deconnect msgmnb and msgmni deactivation and reactivation

[Nadia Derbey <Nadia.Derbey@bull.net>]

Solofo.Ramangalahy@bull.net wrote:
> From: Solofo Ramangalahy <Solofo.Ramangalahy@bull.net>
> 
> The msgmnb and msgmni values are coupled for deactivation and
> reactivation of value computation.
> 
> The uncoupling of msgmn{b,i} for deactivation/reactivation of
> recomputation adds flexibility and testability.
> 
> . Flexibility was discussed during the msgmni series development and
>   ended up with reactivation by negative value on /proc.
> 
> . Testability allows to experiment with the automatic computation of
>   msgmn{b,i} values. For example, if current algorithm does not fit
>   application needs.
> 
> 
> Signed-off-by: Solofo Ramangalahy <Solofo.Ramangalahy@bull.net>
> 
> ---
>  include/linux/ipc_namespace.h |    3 +-
>  ipc/ipc_sysctl.c              |   45 ++++++++++++++++++++++++++++++++----------
>  ipc/ipcns_notifier.c          |    9 --------
>  ipc/msg.c                     |    6 +++++
>  4 files changed, 43 insertions(+), 20 deletions(-)
> 
> Index: b/include/linux/ipc_namespace.h
> ===================================================================
> --- a/include/linux/ipc_namespace.h
> +++ b/include/linux/ipc_namespace.h
> @@ -34,7 +34,9 @@ struct ipc_namespace {
>  
>  	int		msg_ctlmax;
>  	int		msg_ctlmnb;
> +	bool		msg_ctlmnb_activated; /* recompute_msgmnb activation */
>  	int		msg_ctlmni;
> +	bool		msg_ctlmni_activated; /* recompute_msgmni activation */
>  	atomic_t	msg_bytes;
>  	atomic_t	msg_hdrs;
>  
> @@ -53,7 +55,6 @@ extern atomic_t nr_ipc_ns;
>  #define INIT_IPC_NS(ns)		.ns		= &init_ipc_ns,
>  
>  extern int register_ipcns_notifier(struct ipc_namespace *);
> -extern int cond_register_ipcns_notifier(struct ipc_namespace *);
>  extern int unregister_ipcns_notifier(struct ipc_namespace *);
>  extern int ipcns_notify(unsigned long);
>  
> Index: b/ipc/msg.c
> ===================================================================
> --- a/ipc/msg.c
> +++ b/ipc/msg.c
> @@ -91,6 +91,8 @@ void recompute_msgmni(struct ipc_namespa
>  	unsigned long allowed;
>  	int nb_ns;
>  
> +	if (!ns->msg_ctlmni_activated)
> +		return;
>  	si_meminfo(&i);
>  	allowed = (((i.totalram - i.totalhigh) / MSG_MEM_SCALE) * i.mem_unit)
>  		/ ns->msg_ctlmnb;
> @@ -116,6 +118,8 @@ void recompute_msgmni(struct ipc_namespa
>   */
>  void recompute_msgmnb(struct ipc_namespace *ns)
>  {
> +	if (!ns->msg_ctlmnb_activated)
> +		return;
>  	ns->msg_ctlmnb =
>  		min(MSGMNB * num_online_cpus(), MSGMNB * MSG_CPU_SCALE);
>  }
> @@ -123,6 +127,8 @@ void recompute_msgmnb(struct ipc_namespa
>  void msg_init_ns(struct ipc_namespace *ns)
>  {
>  	ns->msg_ctlmax = MSGMAX;
> +	ns->msg_ctlmnb_activated = true;
> +	ns->msg_ctlmni_activated = true;
>  	recompute_msgmnb(ns);
>  
>  	recompute_msgmni(ns);
> Index: b/ipc/ipc_sysctl.c
> ===================================================================
> --- a/ipc/ipc_sysctl.c
> +++ b/ipc/ipc_sysctl.c
> @@ -33,18 +33,42 @@ static void *get_ipc(ctl_table *table)
>   * add/remove or ipc namespace creation/removal.
>   * They can come back to a recomputable state by being set to a <0 value.
>   */
> -static void tunable_set_callback(int val)
> +static void tunable_set_callback(int val, ctl_table *table)
>  {
> -	if (val >= 0)
> -		unregister_ipcns_notifier(current->nsproxy->ipc_ns);
> -	else {
> +	int tunable = table->ctl_name;
> +
> +	if (val >= 0) {
> +		switch (tunable) {
> +		case KERN_MSGMNB:
> +			current->nsproxy->ipc_ns->msg_ctlmnb_activated = false;
> +			break;
> +		case KERN_MSGMNI:
> +			current->nsproxy->ipc_ns->msg_ctlmni_activated = false;
> +			break;
> +		default:
> +			printk(KERN_ERR "ipc: unexpected value %s\n",
> +			       table->procname);
> +			break;
> +		}
> +	} else {
>  		/*
>  		 * Re-enable automatic recomputing only if not already
>  		 * enabled.
>  		 */
> -		recompute_msgmnb(current->nsproxy->ipc_ns);
> -		recompute_msgmni(current->nsproxy->ipc_ns);
> -		cond_register_ipcns_notifier(current->nsproxy->ipc_ns);
> +		switch (tunable) {
> +		case KERN_MSGMNB:
> +			current->nsproxy->ipc_ns->msg_ctlmnb_activated = true;
> +			recompute_msgmnb(current->nsproxy->ipc_ns);
> +							/* fall through */

You shouldn't be falling through here: if you do that, re-enablng msgmnb 
will re-enable msgmni too.

> +		case KERN_MSGMNI:
> +			current->nsproxy->ipc_ns->msg_ctlmni_activated = true;
> +			recompute_msgmni(current->nsproxy->ipc_ns);
> +			break;
> +		default:
> +			printk(KERN_ERR "ipc: unexpected value %s\n",
> +			       table->procname);
> +			break;
> +		}
>  	}
>  }
>  
> @@ -72,7 +96,8 @@ static int proc_ipc_callback_dointvec(ct
>  	rc = proc_dointvec(&ipc_table, write, filp, buffer, lenp, ppos);
>  
>  	if (write && !rc && lenp_bef == *lenp)
> -		tunable_set_callback(*((int *)(ipc_table.data)));
> +		BUG_ON(table == NULL);
> +		tunable_set_callback(*((int *)(ipc_table.data)), table);
>  
>  	return rc;
>  }
> @@ -148,8 +173,8 @@ static int sysctl_ipc_registered_data(ct
>  		 * Tunable has successfully been changed from userland
>  		 */
>  		int *data = get_ipc(table);
> -
> -		tunable_set_callback(*data);
> +		BUG_ON(table == NULL);
> +		tunable_set_callback(*data, table);
>  	}
>  
>  	return rc;
> Index: b/ipc/ipcns_notifier.c
> ===================================================================
> --- a/ipc/ipcns_notifier.c
> +++ b/ipc/ipcns_notifier.c
> @@ -65,15 +65,6 @@ int register_ipcns_notifier(struct ipc_n
>  	return blocking_notifier_chain_register(&ipcns_chain, &ns->ipcns_nb);
>  }
>  
> -int cond_register_ipcns_notifier(struct ipc_namespace *ns)
> -{
> -	memset(&ns->ipcns_nb, 0, sizeof(ns->ipcns_nb));
> -	ns->ipcns_nb.notifier_call = ipcns_callback;
> -	ns->ipcns_nb.priority = IPCNS_CALLBACK_PRI;
> -	return blocking_notifier_chain_cond_register(&ipcns_chain,
> -							&ns->ipcns_nb);
> -}
> -
>  int unregister_ipcns_notifier(struct ipc_namespace *ns)
>  {
>  	return blocking_notifier_chain_unregister(&ipcns_chain,
> 

Doing this, we are completly loosing the benefits of the notification 
chains: since the the notifier blocks remain registered + we are 
unconditionally adding a test at the top of each recompute routine. But 
the other choice would hve been to define another notifier chain 
dedicated to msgmnb. I'm not convinced about what is the best solution?

Regards,
Nadia