Re: [patch 5/9] mutex subsystem, core

From: Ingo Molnar
Date: Thu Dec 22 2005 - 07:52:34 EST



* Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:

> > +#include <linux/config.h>
>
> we don't need config.h anymore, it's included implicitly now.

thanks, fixed.

> > +#include <asm/atomic.h>
>
> Any chance we could include this after the <linux/*.h> headers ?

done.

> > +#include <linux/spinlock_types.h>
>
> What do we need this one for?

for:

spinlock_t wait_lock;

> > +struct mutex {
> > + // 1: unlocked, 0: locked, negative: locked, possible waiters
>
> please use /* */ comments.

done.

>
> > + atomic_t count;
> > + spinlock_t wait_lock;
> > + struct list_head wait_list;
> > +#ifdef CONFIG_DEBUG_MUTEXES
> > + struct thread_info *owner;
> > + struct list_head held_list;
> > + unsigned long acquire_ip;
> > + const char *name;
> > + void *magic;
> > +#endif
> > +};
>
> I know we generally don't like typedefs, but mutex is like spinlocks
> one of those cases where the internals should be completely opaqueue,
> so a mutex_t sounds like a good idea.

yeah, but we have DEFINE_MUTEX ...

> > +#include <linux/syscalls.h>
>
> What do you we need this header for?

correct, fixed.

> > +static inline void __mutex_lock_atomic(struct mutex *lock)
> > +{
> > +#ifdef __ARCH_WANT_XCHG_BASED_ATOMICS
> > + if (unlikely(atomic_xchg(&lock->count, 0) != 1))
> > + __mutex_lock_noinline(&lock->count);
> > +#else
> > + atomic_dec_call_if_negative(&lock->count, __mutex_lock_noinline);
> > +#endif
> > +}
>
> this is the kind of thing I meant in the comment to the announcement.

i've solved that via the CONFIG_MUTEX_XCHG_ALGORITHM switch. It's more
maintainable than 23 asm-*/mutex.h's.

> Just having this in arch code would kill all these ifdefs over mutex.c

it's exactly 3 #ifdefs.

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