Messages in this thread | | | Date | Wed, 25 Mar 2009 11:46:41 +0100 | From | Andi Kleen <> | Subject | Re: [PATCH 02/12] DRBD: activity_log |
| |
On Wed, Mar 25, 2009 at 11:27:22AM +0100, Philipp Reisner wrote: > md_io_page gets allocated with GFP_KERNEL (no GFP_HIGHMEM either). > > > [...] > > > + > > > + spin_lock_irq(&mdev->al_lock); > > > + lc_changed(mdev->act_log, al_ext); > > > + spin_unlock_irq(&mdev->al_lock); > > > + wake_up(&mdev->al_wait); > > > > The wake_up outside the lock looks a little dangerous. > > > > Please share you thoughts, why this looks a little dangerous ?
I haven't double checked the whole path, but unlocked wake up is often a good recipe to potentially lose wake ups.
> [...] > > > + mutex_lock(&mdev->md_io_mutex); /* protects md_io_buffer, al_tr_cycle, > > > ... */ > > > > Doing checksumming inside a lock looks nasty. > > > > Well, that is a mutex, not a spinlock. We need to hold that lock here,
Yes it's independent. If it takes a lot of CPU time you'll likely have a bottle neck. It's normally a bad idea to do anything CPU intensive under a lock covering more than your current limited object.
-Andi
-- ak@linux.intel.com -- Speaking for myself only.
| |