thoughts on potential cleanup of semaphores?

thoughts on potential cleanup of semaphores?

[Robert P. J. Day]

  after submitting one patch related to semaphores and before i submit
any others, any thoughts whether any of the following clean-ups are
valid and/or worthwhile?  (some are admittedly simply aesthetic but
better aesthetics is never a bad thing.)


1) can all instances of sema_init() in the header files be simplified
based on the comment you can see in some of those header files?

========================
static inline void sema_init (struct semaphore *sem, int val)
{
/*
 *      *sem = (struct semaphore)__SEMAPHORE_INITIALIZER((*sem),val);
 *
 * i'd rather use the more flexible initialization above, but sadly
 * GCC 2.7.2.3 emits a bogus warning. EGCS doesnt. Oh well.
 */
        atomic_set(&sem->count, val);
        sem->sleepers = 0;
        init_waitqueue_head(&sem->wait);
}
========================

  one would think there's little value in retaining code that
accommodates something as old as GCC 2.7.2.3, but i'm not the expert
here.


2) [aesthetic] update all __SEMAPHORE_INITIALIZER initialization to
use C99-style structure initializers


3) i'm not a multi-arch wizard so is it true that all architectures
should have a struct semaphore with (approximately) the following
basic structure defined in their semaphore.h?

========================
struct semaphore {
        atomic_t count;
        int sleepers;
        wait_queue_head_t wait;
};
=======================

  you'd think so but there are some discrepancies.  in asm-frv, you
have

struct semaphore {
        unsigned                counter;
        spinlock_t              wait_lock;
        struct list_head        wait_list;

why "unsigned" and not "atomic_t"?  and why "counter" and not just
"count"?  (although, for the frv arch, that variable appears to be
unused except in a single place for debugging.)  and why a "struct
list_head" rather than what everyone else uses, a "wait_queue_head_t"?

i also note that some arches (m68knommu, m68k, cris) define a
"waking" variable instead of "sleepers".  is this supposed to
represent the same thing?  i haven't looked closely enough, sorry.


  and stepping back and looking at the bigger picture, it seems that
the semaphore.h files across all architectures are *almost* identical,
with a small number of differences, such as:

* some take a spinlock
* one (sparc) uses ATOMIC24_INIT rather than just ATOMIC_INIT

and there's probably a couple other things but, if these header files
are so similar, what about defining a single, generic semaphore.h
header file with a couple #ifdef's to handle the few possible
architecture-specific differences?  superficially, it doesn't look
like it would be that hard but that's the voice of youthful enthusiasm
speaking.

rday

p.s.  trying to condense all of the separate semaphore.h files into a
single, configurable one would also solve the problem of incorrect
documentation in some of them that is clearly the result of
cut-and-paste.  but i'm interested in what the experts have to say.

Re: thoughts on potential cleanup of semaphores?

[Steven Rostedt]

On Tue, 2006-10-10 at 12:00 -0400, Robert P. J. Day wrote:
>   after submitting one patch related to semaphores and before i submit
> any others, any thoughts whether any of the following clean-ups are
> valid and/or worthwhile?  (some are admittedly simply aesthetic but
> better aesthetics is never a bad thing.)
> 
> 
> 1) can all instances of sema_init() in the header files be simplified
> based on the comment you can see in some of those header files?
> 
> ========================
> static inline void sema_init (struct semaphore *sem, int val)
> {
> /*
>  *      *sem = (struct semaphore)__SEMAPHORE_INITIALIZER((*sem),val);
>  *
>  * i'd rather use the more flexible initialization above, but sadly
>  * GCC 2.7.2.3 emits a bogus warning. EGCS doesnt. Oh well.
>  */
>         atomic_set(&sem->count, val);
>         sem->sleepers = 0;
>         init_waitqueue_head(&sem->wait);
> }
> ========================
> 
>   one would think there's little value in retaining code that
> accommodates something as old as GCC 2.7.2.3, but i'm not the expert
> here.

Well, I believe it's official, that we don't support GCC 2.7.2.3 anymore
anyway.

