Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1434534 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/5] lockdep: Implement bitlock map allocator Peter Zijlstra <peterz@infradead.org> - 2016-06-30 15:00 +0200
    Re: [PATCH 1/5] lockdep: Implement bitlock map allocator Byungchul Park <byungchul.park@lge.com> - 2016-07-01 02:50 +0200
      Re: [PATCH 1/5] lockdep: Implement bitlock map allocator Peter Zijlstra <peterz@infradead.org> - 2016-07-01 10:00 +0200
        Re: [PATCH 1/5] lockdep: Implement bitlock map allocator Byungchul Park <byungchul.park@lge.com> - 2016-07-04 09:40 +0200
          Re: [PATCH 1/5] lockdep: Implement bitlock map allocator Byungchul Park <byungchul.park@lge.com> - 2016-07-07 12:30 +0200

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

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?


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

Now, I suppose its rather unlikely, but given its entirely silent if it
happens, this is bad.

[toc] | [next] | [standalone]

#1434933

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.

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.

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?

> 
> 
> > +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. IIUC, of course
it would be safer if lockdep_map is included in a structure statically.
However, as you know, sometimes dynamical allocating and connecting can
be more useful and worth under careful implementation. CONFIG_LOCKDEP
is even a debug feature where IMHO it's more important to implement
new value than to keep it very much safest, even though of course we
have to keep it as safe as possible.

> 
> Now, I suppose its rather unlikely, but given its entirely silent if it
> happens, this is bad.

[toc] | [prev] | [next] | [standalone]

#1435096

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.

[toc] | [prev] | [next] | [standalone]

#1436177

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.

[toc] | [prev] | [next] | [standalone]

#1438506

On Mon, Jul 04, 2016 at 04:29:25PM +0900, Byungchul Park wrote:
> > > 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?

I will stop it if it cannot provide any valuable things even I wonder.
I seriously asked it since I wonder it. What do you think about my
question? Is there something I missed?

Or can I proceed it after fixing my bug you pointed?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web