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

Andrew Morton
2007-05-30T20:33:36Z

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.