Re: [slubllv7 04/17] x86: Add support for cmpxchg_double

[Christoph Lameter <cl@linux.com>]

On Wed, 15 Jun 2011, Tejun Heo wrote:

> > > > +#define system_has_cmpxchg_double() cpu_has_cx16
> > >
> > > Where's the fallback %false definition for the above feature macro for
> > > archs which don't support cmpxchg_double?  Also, is system_has_*()
> > > conventional?  Isn't arch_has_*() more conventional for this purpose?
> >
> > There is a convention for querying processor flags from core code?
>
> At least generic ptrace code uses arch_has_block/single_step().
> Probably better than introducing something completely new.

Its not a property of the arch that we are after. We need to know of the
hardware that is running the kernel (the system) has that capability.

> > The system_has_cmpxchg_double() is only used if the arch defines
> > CONFIG_CMPXCHG_DOUBLE
>
> Why?  What's the benefit of that?

You dont have to define system_has_cmpxchg_double() for systems without
CONFIG_CMPXCHG_DOUBLE.

> > This way it is done in the same way on 32 bit than on 64 bit. The use of
> > cmpxchg64 also means that some of the parameters would have to be combined
> > to form 64 bit ints from the 32 bit ones before __cmpxchg64 could be used.
> >
> > __cmpxchg64 has different parameter conventions.
>
> But they all just deal with the starting addresses and the _local
> version already has proper fallback implementation.

The local version is not that problematic since it is just provided for
completeness. If you want to go through the gyrations of providing a
conversion layer with casting and conversion between 32 and 64 bit
entities then please do so. But its not going to be nice.

> > The fallback through the subsystem means that the subsystem can do locking
> > that scales better. In the case of SLUB we fall back to a bit lock in the
> > page struct which is a hot cache line in the hotpaths. This is the same
> > approach as used before the lockless patches and we expect the performance
> > on platforms not supporting cmpxchg_double to stay the same.
>
> Yes, that's nice but you're introducing new operations and they should
> meet the usual conventions and cmpxchg fallback on the arch which I
> don't recall now already uses hashed lock so it's not like this is
> completely new.  As added, the interface basically requires extreme
> ifdeffery which isn't good.

I'd be glad if you could improve on it. But the fallback to a hashed lock
would also mean additional cache footprint which would not be acceptable
for the hotpaths that this is used for now.