From: Peter Zijlstra <peterz@infradead.org>
To: Byungchul Park <byungchul.park@lge.com>
Cc: mingo@kernel.org, linux-kernel@vger.kernel.org, npiggin@suse.de,
sergey.senozhatsky@gmail.com, gregkh@linuxfoundation.org,
minchan@kernel.org
Subject: Re: [PATCH 1/5] lockdep: Implement bitlock map allocator
Date: Fri, 1 Jul 2016 09:53:12 +0200 [thread overview]
Message-ID: <20160701075312.GL30921@twins.programming.kicks-ass.net> (raw)
In-Reply-To: <20160701002444.GV2279@X58A-UD3R>
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.
next prev parent reply other threads:[~2016-07-01 7:53 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-20 4:55 [PATCH 0/5] Implement bitlock map allocator Byungchul Park
2016-06-20 4:55 ` [PATCH 1/5] lockdep: " Byungchul Park
2016-06-30 12:59 ` Peter Zijlstra
2016-07-01 0:24 ` Byungchul Park
2016-07-01 7:53 ` Peter Zijlstra [this message]
2016-07-04 7:29 ` Byungchul Park
2016-07-07 10:22 ` Byungchul Park
2016-07-13 20:17 ` Peter Zijlstra
2016-07-18 1:46 ` Byungchul Park
2016-06-20 4:55 ` [PATCH 2/5] lockdep: Apply bitlock to bit_spin_lock Byungchul Park
2016-06-20 4:55 ` [PATCH 3/5] lockdep: Apply bit_spin_lock lockdep to zram Byungchul Park
2016-06-20 15:36 ` Sergey Senozhatsky
2016-06-21 1:05 ` Byungchul Park
2016-06-20 4:55 ` [PATCH 4/5] fs/buffer.c: Remove trailing white space Byungchul Park
2016-06-20 4:55 ` [PATCH 5/5] lockdep: Apply bit_spin_lock lockdep to BH_Uptodate_Lock Byungchul Park
2016-06-29 12:45 ` [PATCH 0/5] Implement bitlock map allocator Byungchul Park
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160701075312.GL30921@twins.programming.kicks-ass.net \
--to=peterz@infradead.org \
--cc=byungchul.park@lge.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=minchan@kernel.org \
--cc=mingo@kernel.org \
--cc=npiggin@suse.de \
--cc=sergey.senozhatsky@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).