All of lore.kernel.org
 help / color / mirror / Atom feed
From: Steven Rostedt <srostedt@redhat.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Steven Rostedt <rostedt@goodmis.org>,
	Jeremy Fitzhardinge <jeremy@goop.org>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	David Miller <davem@davemloft.net>,
	hugh@veritas.com, mingo@elte.hu, akpm@linux-foundation.org,
	linux-kernel@vger.kernel.org, davej@redhat.com
Subject: Re: [RFC][PATCH 7/7] lockdep: spin_lock_nest_lock()
Date: Wed, 08 Oct 2008 12:03:20 -0400	[thread overview]
Message-ID: <48ECD9C8.4000700@redhat.com> (raw)
In-Reply-To: <alpine.LFD.2.00.0810080836410.3208@nehalem.linux-foundation.org>

Linus Torvalds wrote:
> On Wed, 8 Oct 2008, Steven Rostedt wrote:
>   
>>> And yes, if there is an outer lock, even the order of getting locks is 
>>> irrelevant, as long as anybody who gets more than one inner lock always 
>>> holds the outer one.
>>>       
>> But I need to disagree on a programming practice style.  Unlocking locks
>> in a non nested order is just bad programming practice.
>>     
>
> No it is not.
>
> Unlocking locks in non-nested order can sometimes be very much the rigth 
> thing to do, and thinking otherwise is (a) naive and (b) can generate 
> totally unnecessary and pointless bugs.
>
> The thing is, sometimes you have to do it, and imposing totally made-up 
> rules ("unlocks have to nest") just confuses everybody.
>
> The FACT is, that unlocks do not have to nest cleanly. That's a rock solid 
> *FACT*. The locking order matters, and the unlocking order does not.
>
> And if you cannot accept that as a fact, and you then say "unlock order 
> should matter just to keep things nice and clean", then you end up being 
> screwed and/or confused when you can't hold to the unlock order.
>
> There are many perfectly valid reasons not to unlock in reverse order. 
> Don't create make-believe rules that break those reasons for no gain.
>   

Unfortunately, you cut out my comment that I stated "unless there is a 
good reason not to",
which the below example is a good reason ;-)
> For example:
>  - let's say that you have a singly-linked list of objects.
>  - you need to lock all objects, do something, and then unlock all 
>    objects.
>  - the *only* sane way to do that is to just traverse the list twice.
>  - that means that you will unlock the objects in the same order you 
>    locked them, _not_ in reverse ("nested") order.
>  - if you force a rule of "unlocks must be nested", then
>
> 	YOU ARE A F*CKING MORON.
>
> It's that simple. Don't do made-up rules that have no technical reason for 
> them. 
>
> Lock ordering matters. Unlock ordering does not. It really is that simple. 
> Don't confuse the issue by claiming anything else.
>   

Yes, I totally agree that there are good reasons not to unlock in 
reverse order, and the example
you gave happens to be one of them.

I just find that seeing something like:

    lock(A);
    lock(B);

    [do something]

    unlock(A);
    unlock(B);

just seems to be sloppy.

I wont harp on this, it only came up in conversation in which someone 
pointed out your
post.

