Re: [PATCH 1/5] lockdep: Implement bitlock map allocator

[Peter Zijlstra]

On Fri, Jul 01, 2016 at 09:24:44AM +0900, Byungchul Park wrote:
> On Thu, Jun 30, 2016 at 02:59:19PM +0200, Peter Zijlstra wrote:
> > On Mon, Jun 20, 2016 at 01:55:11PM +0900, Byungchul Park wrote:
> > 
> > > +struct bitlock_map {
> > > +	struct hlist_node	hash_entry;
> > > +	unsigned long		bitaddr; /* ID */
> > > +	struct lockdep_map	map;
> > > +	int			ref; /* reference count */
> > > +};
> > 
> > So this is effectively bigger than just adding a struct lockdep_map into
> > whatever structure holds the bit spinlock to begin with.
> > 
> > What is the gain?
> 
> 1. I don't want to make being aware of lockdep essential to user of
>    bit-base lock, like spin lock, mutex, semaphore ans so on. In other
>    words, I want to make it work transparently.

I want to discourage the use of bitlocks, they stink.

bitlocks must by their constraint be a test-and-set lock, with all the
known problems those have. It also means they're a royal pain for -rt.

Yes, there are a number of places we use them, but people should think
very carefully before they use them and consider all these issues. But
the problem seems to be that people aren't even aware there's problems.

> 2. Bit-base lock can be used with any data type which can be seperately,
>    not within a structure. I mean sometimes it can be ugly to pack the
>    lock bit and lockdep_map instance explicitly.

Yuck, people do this?

> 3. I think this is more general approach because _any_ random bit in 
>    memory can be used as a lock. Do we need to restrict where the bit
>    is so that we can place lockdep_map explicitly around the bit?

Again, yuck!

In any case, that would have made great Changelog material.

> > > +static inline unsigned long get_bitaddr(int bitnum, unsigned long *addr)
> > > +{
> > > +	return (unsigned long)((char *)addr + bitnum);
> > > +}
> > 
> > And given you keep these lockdep_map thingies out-of-line, the original
> > structure remains dense and thus the above munging can easily result in
> > collisions.
> 
> I am sorry. I don't understand what you said exactly. 


#define FOO_FLAG_LOCK	8

struct foo {
	struct hlist_bl_head head;
	unsigned long flags;
};

struct foo bar[];


That structure has 2 bitlocks in, one at:

  0 bytes + 0 bits
  8 bytes + 8 bits

In this case:

  get_bitaddr(8, &bar[0].flags) == get_bitaddr(0, &bar[1].head)

Which is a collision and fail, because they're two different lock
classes.