> 
> 
> 2) [aesthetic] update all __SEMAPHORE_INITIALIZER initialization to
> use C99-style structure initializers
> 
> 
> 3) i'm not a multi-arch wizard so is it true that all architectures
> should have a struct semaphore with (approximately) the following
> basic structure defined in their semaphore.h?
> 
> ========================
> struct semaphore {
>         atomic_t count;
>         int sleepers;
>         wait_queue_head_t wait;
> };
> =======================
> 
>   you'd think so but there are some discrepancies.  in asm-frv, you
> have
> 
> struct semaphore {
>         unsigned                counter;
>         spinlock_t              wait_lock;
>         struct list_head        wait_list;
> 
> why "unsigned" and not "atomic_t"?  and why "counter" and not just
> "count"?  (although, for the frv arch, that variable appears to be
> unused except in a single place for debugging.)  and why a "struct
> list_head" rather than what everyone else uses, a "wait_queue_head_t"?
> 
> i also note that some arches (m68knommu, m68k, cris) define a
> "waking" variable instead of "sleepers".  is this supposed to
> represent the same thing?  i haven't looked closely enough, sorry.
> 
> 
>   and stepping back and looking at the bigger picture, it seems that
> the semaphore.h files across all architectures are *almost* identical,
> with a small number of differences, such as:

All those asm statements :-)

> 
> * some take a spinlock
> * one (sparc) uses ATOMIC24_INIT rather than just ATOMIC_INIT
> 
> and there's probably a couple other things but, if these header files
> are so similar, what about defining a single, generic semaphore.h
> header file with a couple #ifdef's to handle the few possible
> architecture-specific differences?  superficially, it doesn't look
> like it would be that hard but that's the voice of youthful enthusiasm
> speaking.
> 
> rday
> 
> p.s.  trying to condense all of the separate semaphore.h files into a
> single, configurable one would also solve the problem of incorrect
> documentation in some of them that is clearly the result of
> cut-and-paste.  but i'm interested in what the experts have to say.

But the meat in those files are in the down and up functions. Which are
all pretty much drastically different.  All you would accomplish is some
standard initialization of the structure and sema_init.

-- Steve

Re: thoughts on potential cleanup of semaphores?

[Robert P. J. Day]

On Tue, 10 Oct 2006, Steven Rostedt wrote:

>
> On Tue, 2006-10-10 at 12:00 -0400, Robert P. J. Day wrote:

> > 1) can all instances of sema_init() in the header files be simplified
> > based on the comment you can see in some of those header files?
> >
> > ========================
> > static inline void sema_init (struct semaphore *sem, int val)
> > {
> > /*
> >  *      *sem = (struct semaphore)__SEMAPHORE_INITIALIZER((*sem),val);
> >  *
> >  * i'd rather use the more flexible initialization above, but sadly
> >  * GCC 2.7.2.3 emits a bogus warning. EGCS doesnt. Oh well.
> >  */
> >         atomic_set(&sem->count, val);
> >         sem->sleepers = 0;
> >         init_waitqueue_head(&sem->wait);
> > }
> > ========================
> >
> >   one would think there's little value in retaining code that
> > accommodates something as old as GCC 2.7.2.3, but i'm not the expert
> > here.
>
> Well, I believe it's official, that we don't support GCC 2.7.2.3 anymore
> anyway.


ok, so a patch to simplify *all* of the above implementations of
sema_init() to a single call to __SEMAPHORE_INITIALIZER() would not be
out of line.  i'll resubmit my earlier one, making sure it's based on
the latest "git pull".


> >   and stepping back and looking at the bigger picture, it seems that
> > the semaphore.h files across all architectures are *almost* identical,
> > with a small number of differences, such as:
>
> All those asm statements :-)


well, ok, i worded that badly.  let me try again.  at least the
semaphore *interfaces* across all architectures seem to be almost
identical, with the exception, of course, of the inline "asm"
routines.  but (and i know this idea is not going *anywhere*) when a
routine in a header file is defined as inline and consists of a dozen
assembler statements, wouldn't it make more sense to (dons asbestos
suit here) have those routines be part of the *source* file and not
the header file?

