* [PATCH] page-writeback.c: fix update bandwidth time judgment error @ 2012-06-10 4:20 Wanpeng Li 2012-06-10 4:36 ` Fengguang Wu 0 siblings, 1 reply; 6+ messages in thread From: Wanpeng Li @ 2012-06-10 4:20 UTC (permalink / raw) To: Fengguang Wu Cc: LKML, Jan Kara, Andrew Morton, Peter Zijlstra, Johannes Weiner, linux-mm, Gavin Shan, Wanpeng Li, Wanpeng Li From: Wanpneg Li <liwp@linux.vnet.ibm.com> Since bdi_update_bandwidth function should estimate write bandwidth at 200ms intervals, so the time is bdi->bw_time_stamp + BANDWIDTH_INTERVAL == jiffies, but if use time_is_after_eq_jiffies intervals will be bdi->bw_time_stamp + BANDWIDTH_INTERVAL + 1. Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com> --- mm/page-writeback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/page-writeback.c b/mm/page-writeback.c index c833bf0..099e225 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -1032,7 +1032,7 @@ static void bdi_update_bandwidth(struct backing_dev_info *bdi, unsigned long bdi_dirty, unsigned long start_time) { - if (time_is_after_eq_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL)) + if (time_is_after_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL)) return; spin_lock(&bdi->wb.list_lock); __bdi_update_bandwidth(bdi, thresh, bg_thresh, dirty, -- 1.7.9.5 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] page-writeback.c: fix update bandwidth time judgment error 2012-06-10 4:20 [PATCH] page-writeback.c: fix update bandwidth time judgment error Wanpeng Li @ 2012-06-10 4:36 ` Fengguang Wu 2012-06-10 4:54 ` Wanpeng Li 0 siblings, 1 reply; 6+ messages in thread From: Fengguang Wu @ 2012-06-10 4:36 UTC (permalink / raw) To: Wanpeng Li Cc: LKML, Jan Kara, Andrew Morton, Peter Zijlstra, Johannes Weiner, linux-mm, Gavin Shan, Wanpeng Li Wanpeng, Sorry this I won't take this: it don't really improve anything. Even with the changed test, the real intervals are still some random values above (and not far away from) 200ms.. We are saying about 200ms intervals just for convenience. Thanks, Fengguang On Sun, Jun 10, 2012 at 12:20:05PM +0800, Wanpeng Li wrote: > From: Wanpneg Li <liwp@linux.vnet.ibm.com> > > Since bdi_update_bandwidth function should estimate write bandwidth at 200ms intervals, > so the time is bdi->bw_time_stamp + BANDWIDTH_INTERVAL == jiffies, but > if use time_is_after_eq_jiffies intervals will be bdi->bw_time_stamp + > BANDWIDTH_INTERVAL + 1. > > Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com> > --- > mm/page-writeback.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c > index c833bf0..099e225 100644 > --- a/mm/page-writeback.c > +++ b/mm/page-writeback.c > @@ -1032,7 +1032,7 @@ static void bdi_update_bandwidth(struct backing_dev_info *bdi, > unsigned long bdi_dirty, > unsigned long start_time) > { > - if (time_is_after_eq_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL)) > + if (time_is_after_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL)) > return; > spin_lock(&bdi->wb.list_lock); > __bdi_update_bandwidth(bdi, thresh, bg_thresh, dirty, > -- > 1.7.9.5 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] page-writeback.c: fix update bandwidth time judgment error 2012-06-10 4:36 ` Fengguang Wu @ 2012-06-10 4:54 ` Wanpeng Li 2012-06-10 7:24 ` Fengguang Wu 0 siblings, 1 reply; 6+ messages in thread From: Wanpeng Li @ 2012-06-10 4:54 UTC (permalink / raw) To: Fengguang Wu Cc: LKML, Jan Kara, Andrew Morton, PeterZijlstra, Johannes Weiner, linux-mm, Gavin Shan, Wanpeng Li On Sun, Jun 10, 2012 at 12:36:41PM +0800, Fengguang Wu wrote: >Wanpeng, > >Sorry this I won't take this: it don't really improve anything. Even >with the changed test, the real intervals are still some random values >above (and not far away from) 200ms.. We are saying about 200ms >intervals just for convenience. > But some parts like: __bdi_update_bandwidth which bdi_update_bandwidth will call: if(elapsed < BANDWIDTH_INTERVAL) return; or global_update_bandwidth: if(time_before(now, update_time + BANDWIDTH_INTERVAL)) return; You me just ignore this disunion ? Regards, Wanpeng Li > >On Sun, Jun 10, 2012 at 12:20:05PM +0800, Wanpeng Li wrote: >> From: Wanpneg Li <liwp@linux.vnet.ibm.com> >> >> Since bdi_update_bandwidth function should estimate write bandwidth at 200ms intervals, >> so the time is bdi->bw_time_stamp + BANDWIDTH_INTERVAL == jiffies, but >> if use time_is_after_eq_jiffies intervals will be bdi->bw_time_stamp + >> BANDWIDTH_INTERVAL + 1. >> >> Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com> >> --- >> mm/page-writeback.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/page-writeback.c b/mm/page-writeback.c >> index c833bf0..099e225 100644 >> --- a/mm/page-writeback.c >> +++ b/mm/page-writeback.c >> @@ -1032,7 +1032,7 @@ static void bdi_update_bandwidth(struct backing_dev_info *bdi, >> unsigned long bdi_dirty, >> unsigned long start_time) >> { >> - if (time_is_after_eq_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL)) >> + if (time_is_after_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL)) >> return; >> spin_lock(&bdi->wb.list_lock); >> __bdi_update_bandwidth(bdi, thresh, bg_thresh, dirty, >> -- >> 1.7.9.5 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] page-writeback.c: fix update bandwidth time judgment error 2012-06-10 4:54 ` Wanpeng Li @ 2012-06-10 7:24 ` Fengguang Wu 2012-06-10 7:41 ` Wanpeng Li 0 siblings, 1 reply; 6+ messages in thread From: Fengguang Wu @ 2012-06-10 7:24 UTC (permalink / raw) To: Wanpeng Li Cc: LKML, Jan Kara, Andrew Morton, PeterZijlstra, Johannes Weiner, linux-mm, Gavin Shan, Wanpeng Li On Sun, Jun 10, 2012 at 12:54:03PM +0800, Wanpeng Li wrote: > On Sun, Jun 10, 2012 at 12:36:41PM +0800, Fengguang Wu wrote: > >Wanpeng, > > > >Sorry this I won't take this: it don't really improve anything. Even > >with the changed test, the real intervals are still some random values > >above (and not far away from) 200ms.. We are saying about 200ms > >intervals just for convenience. > > > But some parts like: > > __bdi_update_bandwidth which bdi_update_bandwidth will call: > > if(elapsed < BANDWIDTH_INTERVAL) > return; > > or > > global_update_bandwidth: > > if(time_before(now, update_time + BANDWIDTH_INTERVAL)) > return; > > You me just ignore this disunion ? Not a problem for me. But if that consistency makes you feel happy, you might revise the changelog and resend. But it's not that simple for the below reason.. > >On Sun, Jun 10, 2012 at 12:20:05PM +0800, Wanpeng Li wrote: > >> From: Wanpneg Li <liwp@linux.vnet.ibm.com> > >> > >> Since bdi_update_bandwidth function should estimate write bandwidth at 200ms intervals, The above line represents a wrong assumption. It's normal for the re-estimate intervals to be >= 200ms. > >> so the time is bdi->bw_time_stamp + BANDWIDTH_INTERVAL == jiffies, but > >> if use time_is_after_eq_jiffies intervals will be bdi->bw_time_stamp + > >> BANDWIDTH_INTERVAL + 1. Strictly speaking, to ensure that ">= 200ms" is true, we'll have to skip the "jiffies == bw_time_stamp + BANDWIDTH_INTERVAL" case. For example, when HZ=100, the bw_time_stamp may actually be recorded in the very last ms of a 10ms range, and jiffies may be in the very first ms of the current 10ms range. So if using ">=" comparisons, it may actually let less than 200ms intervals go though. We can only reliably ensure "> 200ms", but no way for ">= 200ms". Thanks, Fengguang > >> Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com> > >> --- > >> mm/page-writeback.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/mm/page-writeback.c b/mm/page-writeback.c > >> index c833bf0..099e225 100644 > >> --- a/mm/page-writeback.c > >> +++ b/mm/page-writeback.c > >> @@ -1032,7 +1032,7 @@ static void bdi_update_bandwidth(struct backing_dev_info *bdi, > >> unsigned long bdi_dirty, > >> unsigned long start_time) > >> { > >> - if (time_is_after_eq_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL)) > >> + if (time_is_after_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL)) > >> return; > >> spin_lock(&bdi->wb.list_lock); > >> __bdi_update_bandwidth(bdi, thresh, bg_thresh, dirty, > >> -- > >> 1.7.9.5 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] page-writeback.c: fix update bandwidth time judgment error 2012-06-10 7:24 ` Fengguang Wu @ 2012-06-10 7:41 ` Wanpeng Li 2012-06-10 7:47 ` Fengguang Wu 0 siblings, 1 reply; 6+ messages in thread From: Wanpeng Li @ 2012-06-10 7:41 UTC (permalink / raw) To: Fengguang Wu Cc: LKML, Jan Kara, Andrew Morton, Peter Zijlstra, Johannes Weiner, linux-mm, Gavin Shan, Wanpeng Li, Wanpeng Li On Sun, Jun 10, 2012 at 03:24:14PM +0800, Fengguang Wu wrote: >On Sun, Jun 10, 2012 at 12:54:03PM +0800, Wanpeng Li wrote: >> On Sun, Jun 10, 2012 at 12:36:41PM +0800, Fengguang Wu wrote: >> >Wanpeng, >> > >> >Sorry this I won't take this: it don't really improve anything. Even >> >with the changed test, the real intervals are still some random values >> >above (and not far away from) 200ms.. We are saying about 200ms >> >intervals just for convenience. >> > >> But some parts like: >> >> __bdi_update_bandwidth which bdi_update_bandwidth will call: >> >> if(elapsed < BANDWIDTH_INTERVAL) >> return; >> >> or >> >> global_update_bandwidth: >> >> if(time_before(now, update_time + BANDWIDTH_INTERVAL)) >> return; >> >> You me just ignore this disunion ? > >Not a problem for me. But if that consistency makes you feel happy, >you might revise the changelog and resend. But it's not that simple >for the below reason.. > >> >On Sun, Jun 10, 2012 at 12:20:05PM +0800, Wanpeng Li wrote: >> >> From: Wanpneg Li <liwp@linux.vnet.ibm.com> >> >> >> >> Since bdi_update_bandwidth function should estimate write bandwidth at 200ms intervals, > >The above line represents a wrong assumption. It's normal for the >re-estimate intervals to be >= 200ms. > >> >> so the time is bdi->bw_time_stamp + BANDWIDTH_INTERVAL == jiffies, but >> >> if use time_is_after_eq_jiffies intervals will be bdi->bw_time_stamp + >> >> BANDWIDTH_INTERVAL + 1. > >Strictly speaking, to ensure that ">= 200ms" is true, we'll have to >skip the "jiffies == bw_time_stamp + BANDWIDTH_INTERVAL" case. For >example, when HZ=100, the bw_time_stamp may actually be recorded in >the very last ms of a 10ms range, and jiffies may be in the very first >ms of the current 10ms range. So if using ">=" comparisons, it may >actually let less than 200ms intervals go though. > >We can only reliably ensure "> 200ms", but no way for ">= 200ms". > static void global_update_bandwidth(unsigned long thresh, unsigned long dirty, unsigned long now) { static DEFINE_SPINLOCK(dirty_lock); static unsigned long update_time; /* * check locklessly first to optimize away locking for the most time */ if (time_before(now, update_time + BANDWIDTH_INTERVAL)) return; spin_lock(&dirty_lock); if (time_after_eq(now, update_time + BANDWIDTH_INTERVAL)) { update_dirty_limit(thresh, dirty); update_time = now; } spin_unlock(&dirty_lock); } So time_after_eq in global_update_bandwidth function should also change to time_after, or just ignore this disunion? > >> >> Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com> >> >> --- >> >> mm/page-writeback.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/mm/page-writeback.c b/mm/page-writeback.c >> >> index c833bf0..099e225 100644 >> >> --- a/mm/page-writeback.c >> >> +++ b/mm/page-writeback.c >> >> @@ -1032,7 +1032,7 @@ static void bdi_update_bandwidth(struct backing_dev_info *bdi, >> >> unsigned long bdi_dirty, >> >> unsigned long start_time) >> >> { >> >> - if (time_is_after_eq_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL)) >> >> + if (time_is_after_jiffies(bdi->bw_time_stamp + BANDWIDTH_INTERVAL)) >> >> return; >> >> spin_lock(&bdi->wb.list_lock); >> >> __bdi_update_bandwidth(bdi, thresh, bg_thresh, dirty, >> >> -- >> >> 1.7.9.5 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] page-writeback.c: fix update bandwidth time judgment error 2012-06-10 7:41 ` Wanpeng Li @ 2012-06-10 7:47 ` Fengguang Wu 0 siblings, 0 replies; 6+ messages in thread From: Fengguang Wu @ 2012-06-10 7:47 UTC (permalink / raw) To: Wanpeng Li Cc: LKML, Jan Kara, Andrew Morton, Peter Zijlstra, Johannes Weiner, linux-mm, Gavin Shan, Wanpeng Li > static void global_update_bandwidth(unsigned long thresh, > unsigned long dirty, > unsigned long now) > { > static DEFINE_SPINLOCK(dirty_lock); > static unsigned long update_time; > > /* > * check locklessly first to optimize away locking for the most time > */ > if (time_before(now, update_time + BANDWIDTH_INTERVAL)) > return; > > spin_lock(&dirty_lock); > if (time_after_eq(now, update_time + BANDWIDTH_INTERVAL)) { > update_dirty_limit(thresh, dirty); > update_time = now; > } > spin_unlock(&dirty_lock); > } > > So time_after_eq in global_update_bandwidth function should also change > to time_after, or just ignore this disunion? Let's just ignore them. You are very careful and I like it. Please move on and keep up the good work! Thanks, Fengguang -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-06-10 7:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-06-10 4:20 [PATCH] page-writeback.c: fix update bandwidth time judgment error Wanpeng Li 2012-06-10 4:36 ` Fengguang Wu 2012-06-10 4:54 ` Wanpeng Li 2012-06-10 7:24 ` Fengguang Wu 2012-06-10 7:41 ` Wanpeng Li 2012-06-10 7:47 ` Fengguang Wu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).