Re: [PATCH RESEND 2/3 v2.6.39-rc7] block: make disk_block_events()properly wait for work cancellation
From: Tejun Heo
Date: Tue May 17 2011 - 11:11:17 EST
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/