[PATCH] MIPS: Implement __read_mostly

[PATCH] MIPS: Implement __read_mostly

[David Daney]

Just do what everyone else is doing by placing __read_mostly things in
the .data.read_mostly section.

mips_io_port_base can not be read-only (const) and writable
(__read_mostly) at the same time.  One of them has to go, so I chose
to eliminate the __read_mostly.  It will still get stuck in a portion
of memory that is not adjacent to things that are written, and thus
not be on a dirty cache line, for whatever that is worth.

Signed-off-by: David Daney <ddaney@caviumnetworks.com>
---
 arch/mips/include/asm/cache.h |    2 ++
 arch/mips/kernel/setup.c      |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/mips/include/asm/cache.h b/arch/mips/include/asm/cache.h
index 37f175c..650ac9b 100644
--- a/arch/mips/include/asm/cache.h
+++ b/arch/mips/include/asm/cache.h
@@ -17,4 +17,6 @@
 #define SMP_CACHE_SHIFT		L1_CACHE_SHIFT
 #define SMP_CACHE_BYTES		L1_CACHE_BYTES
 
+#define __read_mostly __attribute__((__section__(".data.read_mostly")))
+
 #endif /* _ASM_CACHE_H */
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 4e68a51..6d0c3be 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -69,7 +69,7 @@ static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
  * mips_io_port_base is the begin of the address space to which x86 style
  * I/O ports are mapped.
  */
-const unsigned long mips_io_port_base __read_mostly = -1;
+const unsigned long mips_io_port_base = -1;
 EXPORT_SYMBOL(mips_io_port_base);
 
 static struct resource code_resource = { .name = "Kernel code", };
-- 
1.7.2.3

Re: [PATCH] MIPS: Implement __read_mostly

[Gleb O. Raiko]

On 14.10.2010 23:36, David Daney wrote:
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -69,7 +69,7 @@ static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
>    * mips_io_port_base is the begin of the address space to which x86 style
>    * I/O ports are mapped.
>    */
> -const unsigned long mips_io_port_base __read_mostly = -1;
> +const unsigned long mips_io_port_base = -1;
>   EXPORT_SYMBOL(mips_io_port_base);

While we're here could we eliminate mips_io_port_base for boards that 
don't need it.

Now, it almost might be done by defining something like
__swizzle_addr_[bwlq](port)	((port) - mips_io_port_base)

Unfortunately, __swizzle_addr_[bwlq] is also used for memory acceeses 
too (in read/write[bwlq]), so definition above doesn't work.

By providing two variants, e.g. __swizzle_io_addr_[bwlq] and 
__swizzle_mem_addr_[bwlq] we can eliminate unnecessary loads of 
mips_io_port_base.

BTW, in recent kernels the trick with mips_io_port_base doesn't work 
well anyway. The solely purpose of the trick was to prevent loading of 
mips_io_port_base across function calls. Now drivers tend to use 
ioread/iowrite that don't use mips_io_port_base at all or use its own 
wrappers for in/out[bwlq] that _do_ load mips_io_port_base on every call.

Gleb.

Re: [PATCH] MIPS: Implement __read_mostly

[David Daney]

On 10/15/2010 04:25 AM, Gleb O. Raiko wrote:
> On 14.10.2010 23:36, David Daney wrote:
>> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
>> --- a/arch/mips/kernel/setup.c
>> +++ b/arch/mips/kernel/setup.c
>> @@ -69,7 +69,7 @@ static char __initdata
>> builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
>> * mips_io_port_base is the begin of the address space to which x86 style
>> * I/O ports are mapped.
>> */
>> -const unsigned long mips_io_port_base __read_mostly = -1;
>> +const unsigned long mips_io_port_base = -1;
>> EXPORT_SYMBOL(mips_io_port_base);
>
> While we're here could we eliminate mips_io_port_base for boards that
> don't need it.
>

That is a logically separate issue, so would be the subject of a 
different patch.  I don't personally plan on working on it, but would be 
happy to review anything someone else came up with.

David Daney


> Now, it almost might be done by defining something like
> __swizzle_addr_[bwlq](port) ((port) - mips_io_port_base)
>
> Unfortunately, __swizzle_addr_[bwlq] is also used for memory acceeses
> too (in read/write[bwlq]), so definition above doesn't work.
>
> By providing two variants, e.g. __swizzle_io_addr_[bwlq] and
> __swizzle_mem_addr_[bwlq] we can eliminate unnecessary loads of
> mips_io_port_base.
>
> BTW, in recent kernels the trick with mips_io_port_base doesn't work
> well anyway. The solely purpose of the trick was to prevent loading of
> mips_io_port_base across function calls. Now drivers tend to use
> ioread/iowrite that don't use mips_io_port_base at all or use its own
> wrappers for in/out[bwlq] that _do_ load mips_io_port_base on every call.
>
> Gleb.
>

Re: [PATCH] MIPS: Implement __read_mostly

[Geert Uytterhoeven]

