Use attribute((malloc)) for gcc 3.2

PATCH In review Andi Kleen 10 participants 17 messages Sep 29, 2002 – Oct 7, 2002

Subsystems (5)

Networking MM core Page allocator Slab allocator +1 more subsystems

lore

Share & tools

lore

Andi Kleen

author

2002-09-29 15:22 UTC #0 lore raw

PATCH 1/1 Use __attribute__((malloc)) for gcc 3.2

gcc 3.2 has an __attribute__((malloc)) function attribute. It tells gcc that a function returns newly allocated memory and that the return pointer cannot alias with any other pointer in the parent function. This often allows gcc to generate better code because the optimizer doesn't need take pointer aliasing in account.

This patch implements it for 2.5.39 and some common allocation functions.

Also added an noinline macro to wrap __attribute__((noinline)). That's not used yet. It tells the compiler that it should never inline, which may be useful to prevent some awful code generation for those misguided folks who use -O3 (gcc often screws up the register allocation of a function completely when bigger functions are inlined).

Patch for 2.5.39.

Please consider applying.

-Andi

include/asm-i386/pgalloc.h +2 -2

diff -X ../KDIFX -burp linux/include/asm-i386/pgalloc.h linux-2.5.39-work/include/asm-i386/pgalloc.h
@@ -20,10 +20,10 @@ static inline void pmd_populate(struct m
  * Allocate and free page tables.
  */
 
-extern pgd_t *pgd_alloc(struct mm_struct *);
+extern pgd_t *pgd_alloc(struct mm_struct *) malloc_function;
 extern void pgd_free(pgd_t *pgd);
 
-extern pte_t *pte_alloc_one_kernel(struct mm_struct *, unsigned long);
+extern pte_t *pte_alloc_one_kernel(struct mm_struct *, unsigned long) malloc_function;
 extern struct page *pte_alloc_one(struct mm_struct *, unsigned long);
 
 static inline void pte_free_kernel(pte_t *pte)

include/linux/bootmem.h +2 -2

diff -X ../KDIFX -burp linux/include/linux/bootmem.h linux-2.5.39-work/include/linux/bootmem.h
@@ -37,7 +37,7 @@ typedef struct bootmem_data {
 extern unsigned long __init bootmem_bootmap_pages (unsigned long);
 extern unsigned long __init init_bootmem (unsigned long addr, unsigned long memend);
 extern void __init free_bootmem (unsigned long addr, unsigned long size);
-extern void * __init __alloc_bootmem (unsigned long size, unsigned long align, unsigned long goal);
+extern void * __init __alloc_bootmem (unsigned long size, unsigned long align, unsigned long goal) malloc_function;
 #ifndef CONFIG_HAVE_ARCH_BOOTMEM_NODE
 extern void __init reserve_bootmem (unsigned long addr, unsigned long size);
 #define alloc_bootmem(x) \

@@ -55,7 +55,7 @@ extern unsigned long __init init_bootmem
 extern void __init reserve_bootmem_node (pg_data_t *pgdat, unsigned long physaddr, unsigned long size);
 extern void __init free_bootmem_node (pg_data_t *pgdat, unsigned long addr, unsigned long size);
 extern unsigned long __init free_all_bootmem_node (pg_data_t *pgdat);
-extern void * __init __alloc_bootmem_node (pg_data_t *pgdat, unsigned long size, unsigned long align, unsigned long goal);
+extern void * __init __alloc_bootmem_node (pg_data_t *pgdat, unsigned long size, unsigned long align, unsigned long goal) malloc_function;
 #ifndef CONFIG_HAVE_ARCH_BOOTMEM_NODE
 #define alloc_bootmem_node(pgdat, x) \
 	__alloc_bootmem_node((pgdat), (x), SMP_CACHE_BYTES, __pa(MAX_DMA_ADDRESS))

include/linux/dcache.h +2 -2

diff -X ../KDIFX -burp linux/include/linux/dcache.h linux-2.5.39-work/include/linux/dcache.h
@@ -173,8 +173,8 @@ extern void d_instantiate(struct dentry 
 extern void d_delete(struct dentry *);
 
 /* allocate/de-allocate */
-extern struct dentry * d_alloc(struct dentry *, const struct qstr *);
-extern struct dentry * d_alloc_anon(struct inode *);
+extern struct dentry * d_alloc(struct dentry *, const struct qstr *) malloc_function;
+extern struct dentry * d_alloc_anon(struct inode *) malloc_function;
 extern struct dentry * d_splice_alias(struct inode *, struct dentry *);
 extern void shrink_dcache_sb(struct super_block *);
 extern void shrink_dcache_parent(struct dentry *);

include/linux/gfp.h +6 -5

diff -X ../KDIFX -burp linux/include/linux/gfp.h linux-2.5.39-work/include/linux/gfp.h
@@ -39,8 +39,8 @@
  * can allocate highmem pages, the *get*page*() variants return
  * virtual kernel addresses to the allocated page(s).
  */
-extern struct page * FASTCALL(__alloc_pages(unsigned int gfp_mask, unsigned int order, struct zonelist *zonelist));
-extern struct page * alloc_pages_node(int nid, unsigned int gfp_mask, unsigned int order);
+extern struct page * FASTCALL(__alloc_pages(unsigned int gfp_mask, unsigned int order, struct zonelist *zonelist)) malloc_function;
+extern struct page * alloc_pages_node(int nid, unsigned int gfp_mask, unsigned int order) malloc_function;
 
 /*
  * We get the zone list from the current node and the gfp_mask.

@@ -49,7 +49,8 @@ extern struct page * alloc_pages_node(in
  * For the normal case of non-DISCONTIGMEM systems the NODE_DATA() gets
  * optimized to &contig_page_data at compile-time.
  */
-static inline struct page * alloc_pages(unsigned int gfp_mask, unsigned int order)
+static inline malloc_function struct page * 
+alloc_pages(unsigned int gfp_mask, unsigned int order)
 {
 	pg_data_t *pgdat = NODE_DATA(numa_node_id());
 	unsigned int idx = (gfp_mask & GFP_ZONEMASK);

@@ -62,8 +63,8 @@ static inline struct page * alloc_pages(
 
 #define alloc_page(gfp_mask) alloc_pages(gfp_mask, 0)
 
-extern unsigned long FASTCALL(__get_free_pages(unsigned int gfp_mask, unsigned int order));
-extern unsigned long FASTCALL(get_zeroed_page(unsigned int gfp_mask));
+extern unsigned long FASTCALL(__get_free_pages(unsigned int gfp_mask, unsigned int order)) malloc_function; 
+extern unsigned long FASTCALL(get_zeroed_page(unsigned int gfp_mask)) malloc_function;
 
 #define __get_free_page(gfp_mask) \
 		__get_free_pages((gfp_mask),0)

include/linux/jbd.h +1 -1

diff -X ../KDIFX -burp linux/include/linux/jbd.h linux-2.5.39-work/include/linux/jbd.h
@@ -54,7 +54,7 @@ extern int journal_enable_debug;
 #define jbd_debug(f, a...)	/**/
 #endif
 
-extern void * __jbd_kmalloc (char *where, size_t size, int flags, int retry);
+extern void * __jbd_kmalloc (char *where, size_t size, int flags, int retry) malloc_function;
 #define jbd_kmalloc(size, flags) \
 	__jbd_kmalloc(__FUNCTION__, (size), (flags), journal_oom_retry)
 #define jbd_rep_kmalloc(size, flags) \

include/linux/kernel.h +10 -0

diff -X ../KDIFX -burp linux/include/linux/kernel.h linux-2.5.39-work/include/linux/kernel.h
@@ -47,6 +47,16 @@ void __might_sleep(char *file, int line)
 #define might_sleep() do {} while(0)
 #endif
 
+#if __GNUC__ >= 3 && __GNUC_MINOR__ >= 1
+/* Function allocates new memory and return cannot alias with anything */
+#define malloc_function __attribute__((malloc))
+/* Never inline */
+#define noinline __attribute__((noinline))
+#else
+#define malloc_function
+#define noinline
+#endif
+
 extern struct notifier_block *panic_notifier_list;
 NORET_TYPE void panic(const char * fmt, ...)
 	__attribute__ ((NORET_AND format (printf, 1, 2)));

include/linux/mempool.h +2 -2

diff -X ../KDIFX -burp linux/include/linux/mempool.h linux-2.5.39-work/include/linux/mempool.h
@@ -21,10 +21,10 @@ typedef struct mempool_s {
 	wait_queue_head_t wait;
 } mempool_t;
 extern mempool_t * mempool_create(int min_nr, mempool_alloc_t *alloc_fn,
-				 mempool_free_t *free_fn, void *pool_data);
+				 mempool_free_t *free_fn, void *pool_data) malloc_function;
 extern int mempool_resize(mempool_t *pool, int new_min_nr, int gfp_mask);
 extern void mempool_destroy(mempool_t *pool);
-extern void * mempool_alloc(mempool_t *pool, int gfp_mask);
+extern void * mempool_alloc(mempool_t *pool, int gfp_mask) malloc_function;
 extern void mempool_free(void *element, mempool_t *pool);
 
 #endif /* _LINUX_MEMPOOL_H */

include/linux/mm.h +2 -1

diff -X ../KDIFX -burp linux/include/linux/mm.h linux-2.5.39-work/include/linux/mm.h
@@ -417,7 +417,8 @@ static inline int set_page_dirty(struct 
  * inlining and the symmetry break with pte_alloc_map() that does all
  * of this out-of-line.
  */
-static inline pmd_t *pmd_alloc(struct mm_struct *mm, pgd_t *pgd, unsigned long address)
+static inline malloc_function pmd_t *
+pmd_alloc(struct mm_struct *mm, pgd_t *pgd, unsigned long address) 
 {
 	if (pgd_none(*pgd))
 		return __pmd_alloc(mm, pgd, address);

include/linux/skbuff.h +4 -4

diff -X ../KDIFX -burp linux/include/linux/skbuff.h linux-2.5.39-work/include/linux/skbuff.h
@@ -259,11 +259,11 @@ struct sk_buff {
 #include <asm/system.h>
 
 extern void	       __kfree_skb(struct sk_buff *skb);
-extern struct sk_buff *alloc_skb(unsigned int size, int priority);
+extern struct sk_buff *alloc_skb(unsigned int size, int priority) malloc_function;
 extern void	       kfree_skbmem(struct sk_buff *skb);
-extern struct sk_buff *skb_clone(struct sk_buff *skb, int priority);
-extern struct sk_buff *skb_copy(const struct sk_buff *skb, int priority);
-extern struct sk_buff *pskb_copy(struct sk_buff *skb, int gfp_mask);
+extern struct sk_buff *skb_clone(struct sk_buff *skb, int priority) malloc_function;
+extern struct sk_buff *skb_copy(const struct sk_buff *skb, int priority) malloc_function;
+extern struct sk_buff *pskb_copy(struct sk_buff *skb, int gfp_mask) malloc_function;
 extern int	       pskb_expand_head(struct sk_buff *skb,
 					int nhead, int ntail, int gfp_mask);
 extern struct sk_buff *skb_realloc_headroom(struct sk_buff *skb,

include/linux/slab.h +2 -2

diff -X ../KDIFX -burp linux/include/linux/slab.h linux-2.5.39-work/include/linux/slab.h
@@ -55,11 +55,11 @@ extern kmem_cache_t *kmem_cache_create(c
 				       void (*)(void *, kmem_cache_t *, unsigned long));
 extern int kmem_cache_destroy(kmem_cache_t *);
 extern int kmem_cache_shrink(kmem_cache_t *);
-extern void *kmem_cache_alloc(kmem_cache_t *, int);
+extern void *kmem_cache_alloc(kmem_cache_t *, int) malloc_function;
 extern void kmem_cache_free(kmem_cache_t *, void *);
 extern unsigned int kmem_cache_size(kmem_cache_t *);
 
-extern void *kmalloc(size_t, int);
+extern void *kmalloc(size_t, int) malloc_function;
 extern void kfree(const void *);
 
 extern int FASTCALL(kmem_cache_reap(int));

include/linux/vmalloc.h +3 -3

diff -X ../KDIFX -burp linux/include/linux/vmalloc.h linux-2.5.39-work/include/linux/vmalloc.h
@@ -21,9 +21,9 @@ struct vm_struct {
 /*
  *	Highlevel APIs for driver use
  */
-extern void *vmalloc(unsigned long size);
-extern void *vmalloc_32(unsigned long size);
-extern void *__vmalloc(unsigned long size, int gfp_mask, pgprot_t prot);
+extern void *vmalloc(unsigned long size) malloc_function;
+extern void *vmalloc_32(unsigned long size) malloc_function;
+extern void *__vmalloc(unsigned long size, int gfp_mask, pgprot_t prot) malloc_function;
 extern void vfree(void *addr);
 
 extern void *vmap(struct page **pages, unsigned int count);

Adrian Bunk

2002-09-29 16:18 UTC #1 lore raw

Quoted 8 lines · Andi Kleen

...

include/linux/kernel.h +1 -0

@@ -47,6 +47,16 @@ void __might_sleep(char *file, int line)
 #define might_sleep() do {} while(0)
 #endif
 
+#if __GNUC__ >= 3 && __GNUC_MINOR__ >= 1

include/linux/kernel.h void __might_sleep(char *file, int line) · line 59

Quoted 10 lines

include/linux/kernel.h +9 -0

+/* Function allocates new memory and return cannot alias with anything */
+#define malloc_function __attribute__((malloc))
+/* Never inline */
+#define noinline __attribute__((noinline))
+#else
+#define malloc_function
+#define noinline
+#endif
+
...

Why did you put it in kernel.h and not in compiler.h?

cu
Adrian

Signature · 3 lines

You only think this is a free country. Like the US the UK spends a lot of
time explaining its a free country because its a police state.
								Alan Cox

Christoph Hellwig

2002-09-29 17:21 UTC #2 lore raw

In reply to Andi Kleen

include/asm-i386/pgalloc.h static inline void pmd_populate(struct m · line 23

Quoted 2 lines · Andi Kleen

include/asm-i386/pgalloc.h +1 -1

-extern pgd_t *pgd_alloc(struct mm_struct *);
+extern pgd_t *pgd_alloc(struct mm_struct *) malloc_function;

Anz chance you could make that __malloc? That how the other atrributes in the kernel (e.g. __init/__exit) work.

include/linux/kernel.h void __might_sleep(char *file, int line) · line 58

Quoted 8 lines

include/linux/kernel.h +8 -0

+/* Function allocates new memory and return cannot alias with anything */
+#define malloc_function __attribute__((malloc))
+/* Never inline */
+#define noinline __attribute__((noinline))
+#else
+#define malloc_function
+#define noinline
+#endif

Dito for __noinline? And IMHO compiler.h is the better place for this.

BTW, do you have any stats on the better optimization?

David S. Miller

2002-09-30 00:12 UTC #3 lore raw

From: Christoph Hellwig <[email protected]>
Date: Sun, 29 Sep 2002 18:26:43 +0100

BTW, do you have any stats on the better optimization?

Unlikely since we disable strict aliasing on the gcc command line which is why I think this suggested __malloc thing is utterly pointless.

Andi Kleen

author

2002-09-30 00:24 UTC #4 lore raw

Quoted 8 lines · David S. Miller

From: Christoph Hellwig <[email protected]>
Date: Sun, 29 Sep 2002 18:26:43 +0100
BTW, do you have any stats on the better optimization?
Unlikely since we disable strict aliasing on the gcc command line which is why I think this suggested __malloc thing is utterly pointless.

My understanding of it is: Please correct me if I'm wrong [i haven't verified this with tree dumps]

-fno-strict-aliasing tells each pointer that it can alias with everything else (puts it in alias set 0)

__attribute__((malloc)) overwrites this so that the compiler knows that this particular pointer that has been returned (puts it into an own alias set again and overwrites the -fno-strict-aliasing default)

Another way to overwrite it would be restrict, but nobody uses that currently. May make sense to add it to compile compiler version checked macros too, so that it can be safely used.

Then it would be actually possible to write functions that get optimized this way. I'm aware that the aliasing stuff mostly helps with array walks and loops, which are not that common in the kernel, but it could be still useful.

-Andi

Jakub Jelinek

2002-09-29 19:56 UTC #5 lore raw

In reply to Andi Kleen

Quoted 6 lines · Andi Kleen

gcc 3.2 has an __attribute__((malloc)) function attribute. It tells gcc that a function returns newly allocated memory and that the return pointer cannot alias with any other pointer in the parent function. This often allows gcc to generate better code because the optimizer doesn't need take pointer aliasing in account.

Does this matter when the kernel is compiled with -fno-strict-aliasing?

Jakub

Andi Kleen

author

2002-09-30 00:00 UTC #6 lore raw

Quoted 9 lines · Jakub Jelinek

Quoted 6 lines · Andi Kleen
gcc 3.2 has an __attribute__((malloc)) function attribute. It tells gcc that a function returns newly allocated memory and that the return pointer cannot alias with any other pointer in the parent function. This often allows gcc to generate better code because the optimizer doesn't need take pointer aliasing in account.
Does this matter when the kernel is compiled with -fno-strict-aliasing?

I don't know. But it's certainly worth trying, isn't it ? Giving the compiler more information can't be a bad thing.

-Andi

Richard Henderson

2002-10-07 10:00 UTC #7 lore raw

In reply to Jakub Jelinek

Quoted 1 line · Jakub Jelinek

Does this matter when the kernel is compiled with -fno-strict-aliasing?

Yes. The malloc attribute uses REG_NOALIAS, not alias sets.

David S. Miller

2002-10-07 10:30 UTC #8 lore raw

From: Richard Henderson <[email protected]>
Date: Mon, 7 Oct 2002 03:05:41 -0700

Quoted 1 line · Jakub Jelinek

Does this matter when the kernel is compiled with -fno-strict-aliasing?

Yes. The malloc attribute uses REG_NOALIAS, not alias sets.

Great, I'm all for Andi's patch in that case.

Andi Kleen

author

2002-10-07 10:51 UTC #9 lore raw

Quoted 9 lines · David S. Miller

From: Richard Henderson <[email protected]>
Date: Mon, 7 Oct 2002 03:05:41 -0700
Quoted 1 line · Jakub Jelinek
Does this matter when the kernel is compiled with -fno-strict-aliasing?
Yes. The malloc attribute uses REG_NOALIAS, not alias sets.
Great, I'm all for Andi's patch in that case.

I retested it on gcc 3.2 and it unfortunately doesn't make any difference (resulting kernel is byte-for-byte identical). So it looks like with current gcc it isn't worth the effort.

-Andi

Arjan van de Ven

2002-10-07 10:58 UTC #10 lore raw

Quoted 7 lines · Andi Kleen

Quoted 6 lines · David S. Miller
From: Richard Henderson <[email protected]>
Date: Mon, 7 Oct 2002 03:05:41 -0700
Quoted 1 line · Jakub Jelinek
Does this matter when the kernel is compiled with -fno-strict-aliasing?

Quoted 7 lines

Quoted 3 lines
Yes. The malloc attribute uses REG_NOALIAS, not alias sets.
Great, I'm all for Andi's patch in that case.
I retested it on gcc 3.2 and it unfortunately doesn't make any difference (resulting kernel is byte-for-byte identical). So it looks like with current gcc it isn't worth the effort.

I'm still in favor of doing it, even if it's just to break the chick-and-egg deadlock that such issues can have.

Greetings,
Arjan van de Ven

Richard Henderson

2002-10-07 11:39 UTC #11 lore raw

In reply to Andi Kleen

Quoted 3 lines · Andi Kleen

I retested it on gcc 3.2 and it unfortunately doesn't make any difference (resulting kernel is byte-for-byte identical). So it looks like with current gcc it isn't worth the effort.

*shrug* It's still good documentation of intent. And one of these days we might get around to writing some aliasing code that doesn't suck quite so much. ;-)

Andi Kleen

author

2002-10-07 12:22 UTC #12 lore raw

Quoted 8 lines · Richard Henderson

Quoted 3 lines · Andi Kleen
I retested it on gcc 3.2 and it unfortunately doesn't make any difference (resulting kernel is byte-for-byte identical). So it looks like with current gcc it isn't worth the effort.
*shrug* It's still good documentation of intent. And one of these days we might get around to writing some aliasing code that doesn't suck quite so much. ;-)

I'll resubmit the patch then.

Thanks for the feedback.

-Andi

Anton Blanchard

2002-09-29 23:48 UTC #13 lore raw

In reply to Andi Kleen

Hi Andi,

Quoted 5 lines

Also added an noinline macro to wrap __attribute__((noinline)). That's not used yet. It tells the compiler that it should never inline, which may be useful to prevent some awful code generation for those misguided folks who use -O3 (gcc often screws up the register allocation of a function completely when bigger functions are inlined).

Could you also add an always inline? It would be useful for functions like context_switch, where we require it to be inlined (otherwise it falls outside scheduling_functions_{start,end}_here and wchan handling fails).

Anton

Andi Kleen

author

2002-09-30 00:26 UTC #14 lore raw

Quoted 9 lines · Anton Blanchard

Hi Andi,
Quoted 5 lines
Also added an noinline macro to wrap __attribute__((noinline)). That's not used yet. It tells the compiler that it should never inline, which may be useful to prevent some awful code generation for those misguided folks who use -O3 (gcc often screws up the register allocation of a function completely when bigger functions are inlined).

Quoted 4 lines

Could you also add an always inline? It would be useful for functions like context_switch, where we require it to be inlined (otherwise it falls outside scheduling_functions_{start,end}_here and wchan handling fails).

Ok. gcc supports it with __attribute__((always_inline))

Suggestions for a name? alwaysinline would be a bit lengthy.

-Andi

Daniel Jacobowitz

2002-09-30 01:02 UTC #15 lore raw

Quoted 10 lines · Andi Kleen

Quoted 9 lines · Anton Blanchard
Hi Andi,
Quoted 5 lines
Also added an noinline macro to wrap __attribute__((noinline)). That's not used yet. It tells the compiler that it should never inline, which may be useful to prevent some awful code generation for those misguided folks who use -O3 (gcc often screws up the register allocation of a function completely when bigger functions are inlined).

Quoted 8 lines

Quoted 4 lines
Could you also add an always inline? It would be useful for functions like context_switch, where we require it to be inlined (otherwise it falls outside scheduling_functions_{start,end}_here and wchan handling fails).
Ok. gcc supports it with __attribute__((always_inline))
Suggestions for a name? alwaysinline would be a bit lengthy.

Stick with always_inline?

Signature · 2 lines

Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Pavel Machek

2002-10-02 21:53 UTC #16 lore raw

In reply to Andi Kleen

Hi!

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
may be useful to prevent some awful code generation for those misguided folks who use -O3 (gcc often screws up the register allocation of a function completely when bigger functions are inlined).

Quoted 8 lines

Quoted 4 lines
Could you also add an always inline? It would be useful for functions like context_switch, where we require it to be inlined (otherwise it falls outside scheduling_functions_{start,end}_here and wchan handling fails).
Ok. gcc supports it with __attribute__((always_inline))
Suggestions for a name? alwaysinline would be a bit lengthy.

do_inline?
Pavel

Signature · 2 lines

Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.