linux-kernel - Re: [PATCH] FRV: Fix the section attribute on UP DECLARE_PER

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090415120307.GB17775@elte.hu>
Date:	Wed, 15 Apr 2009 14:03:07 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	David Howells <dhowells@...hat.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>, tj@...nel.org,
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] FRV: Fix the section attribute on UP DECLARE_PER_CPU()

* David Howells <dhowells@...hat.com> wrote:

> Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> > But yeah, I didn't look at all the details. It _looked_ pretty 
> > straightforward to just move the DEFINE/DECLARE stuff up, but 
> > there may well be something subtle I'm missing.
> 
> The problem is mainly one of #include recursion.  There's way too 
> much of it.

that should be mapped and eliminated then.

> I wonder if we should replace the standard headerfile boilerplate:
> 
> 	#ifndef _THIS_HEADER_H
> 	#define _THIS_HEADER_H
> 	...
> 	#endif /* _THIS_HEADER_H */
> 
> with something a bit nastier:
> 
> 	#ifndef _THIS_HEADER_H
> 	#define _THIS_HEADER_H 1
> 	...
> 	#undef _THIS_HEADER_H
> 	#define _THIS_HEADER_H 2
> 	#elif  _THIS_HEADER_H == 1
> 	#error Recursive inclusion is not permitted
> 	#endif /* _THIS_HEADER_H */
> 
> and make people break up their header files to avoid getting this 
> error.
> 
> There are a number of problems with doing this, of course; the 
> least of which is that cpp has special code for handling the first 
> case efficiently, IIRC.

The other problem with your scheme is that this does not make much 
sense.

There is absolutely _nothing_ wrong about defining structures in a 
hierarchical way! For example task_struct does have to combine a lot 
of derived types. It can use types directly and indirectly as well. 
Hierarchical data structures are _good_.

What _is_ wrong here is to allow inline functions _combine_ 
unrelated types randomly and 'mix up' an otherwise clean and logical 
hierarchy of data types.

The solution to this is the creation of a separate data versus 
method include file hiearchy: for example mm_types.h and 
spinlock_types.h.

Without having looked very deep into this particular problem you are 
hitting, this seems to be the problem here too.

There's three too 'thick' headers: linux/percpu.h, linux/prefetch.h 
and asm/processor.h.

include/linux/percpu.h, which provides essential data types and core 
primitives, also includes:

#include <linux/preempt.h>
#include <linux/slab.h> /* For kmalloc() */
#include <linux/smp.h>
#include <linux/cpumask.h>
#include <linux/pfn.h>

Just to be able to define an inline method:

static inline void *__alloc_percpu(size_t size, size_t align)
{
        /*
         * Can't easily make larger alignment work with kmalloc.  WARN
         * on it.  Larger alignment should only be used for module
         * percpu sections on SMP for which this path isn't used.
         */
        WARN_ON_ONCE(align > SMP_CACHE_BYTES);
        return kzalloc(size, GFP_KERNEL);
}

Tada - now we've the combined 'percpu' and 'slab' spaces, on a very 
low level! These kinds of cross-dependencies quickly explode and 
create circular dependencies the moment you try to include any of 
this supposedly-simple header below the 'level' of slab.

The solution for this one?

Please create include/linux/percpu_types.h for basic data types and 
simple, self-sufficient primitives. Also have an 
include/linux/percpu_api.h or include/linux/percpu.h include file 
for convenience/speedup inlines. The latter will only be included in 
.c files, where 'combination' of type spaces is not a problem. (it 
is a virtue there.)

We did this in a number of other cases, and the concept works well. 
There are a number of details along the way, and a lot of build 
breakages and wide changes to overcome - but once the basics are in 
place that is mostly a mechanic, machine-helped spreading of the 
concept, not fragile or troublesome in any way.

The other problem is processor.h:

#include <asm/vm86.h>
#include <asm/math_emu.h>
#include <asm/segment.h>
#include <asm/types.h>
#include <asm/sigcontext.h>
#include <asm/current.h>
#include <asm/cpufeature.h>
#include <asm/system.h>
#include <asm/page.h>
#include <asm/pgtable_types.h>
#include <asm/percpu.h>
#include <asm/msr.h>
#include <asm/desc_defs.h>
#include <asm/nops.h>
#include <asm/ds.h>

#include <linux/personality.h>
#include <linux/cpumask.h>
#include <linux/cache.h>
#include <linux/threads.h>
#include <linux/init.h>

That too seems quite wide. linux/prefetch.h as well:

#include <linux/types.h>
#include <asm/processor.h>
#include <asm/cache.h>

It brings in processor.h which brings in a lot of other 
dependencies. For a supposedly simple-looking primitive - which is 
used in an inline function in processor.h.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/