Re: thoughts on potential cleanup of semaphores?

[Steven Rostedt <rostedt@goodmis.org>]

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