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

[Ingo Molnar <mingo@elte.hu>]

* Christoph Hellwig <hch@infradead.org> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/