Messages in this thread |  | | Date | Mon, 4 Jul 2016 16:29:25 +0900 | From | Byungchul Park <> | Subject | Re: [PATCH 1/5] lockdep: Implement bitlock map allocator |
| |
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.
|  |