On Fri, 8 Jun 2018, Mike Snitzer wrote: > On Thu, Jun 07 2018 at 11:48am -0400, > Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote: > > > This is second version of this patch - it also removes the label > > continue_locked, because it is no longer needed. If forgot to refresh the > > patch before sending it, so I sent an olded version. > > > > > > From: Mikulas Patocka <mpatocka@xxxxxxxxxx> > > Subject: [patch 2/3 v2] dm-writecache: convert wait queue to wake_up_process > > > > If there's just one process that can wait on a queue, we can use > > wake_up_process. According to Linus, it is safe to call wake_up_process > > on a process even if the process may be doing something else. > > > > Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx> > > > > --- > > drivers/md/dm-writecache.c | 34 +++++++++++++++------------------- > > 1 file changed, 15 insertions(+), 19 deletions(-) > > > > Index: linux-2.6/drivers/md/dm-writecache.c > > =================================================================== > > --- linux-2.6.orig/drivers/md/dm-writecache.c 2018-06-05 22:54:49.000000000 +0200 > > +++ linux-2.6/drivers/md/dm-writecache.c 2018-06-07 17:44:11.000000000 +0200 > > @@ -1273,10 +1272,11 @@ static void writecache_writeback_endio(s > > struct dm_writecache *wc = wb->wc; > > unsigned long flags; > > > > - raw_spin_lock_irqsave(&wc->endio_thread_wait.lock, flags); > > + raw_spin_lock_irqsave(&wc->endio_list_lock, flags); > > + if (unlikely(list_empty(&wc->endio_list))) > > + wake_up_process(wc->endio_thread); > > list_add_tail(&wb->endio_entry, &wc->endio_list); > > - swake_up_locked(&wc->endio_thread_wait); > > - raw_spin_unlock_irqrestore(&wc->endio_thread_wait.lock, flags); > > + raw_spin_unlock_irqrestore(&wc->endio_list_lock, flags); > > } > > I'm not following the logic you're using for the above pattern of using > wake_up_process if the list is empty.. seems unintuitive. > > Given you add to the list (be it endio here, or flush elsewhere), why > not just add to the list and then always wake_up_process()? > > Mike Because wake_up_process is costly (it takes a spinlock on the process). If multiple CPUs are simultaneously hammering on a spinlock, it degrades performance. The process checks if the list is empty before going to sleep (and doesn't sleep if it is non-empty) - consequently, if the process goes to sleep, the list must have been empty. So, we can wake the process up only once - when the list transitions from empty to non-empty - we don't have to wake it up with every item added to the list. Originally, the code was like this: raw_spin_lock_irqsave(&wc->endio_list_lock, flags); need_wake = list_empty(&wc->endio_list); list_add_tail(&wb->endio_entry, &wc->endio_list); if (need_wake) wake_up_process(wc->endio_thread); raw_spin_unlock_irqrestore(&wc->endio_thread_wait.lock, flags); However, because the target process takes the spinlock too, we can wake it up before we add the entry to the list - it doesn't matter here if we wake it before or after adding the entry to the list, because the target process will take the same spinlock when it is woken up. Calling wake_up_process before list_add_tail results in slightly smaller code. Mikulas -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