On Thu, Oct 14, 2010 at 21:36, David Daney <ddaney@caviumnetworks.com> wrote:
> Just do what everyone else is doing by placing __read_mostly things in
> the .data.read_mostly section.
>
> mips_io_port_base can not be read-only (const) and writable
> (__read_mostly) at the same time.  One of them has to go, so I chose
> to eliminate the __read_mostly.  It will still get stuck in a portion
> of memory that is not adjacent to things that are written, and thus
> not be on a dirty cache line, for whatever that is worth.
>
> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
> ---
>  arch/mips/include/asm/cache.h |    2 ++
>  arch/mips/kernel/setup.c      |    2 +-
>  2 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/arch/mips/include/asm/cache.h b/arch/mips/include/asm/cache.h
> index 37f175c..650ac9b 100644
> --- a/arch/mips/include/asm/cache.h
> +++ b/arch/mips/include/asm/cache.h
> @@ -17,4 +17,6 @@
>  #define SMP_CACHE_SHIFT                L1_CACHE_SHIFT
>  #define SMP_CACHE_BYTES                L1_CACHE_BYTES
>
> +#define __read_mostly __attribute__((__section__(".data.read_mostly")))
> +
>  #endif /* _ASM_CACHE_H */
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 4e68a51..6d0c3be 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -69,7 +69,7 @@ static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
>  * mips_io_port_base is the begin of the address space to which x86 style
>  * I/O ports are mapped.
>  */
> -const unsigned long mips_io_port_base __read_mostly = -1;
> +const unsigned long mips_io_port_base = -1;
>  EXPORT_SYMBOL(mips_io_port_base);

Ugh. So as soon as someone implements MMU protection for the read-only data
section, it'll break silently?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] MIPS: Implement __read_mostly

[Geert Uytterhoeven]

On Sat, Oct 16, 2010 at 10:04, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Oct 14, 2010 at 21:36, David Daney <ddaney@caviumnetworks.com> wrote:
>> Just do what everyone else is doing by placing __read_mostly things in
>> the .data.read_mostly section.
>>
>> mips_io_port_base can not be read-only (const) and writable
>> (__read_mostly) at the same time.  One of them has to go, so I chose
>> to eliminate the __read_mostly.  It will still get stuck in a portion
>> of memory that is not adjacent to things that are written, and thus
>> not be on a dirty cache line, for whatever that is worth.
>>
>> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
>> ---
>>  arch/mips/include/asm/cache.h |    2 ++
>>  arch/mips/kernel/setup.c      |    2 +-
>>  2 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/mips/include/asm/cache.h b/arch/mips/include/asm/cache.h
>> index 37f175c..650ac9b 100644
>> --- a/arch/mips/include/asm/cache.h
>> +++ b/arch/mips/include/asm/cache.h
>> @@ -17,4 +17,6 @@
>>  #define SMP_CACHE_SHIFT                L1_CACHE_SHIFT
>>  #define SMP_CACHE_BYTES                L1_CACHE_BYTES
>>
>> +#define __read_mostly __attribute__((__section__(".data.read_mostly")))
>> +
>>  #endif /* _ASM_CACHE_H */
>> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
>> index 4e68a51..6d0c3be 100644
>> --- a/arch/mips/kernel/setup.c
>> +++ b/arch/mips/kernel/setup.c
>> @@ -69,7 +69,7 @@ static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
>>  * mips_io_port_base is the begin of the address space to which x86 style
>>  * I/O ports are mapped.
>>  */
>> -const unsigned long mips_io_port_base __read_mostly = -1;
>> +const unsigned long mips_io_port_base = -1;
>>  EXPORT_SYMBOL(mips_io_port_base);
>
> Ugh. So as soon as someone implements MMU protection for the read-only data
> section, it'll break silently?

Sorry, not silently. It'll panic ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] MIPS: Implement __read_mostly

[Ralf Baechle]

On Sat, Oct 16, 2010 at 10:04:59AM +0200, Geert Uytterhoeven wrote:

> > -const unsigned long mips_io_port_base __read_mostly = -1;
> > +const unsigned long mips_io_port_base = -1;
> >  EXPORT_SYMBOL(mips_io_port_base);
> 
> Ugh. So as soon as someone implements MMU protection for the read-only data
> section, it'll break silently?

That's not the only failure mode.  A const might be replicated by a compiler
for example into multiple small data sections or by loading the rodata
into multiple NUMA nodes.  The later hits IP27 but that one got potencially
many PCI busses anyway, so mips_io_port_base was always problematic.  From
the time I put this optimization hack in it was clear that I was gaming
GCC and sooner or later it was going to blow up.  It's held for like a
decade so I got my money's worth :)

  Ralf

Re: [PATCH] MIPS: Implement __read_mostly

[Geert Uytterhoeven]

On Wed, Oct 20, 2010 at 21:05, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Sat, Oct 16, 2010 at 10:04:59AM +0200, Geert Uytterhoeven wrote:
>> > -const unsigned long mips_io_port_base __read_mostly = -1;
>> > +const unsigned long mips_io_port_base = -1;
>> >  EXPORT_SYMBOL(mips_io_port_base);
>>
>> Ugh. So as soon as someone implements MMU protection for the read-only data
>> section, it'll break silently?
>
> That's not the only failure mode.  A const might be replicated by a compiler
> for example into multiple small data sections or by loading the rodata
> into multiple NUMA nodes.  The later hits IP27 but that one got potencially

Didn't think about that. Ough...

> many PCI busses anyway, so mips_io_port_base was always problematic.  From
> the time I put this optimization hack in it was clear that I was gaming
> GCC and sooner or later it was going to blow up.  It's held for like a
> decade so I got my money's worth :)

Or it may fool "smart" debuggers, who read the data from the original image
instead of from memory. Once we had a buffer overflow corrupting subsequent
read-only data on a VxWorks platform, but the debugger didn't see it
as it refused to
look at the actual data in RAM...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] MIPS: Implement __read_mostly

[Ralf Baechle]

Thanks, merged.

Last patch for today, the swimming pool chlorine is making my eye want to
pop out ...

  Ralf