linux-kernel - Re: [PATCH RESEND 2/3 v2.6.39-rc7] block: make disk_block_events() properly wait for work cancellation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110517151107.GQ20624@htj.dyndns.org>
Date:	Tue, 17 May 2011 17:11:07 +0200
From:	Tejun Heo <tj@...nel.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Jens Axboe <axboe@...nel.dk>, Sitsofe Wheeler <sitsofe@...oo.com>,
	Borislav Petkov <bp@...en8.de>, Meelis Roos <mroos@...ux.ee>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Kay Sievers <kay.sievers@...y.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND 2/3 v2.6.39-rc7] block: make disk_block_events()
 properly wait for work cancellation

Hello, Linus.

On Tue, May 17, 2011 at 07:46:25AM -0700, Linus Torvalds wrote:
> This is pretty disgusting.
> 
> You're not using a real lock, and to compensate for that you use a
> bloccking bit-lock hack. And to make that hack extra ugly, you define
> the bit as a bitmask, and use the ilog2() macro to turn it into a bit
> pos.
> 
> Horrid. Horrid.

Heh, okay.  It's not a lock tho.  It's multiple waiter waiting for a
single event - so either explicit waitqueue or completion.  I was
doing a waitqueue but bit waitqueue didn't seem to add too much
complexity, so...

> Is there some fundamental reason why you cannot just turn the ev->lock
> into a real semaphore (allowing blocking), and then doing the dwork
> cancel under the semaphore - avoiding all the crazy bit-lock crud.

disk_check_events() can be called from non-sleepable context so we
need a spinlock protecting block count.

> Or just _add_ a semaphore to the 'struct disk_events', for chrissake.

Alright, a completion then.

> This is just too ugly to survive. And even if you fixed the ilog()
> (hint: just define the bit, and then use (1u<<BIT) to define the
> mask), it would be too ugly.

In many cases there are more C bitops than atomic or wait_bit() type
operations which take bit position rather than mask.  I find it less
painful to define constants as bit masks and using ilog2() on those
ops than doing lots of 1 << BIT.

I don't know, this is constantly painful.  More are defined as bit
masks but some prominent ones use bit positions and some even define
both.  Given that the C bitops are there by default, I wish the
explicit bitops also took bitmask and just did ilog2() internally, but
well it's too late and we're stuck with the mixed situation.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/