lkml.org 
[lkml]   [2010]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count
On Thu, May 27, 2010 at 01:51:09PM +0300, Artem Bityutskiy wrote:
> Nick, thanks for serialization suggestion.
>
> On Thu, 2010-05-27 at 17:22 +1000, Nick Piggin wrote:
> > Yeah, we definitely don't want to add global cacheline writes in the
> > common case. Also I don't know why you do the strange -1 value. I
> > couldn't seem to find where you defined bdi_arm_supers_timer();
>
> It is in mm/backing-dev.c:376 in today's Linus' tree. The -1 is used to

Yep I should have grepped. /hangs head


> indicate that 'sync_supers()' is in progress and avoid arming timer in
> that case. But yes, this is not really needed.

OK please remove it.


> > But why doesn't this work?
> >
> > sb->s_dirty = 1;
> > smp_mb(); /* corresponding MB is in test_and_clear_bit */
>
> AFAIU, test_and_clear_bit assumes 2 barriers - before the test and after
> the clear. Then I do not really understand why this smp_mb is needed.

You almost always need barriers executed on all sides of the
synchronisation protocol. Actually we need another, I confused
myself with the test_and_clear at the end.

1. sb->s_dirty = 1; /* store */
2. if (!supers_timer_armed) /* load */
3. supers_timer_armed = 1; /* store */

and

A. supers_timer_armed = 0; /* store */
B. if (sb->s_dirty) /* load */
C. sb->s_dirty = 0 /* store */

If these two sequences are executed, it must result in
sb->s_dirty == 1 iff supers_timer_armed

* If 2 is executed before 1 is visible, then 2 may miss A before B sees 1.
* If B is executed before A is visible, then B may miss 1 before 2 sees A.

So we need smp_mb() between 1/2 and A/B (I missed the 2nd one).

Now we still have a problem. After sync task rechecks
supers_timer_armed, the supers timer might execute before we mark
ourself as sleeping, and so we have another lost wakeup. It needs
to be checked after set_current_state.

Let's try this again. I much prefer to name the variable something
that indicates whether there is more work to be done, or whether we
can sleep.

How about something like this?
--

Index: linux-2.6/mm/backing-dev.c
===================================================================
--- linux-2.6.orig/mm/backing-dev.c
+++ linux-2.6/mm/backing-dev.c
@@ -45,6 +45,7 @@ LIST_HEAD(bdi_pending_list);

static struct task_struct *sync_supers_tsk;
static struct timer_list sync_supers_timer;
+static unsigned long supers_dirty __read_mostly;

static int bdi_sync_supers(void *);
static void sync_supers_timer_fn(unsigned long);
@@ -251,7 +252,6 @@ static int __init default_bdi_init(void)

init_timer(&sync_supers_timer);
setup_timer(&sync_supers_timer, sync_supers_timer_fn, 0);
- bdi_arm_supers_timer();

err = bdi_init(&default_backing_dev_info);
if (!err)
@@ -362,17 +362,28 @@ static int bdi_sync_supers(void *unused)

while (!kthread_should_stop()) {
set_current_state(TASK_INTERRUPTIBLE);
- schedule();
+ if (!supers_dirty)
+ schedule();
+ else
+ __set_current_state(TASK_RUNNING);

+ supers_dirty = 0;
/*
- * Do this periodically, like kupdated() did before.
+ * supers_dirty store must be visible to mark_sb_dirty (below)
+ * before sync_supers runs (which loads sb->s_dirty).
*/
+ smp_mb();
sync_supers();
}

return 0;
}

+static void sync_supers_timer_fn(unsigned long unused)
+{
+ wake_up_process(sync_supers_tsk);
+}
+
void bdi_arm_supers_timer(void)
{
unsigned long next;
@@ -384,9 +395,17 @@ void bdi_arm_supers_timer(void)
mod_timer(&sync_supers_timer, round_jiffies_up(next));
}

-static void sync_supers_timer_fn(unsigned long unused)
+void mark_sb_dirty(struct super_block *sb)
{
- wake_up_process(sync_supers_tsk);
+ sb->s_dirty = 1;
+ /*
+ * sb->s_dirty store must be visible to sync_supers (above) before we
+ * load supers_dirty in case we need to re-arm the timer.
+ */
+ smp_mb();
+ if (likely(supers_dirty))
+ return;
+ supers_dirty = 1;
bdi_arm_supers_timer();
}


\
 
 \ /
  Last update: 2010-05-27 14:09    [W:0.093 / U:0.536 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site