i'm probably making more of this than it's worth but, as i was digging
around in the semaphore implementations, it struck me that there was a
lot of unnecessary duplication across the numerous semaphore.h files.
is there no reasonable way to get rid of at least *some* of that?


> > p.s.  trying to condense all of the separate semaphore.h files
> > into a single, configurable one would also solve the problem of
> > incorrect documentation in some of them that is clearly the result
> > of cut-and-paste.  but i'm interested in what the experts have to
> > say.
>
> But the meat in those files are in the down and up functions. Which
> are all pretty much drastically different.  All you would accomplish
> is some standard initialization of the structure and sema_init.


so perhaps the up and down functions represent the stuff that can be
kept in arch-specific files, while the rest of semaphore.h is
condensed to a single, cross-arch file.  it still seems like there's
lots of redundancy that can be eliminated here with very little
effort.

in defense of minimalism,
rday

Re: thoughts on potential cleanup of semaphores?

[Steven Rostedt]

On Wed, 11 Oct 2006, Robert P. J. Day wrote:
>
> > >   and stepping back and looking at the bigger picture, it seems that
> > > the semaphore.h files across all architectures are *almost* identical,
> > > with a small number of differences, such as:
> >
> > All those asm statements :-)
>
>
> well, ok, i worded that badly.  let me try again.  at least the
> semaphore *interfaces* across all architectures seem to be almost
> identical, with the exception, of course, of the inline "asm"
> routines.  but (and i know this idea is not going *anywhere*) when a
> routine in a header file is defined as inline and consists of a dozen
> assembler statements, wouldn't it make more sense to (dons asbestos
> suit here) have those routines be part of the *source* file and not
> the header file?

Not when you need them to be as fast as possible. Although, with the
introduction of the linux mutex, these are not used nearly as much.

So, for speed, the down and up are "inlined" into the code, so we
have them in a header file to be inlined everywhere.


>
> i'm probably making more of this than it's worth but, as i was digging
> around in the semaphore implementations, it struck me that there was a
> lot of unnecessary duplication across the numerous semaphore.h files.
> is there no reasonable way to get rid of at least *some* of that?

Maybe, see below (but beware it may be some tedious work ahead).

>
>
> > > p.s.  trying to condense all of the separate semaphore.h files
> > > into a single, configurable one would also solve the problem of
> > > incorrect documentation in some of them that is clearly the result
> > > of cut-and-paste.  but i'm interested in what the experts have to
> > > say.
> >
> > But the meat in those files are in the down and up functions. Which
> > are all pretty much drastically different.  All you would accomplish
> > is some standard initialization of the structure and sema_init.
>
>
> so perhaps the up and down functions represent the stuff that can be
> kept in arch-specific files, while the rest of semaphore.h is
> condensed to a single, cross-arch file.  it still seems like there's
> lots of redundancy that can be eliminated here with very little
> effort.
>
> in defense of minimalism,

I appreciate what you're trying to do. But it becomes a bigger mess
sometimes when you break up a asm file and do a "do generic" if you are
not doing *everything* generic.  If all of semaphore.h was the same in
most of the archs, then that would be the right thing to do.  But when you
only need a few things from the "generic" version then you get into #ifdef
hell.

So I just looked at a couple of semaphore.h's in sparc, frv and m32r,
here's the top of m32r:

-----
struct semaphore {
	atomic_t count;
	int sleepers;
	wait_queue_head_t wait;
};

#define __SEMAPHORE_INITIALIZER(name, n)				\
{									\
	.count		= ATOMIC_INIT(n),				\
	.sleepers	= 0,						\
	.wait		= __WAIT_QUEUE_HEAD_INITIALIZER((name).wait)	\
}

#define __DECLARE_SEMAPHORE_GENERIC(name,count) \
	struct semaphore name = __SEMAPHORE_INITIALIZER(name,count)

#define DECLARE_MUTEX(name) __DECLARE_SEMAPHORE_GENERIC(name,1)
#define DECLARE_MUTEX_LOCKED(name) __DECLARE_SEMAPHORE_GENERIC(name,0)