-- Steve


  reply	other threads:[~2008-10-08 16:04 UTC|newest]

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-08-04 13:03 [RFC][PATCH 0/7] lockdep Peter Zijlstra
2008-08-04 13:03 ` [RFC][PATCH 1/7] lockdep: Fix combinatorial explosion in lock subgraph traversal Peter Zijlstra
2008-08-05  8:34   ` David Miller
2008-08-05  8:46     ` Peter Zijlstra
2008-08-13  3:48       ` Tim Pepper
2008-08-13 10:56         ` Ingo Molnar
2008-08-04 13:03 ` [RFC][PATCH 2/7] lockdep: lock_set_subclass - reset a held locks subclass Peter Zijlstra
2008-08-05  8:35   ` David Miller
2008-08-04 13:03 ` [RFC][PATCH 3/7] lockdep: re-annotate scheduler runqueues Peter Zijlstra
2008-08-05  8:35   ` David Miller
2008-08-04 13:03 ` [RFC][PATCH 4/7] lockdep: shrink held_lock structure Peter Zijlstra
2008-08-05 16:08   ` Peter Zijlstra
2008-08-06  7:17   ` Peter Zijlstra
2008-08-04 13:03 ` [RFC][PATCH 5/7] lockdep: map_acquire Peter Zijlstra
2008-08-04 13:03 ` [RFC][PATCH 6/7] lockdep: lock protection locks Peter Zijlstra
2008-08-04 13:03 ` [RFC][PATCH 7/7] lockdep: spin_lock_nest_lock() Peter Zijlstra
2008-08-04 14:07   ` Roland Dreier
2008-08-04 14:19     ` Peter Zijlstra
2008-08-04 14:26       ` Roland Dreier
2008-08-04 14:32         ` Peter Zijlstra
2008-08-04 14:53           ` Dave Jones
2008-08-04 14:56             ` Peter Zijlstra
2008-08-04 16:26               ` Andrea Arcangeli
2008-08-04 16:38                 ` Peter Zijlstra
2008-08-04 17:27                   ` Andrea Arcangeli
2008-08-04 17:46                     ` Andrea Arcangeli
2008-08-04 17:57                       ` [PATCH] workaround minor lockdep bug triggered by mm_take_all_locks Andrea Arcangeli
2008-08-04 18:48                         ` Peter Zijlstra
2008-08-04 18:56                           ` Roland Dreier
2008-08-04 19:05                             ` Peter Zijlstra
2008-08-04 20:15                           ` Andrea Arcangeli
2008-08-04 20:37                             ` Peter Zijlstra
2008-08-04 21:09                               ` Andrea Arcangeli
2008-08-04 21:14                                 ` Pekka Enberg
2008-08-04 21:30                                   ` Andrea Arcangeli
2008-08-04 21:41                                     ` Andrew Morton
2008-08-04 22:12                                       ` Andrea Arcangeli
2008-08-04 21:42                                     ` Arjan van de Ven
2008-08-04 22:30                                       ` Andrea Arcangeli
2008-08-04 23:38                                         ` Arjan van de Ven
2008-08-05  0:47                                           ` Andrea Arcangeli
2008-08-04 21:27                                 ` Arjan van de Ven
2008-08-04 21:54                                   ` Andrea Arcangeli
2008-08-04 21:57                                 ` David Miller
2008-08-05  2:00                                 ` Roland Dreier
2008-08-05  2:18                                   ` Andrea Arcangeli
2008-08-05 12:02                                     ` Roland Dreier
2008-08-05 12:20                                       ` Andrea Arcangeli
2008-08-04 18:48                     ` [RFC][PATCH 7/7] lockdep: spin_lock_nest_lock() Peter Zijlstra
2008-08-04 21:32                   ` David Miller
2008-08-04 18:06   ` Jeremy Fitzhardinge
2008-08-04 18:54     ` Peter Zijlstra
2008-08-04 19:26       ` Jeremy Fitzhardinge
2008-08-04 19:31         ` Linus Torvalds
2008-08-04 19:39           ` Peter Zijlstra
2008-08-04 20:16           ` Jeremy Fitzhardinge
2008-10-08 15:27           ` Steven Rostedt
2008-10-08 15:43             ` Linus Torvalds
2008-10-08 16:03               ` Steven Rostedt [this message]
2008-10-08 16:19                 ` Linus Torvalds
2008-10-08 16:53                   ` Steven Rostedt
2008-10-08 15:52             ` Nick Piggin
2008-10-08 17:18               ` Steven Rostedt
2008-08-07 11:25   ` Peter Zijlstra
2008-08-07 11:25 ` [RFC][PATCH 8/7] lockdep: annotate mm_take_all_locks() Peter Zijlstra
2008-08-07 11:25 ` [RFC][PATCH 9/7] mm: fix mm_take_all_locks() locking order Peter Zijlstra
2008-08-07 12:14   ` Hugh Dickins
2008-08-07 12:41     ` Peter Zijlstra
2008-08-07 13:27       ` Hugh Dickins
2008-08-07 21:46   ` Andrea Arcangeli
2008-08-08  1:34     ` Andrea Arcangeli
2008-08-08  7:16     ` Peter Zijlstra
2008-08-11 10:08 ` [RFC][PATCH 0/7] lockdep Ingo Molnar

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=48ECD9C8.4000700@redhat.com \
    --to=srostedt@redhat.com \
    --cc=a.p.zijlstra@chello.nl \
    --cc=akpm@linux-foundation.org \
    --cc=davej@redhat.com \
    --cc=davem@davemloft.net \
    --cc=hugh@veritas.com \
    --cc=jeremy@goop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=rostedt@goodmis.org \
    --cc=torvalds@linux-foundation.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.