From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753084AbcGDHbO (ORCPT ); Mon, 4 Jul 2016 03:31:14 -0400 Received: from LGEAMRELO13.lge.com ([156.147.23.53]:46668 "EHLO lgeamrelo13.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750895AbcGDHbM (ORCPT ); Mon, 4 Jul 2016 03:31:12 -0400 X-Original-SENDERIP: 156.147.1.125 X-Original-MAILFROM: byungchul.park@lge.com X-Original-SENDERIP: 10.177.222.33 X-Original-MAILFROM: byungchul.park@lge.com Date: Mon, 4 Jul 2016 16:29:25 +0900 From: Byungchul Park To: Peter Zijlstra 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 Message-ID: <20160704072925.GY2279@X58A-UD3R> References: <1466398515-1005-1-git-send-email-byungchul.park@lge.com> <1466398515-1005-2-git-send-email-byungchul.park@lge.com> <20160630125919.GD30154@twins.programming.kicks-ass.net> <20160701002444.GV2279@X58A-UD3R> <20160701075312.GL30921@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160701075312.GL30921@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.