All of lore.kernel.org
 help / color / mirror / Atom feed
From: Pavel Machek <pavel@ucw.cz>
To: Nick Piggin <nickpiggin@yahoo.com.au>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org
Subject: Re: [patch 1/5] wait: use lock bitops for __wait_on_bit_lock
Date: Thu, 11 Oct 2007 20:56:42 +0000	[thread overview]
Message-ID: <20071011205641.GA3864@ucw.cz> (raw)
In-Reply-To: <200710251217.10704.nickpiggin@yahoo.com.au>

Hi!

> > Sorry, I'm just not going to apply a patch like that.
> >
> > I mean, how the heck is anyone else supposed to understand what you're up
> > to?
> 
> Hmm, I might just withdraw this patch 1/5. This is generally a slowpath,
> and it's hard to guarantee that any exported user doesn't rely on the
> full barrier here (not that they would know whether they do or not, let
> alone document the fact).
> 
> I think all the others are pretty clear, though? (read on if no)
...
> > There's a bit of documentation in Documentation/atomic_ops.txt 
> > (probably enough, I guess) but it is totally unobvious to 98.3% of kernel
> > developers when they should use test_and_set_bit() versus
> > test_and_set_bit_lock() and it is far too much work to work out why it was
> > used in __wait_on_bit_lock(), whether it is correct, what advantages it
> > brings and whether and where others should emulate it.
> 
> If you set a bit for the purpose of opening a critical section, then
> you can use this. And clear_bit_unlock to close it.
> 
> The advantages are that it is faster, and the hapless driver writer
> doesn't have to butcher or forget about doing the correct
> smp_mb__before_clear_bit(), or have reviewers wondering exactly WTF
> that barrier is for, etc.

Actually, I'd expect *_lock() ro be slower than *()...

> Basically, it is faster, harder to get wrong, and more self-docuemnting.

So I'd not call it self-documenting.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

  reply	other threads:[~2007-11-01 14:34 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-10-24  8:13 [patch 0/5] lock bitops patches npiggin
2007-10-24  8:13 ` [patch 1/5] wait: use lock bitops for __wait_on_bit_lock npiggin
2007-10-25  1:14   ` Andrew Morton
2007-10-25  2:17     ` Nick Piggin
2007-10-11 20:56       ` Pavel Machek [this message]
2007-10-24  8:13 ` [patch 2/5] tasklet: use lock bitops for tasklet lock npiggin
2007-10-24  8:13 ` [patch 3/5] mm: rename page lock npiggin
2007-10-24  8:13 ` [patch 4/5] mm: use lock bitops for " npiggin
2007-10-24  8:13 ` [patch 5/5] fs: use lock bitops for the buffer lock npiggin

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=20071011205641.GA3864@ucw.cz \
    --to=pavel@ucw.cz \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nickpiggin@yahoo.com.au \
    /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.