static inline void sema_init (struct semaphore *sem, int val)
{
/*
 *	*sem = (struct semaphore)__SEMAPHORE_INITIALIZER((*sem),val);
 *
 * i'd rather use the more flexible initialization above, but sadly
 * GCC 2.7.2.3 emits a bogus warning. EGCS doesnt. Oh well.
 */
	atomic_set(&sem->count, val);
	sem->sleepers = 0;
	init_waitqueue_head(&sem->wait);
}

static inline void init_MUTEX (struct semaphore *sem)
{
	sema_init(sem, 1);
}

static inline void init_MUTEX_LOCKED (struct semaphore *sem)
{
	sema_init(sem, 0);
}

asmlinkage void __down_failed(void /* special register calling convention */);
asmlinkage int  __down_failed_interruptible(void  /* params in registers */);
asmlinkage int  __down_failed_trylock(void  /* params in registers */);
asmlinkage void __up_wakeup(void /* special register calling convention */);

asmlinkage void __down(struct semaphore * sem);
asmlinkage int  __down_interruptible(struct semaphore * sem);
asmlinkage int  __down_trylock(struct semaphore * sem);
asmlinkage void __up(struct semaphore * sem);
----

Of that, all I can see in a "generic" header, or just linux/semaphore.h,
would be:

#define __DECLARE_SEMAPHORE_GENERIC(name,count) \
	struct semaphore name = __SEMAPHORE_INITIALIZER(name,count)

#define DECLARE_MUTEX(name) __DECLARE_SEMAPHORE_GENERIC(name,1)
#define DECLARE_MUTEX_LOCKED(name) __DECLARE_SEMAPHORE_GENERIC(name,0)

and

static inline void init_MUTEX (struct semaphore *sem)
{
	sema_init(sem, 1);
}

static inline void init_MUTEX_LOCKED (struct semaphore *sem)
{
	sema_init(sem, 0);
}


Anything else would take the work of understanding all the archs that are
different.  So if you want to make it a little more standard you need to
do the following: (not promising that this will be accepted)

1. Make a linux/semaphore.h header that includes <asm/semaphore.h>

2. change all drivers and code that currently includes <asm/semaphore.h>
   to <linux/semaphore.h>

3. remove all the same things out of each arch asm/semaphore.h and place
   it into your new linux/semaphore.h

  $ find linux-2.6.18 -name '*.[ch]' | xargs grep '<asm/semaphore.h>' |wc -l
  204

    That's 204 files to be changed.

4. Go to each of the arch maintainers and convince them to change their
   code to be like the majority of the other code (good luck).
   Supply patches to do this for them and ask them what's wrong with your
   patches.

5. Once there is something else that is completely the same on all archs,
   remove it from the arch asm/semaphore.h's and place it in the
   linux/semaphore.h

Perhaps you could do all of the above in a separate patch for each step,
then submit them (to get attention, also CC the linux-arch@vger.kernel.org)
Then be prepared to argue and fight.  These patches might be considered
nice clean ups and accepted friendly, but there might be a arch maintainer
that really has some reason they did it the way they did and it only makes
sense to those that understand that arch.

But, by no means, add any more #ifdefs! Code is much cleaner without those
pesky compiler preprocessors around.  And if this is to be a clean up
patch, it had better not be adding any.

Just some thoughts.

-- Steve

Re: thoughts on potential cleanup of semaphores?

[Andi Kleen]

"Robert P. J. Day" <rpjday@mindspring.com> writes:

>   after submitting one patch related to semaphores and before i submit
> any others, any thoughts whether any of the following clean-ups are
> valid and/or worthwhile?  (some are admittedly simply aesthetic but
> better aesthetics is never a bad thing.)

Semaphores and rwsems need a lot of cleanup. Some time ago we put
spinlocks which are a lot more time critical than semaphores
out of line. So the same could be done for semaphores too. This
had the advantages that most of the assembler voodoo wouldn't
be needed anymore and they could be simple straight C implementations
shared by all architectures.

-Andi