Re: [patch 1/9] Conditional Calls - Architecture Independent Code

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

On Wed, 30 May 2007 10:00:26 -0400
Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:

> Conditional calls are used to compile in code that is meant to be dynamically
> enabled at runtime. When code is disabled, it has a very small footprint.
> 
> It has a generic cond_call version and optimized per architecture cond_calls.
> The optimized cond_call uses a load immediate to remove a data cache hit.
> 
> It adds a new rodata section "__cond_call" to place the pointers to the enable
> value.
> 
> cond_call activation functions sits in module.c.
> 

The above doesn't really describe what these things are, nor what they are
used for, nor how they actually work.  It's all a bit of a mystery.

The i386 implementation appears to be using self-modifying code, which is
intriguing.

<finds the documentation patch>

OK, that helps somewhat.

> ---
>  include/asm-generic/vmlinux.lds.h |   16 +-
>  include/linux/condcall.h          |   91 +++++++++++++
>  include/linux/module.h            |    4 
>  kernel/module.c                   |  248 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 356 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6-lttng/include/linux/condcall.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6-lttng/include/linux/condcall.h	2007-05-17 02:13:53.000000000 -0400
> @@ -0,0 +1,91 @@
> +#ifndef _LINUX_CONDCALL_H
> +#define _LINUX_CONDCALL_H
> +
> +/*
> + * Conditional function calls. Cheap when disabled, enabled at runtime.
> + *
> + * (C) Copyright 2007 Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> + *
> + * This file is released under the GPLv2.
> + * See the file COPYING for more details.
> + */
> +
> +#ifdef __KERNEL__
> +
> +struct __cond_call_struct {
> +	const char *name;
> +	void *enable;
> +	int flags;
> +} __attribute__((packed));

Please document data structures lavishly.

> +
> +/* Cond call flags : selects the mechanism used to enable the conditional calls
> + * and prescribe what can be executed within their function. This is primarily
> + * used at reentrancy-unfriendly sites. */

You consistently use the wrong commenting style.  We prefer

/* Cond call flags : selects the mechanism used to enable the conditional calls
 * and prescribe what can be executed within their function. This is primarily
 * used at reentrancy-unfriendly sites.
 */

or

/*
 * Cond call flags : selects the mechanism used to enable the conditional calls
 * and prescribe what can be executed within their function. This is primarily
 * used at reentrancy-unfriendly sites.
 */

> +#ifdef CONFIG_COND_CALL_ENABLE_OPTIMIZATION
> +#include <asm/condcall.h>		/* optimized cond_call flavor */
> +#else
> +#include <asm-generic/condcall.h>	/* fallback on generic cond_call */
> +#endif

The preferred way to do this is to give every architecture an
asm/condcall.h and from within that, include asm-generic/condcall.h.  Your
[patch 3/9] does most of that, but it didn't remove the above ifdef, and I
don't think it removed the should-be-unneeded
CONFIG_COND_CALL_ENABLE_OPTIMIZATION either?

> +#define COND_CALL_MAX_FORMAT_LEN	1024
> +
> +extern int cond_call_arm(const char *name);
> +extern int cond_call_disarm(const char *name);
> +
> +/* cond_call_query : Returns 1 if enabled, 0 if disabled or not present */
> +extern int cond_call_query(const char *name);
> +extern int cond_call_list(const char *name);
> +
> +#endif /* __KERNEL__ */
> +#endif
> Index: linux-2.6-lttng/include/asm-generic/vmlinux.lds.h
> ===================================================================
> --- linux-2.6-lttng.orig/include/asm-generic/vmlinux.lds.h	2007-05-17 02:12:25.000000000 -0400
> +++ linux-2.6-lttng/include/asm-generic/vmlinux.lds.h	2007-05-17 02:13:42.000000000 -0400
> @@ -116,11 +116,22 @@
>  		*(__kcrctab_gpl_future)					\
>  		VMLINUX_SYMBOL(__stop___kcrctab_gpl_future) = .;	\
>  	}								\
> -									\
> +	/* Conditional calls: pointers */				\
> +	__cond_call : AT(ADDR(__cond_call) - LOAD_OFFSET) {		\
> +		VMLINUX_SYMBOL(__start___cond_call) = .;		\
> +		*(__cond_call)						\
> +		VMLINUX_SYMBOL(__stop___cond_call) = .;			\
> +	}								\
>  	/* Kernel symbol table: strings */				\
>          __ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) {	\
>  		*(__ksymtab_strings)					\
> -	}								\
> + 	}								\
> +	/* Conditional calls: strings */				\
> +        __cond_call_strings : AT(ADDR(__cond_call_strings) - LOAD_OFFSET) { \

whitespace went bad here.