Re: [RFC local_t removal V1 1/4] Add add_local() and add_local_return()

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 05 January 2010, Mathieu Desnoyers wrote:
> 
> The problem I see here is that with ~5-6 operations, we will end up
> having 20*5 = 100 headers only for this. Can we combine these in a
> single header file instead ? local.h wasn't bad in this respect.

I have an old patch that I was planning to dig out for 2.6.34,
which autogenerates arch/*/include/foo.h files that only contain
"#include <asm-generic/foo.h>".

I guess this would be sufficient to avoid the overload with all
these header files.

> Also, separating all these in sub-files will make it a bit difficult to
> pinpoint errors that would arise from using a bad combination of, e.g.
> generic add with a non-protected read or set.

Yes.

> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6/include/asm-generic/add-local-generic.h	2010-01-05 15:36:02.000000000 -0600
> > @@ -0,0 +1,40 @@
> > +#ifndef __ASM_GENERIC_ADD_LOCAL_GENERIC_H
> > +#define __ASM_GENERIC_ADD_LOCAL_GENERIC_H

Why split the file between asm-generic/add-local.h and add-local-generic.h?
I don't see how any architecture could use one but not the other.

> > +#include <linux/types.h>
> > +
> > +extern unsigned long wrong_size_add_local(volatile void *ptr);
> > +
> > +/*
> > + * Generic version of __add_return_local (disables interrupts). Takes an
> > + * unsigned long parameter, supporting various types of architectures.
> > + */
> > +static inline unsigned long __add_return_local_generic(volatile void *ptr,
> > +		unsigned long value, int size)

You could probably lose the 'volatile' here, if you want to discourage
marking data as volatile in the code.

> > +{
> > +	unsigned long flags, r;
> > +
> > +	/*
> > +	 * Sanity checking, compile-time.
> > +	 */
> > +	if (size == 8 && sizeof(unsigned long) != 8)
> > +		wrong_size_add_local(ptr);

It can be BUILD_BUG_ON if you move it to the outer macro.

> > +	local_irq_save(flags);
> > +	switch (size) {
> > +	case 1: r = (*((u8 *)ptr) += value);
> > +		break;
> > +	case 2: r = (*((u16 *)ptr) += value);
> > +		break;
> > +	case 4: r = (*((u32 *)ptr) += value);
> > +		break;
> > +	case 8: r = (*((u64 *)ptr) += value);
> > +		break;

But I think here you actually need to add the volatile in order
to make these atomic assignments.

	Arnd