On Fri, 8 Jun 2018, Mike Snitzer wrote:
> On Thu, Jun 07 2018 at 11:48am -0400,
> Mikulas Patocka <mpato...@redhat.com> 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 <mpato...@redhat.com>
> > 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 <mpato...@redhat.com>
> >
> > ---
> > 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@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel