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

[Byungchul Park]

On Fri, Jul 01, 2016 at 09:53:12AM +0200, Peter Zijlstra wrote:
> 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.

I agree it has some problems. But someone who are sensive to memory
consumption still need to use bit-based lock. Right?

I can stop this proposal because it's meaningless if bit-based lock can be
removed entirely since any requirement for bit-based lock does not exist
at all. But IMHO, it's worthy if the requirement be.

> 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.

I also think it's better to use rather spinlock in most cases unless memory
consumption is critical problem. But in the case memory consumption is
critical... what can we do?

> 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!

You mean we should never provide lockdep checking mechanism tranparently,
but the user of bit-based lock must add lockdep_map manually, case by
case. Right? Do I understand correctly? If so, I wonder why?

> > > > +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.

OOPS! What a fool I was. That's my mistake. I can fix it. Sorry.