page-writeback.c: fix update bandwidth time judgment error

PATCH In review Wanpeng Li 2 participants 6 messages Jun 10, 2012

Subsystems (2)

Memory management Page cache

lore

Share & tools

lore

Wanpeng Li

author

2012-06-10 04:20 UTC #0 lore raw

PATCH 1/1 page-writeback.c: fix update bandwidth time judgment error

From: Wanpneg Li <[email protected]>

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 <[email protected]>

---
 mm/page-writeback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/page-writeback.c +1 -1

index c833bf0..099e225 100644
@@ -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,

Signature · 7 lines

1.7.9.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Fengguang Wu

2012-06-10 04:36 UTC #1 lore raw

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

Quoted 27 lines · Wanpeng Li

From: Wanpneg Li <[email protected]>
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 <[email protected]>
---
 mm/page-writeback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/page-writeback.c +1 -1
index c833bf0..099e225 100644
@@ -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,
Signature · 1 line
1.7.9.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Wanpeng Li

author

2012-06-10 04:54 UTC #2 lore raw

Quoted 7 lines · Fengguang Wu

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;

global_update_bandwidth:

if(time_before(now, update_time + BANDWIDTH_INTERVAL))

	return;

You me just ignore this disunion ?

Regards,
Wanpeng Li

Quoted 14 lines · Wanpeng Li

Quoted 12 lines
From: Wanpneg Li <[email protected]>
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 <[email protected]>
---
 mm/page-writeback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 15 lines

mm/page-writeback.c +1 -1

index c833bf0..099e225 100644
@@ -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,

Signature · 1 line

1.7.9.5

Fengguang Wu

2012-06-10 07:24 UTC #3 lore raw

Quoted 15 lines · Wanpeng Li

Quoted 7 lines · Fengguang Wu
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;

Quoted 8 lines

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..

Quoted 4 lines

Quoted 4 lines · Wanpeng Li
Quoted 3 lines
From: Wanpneg Li <[email protected]>
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.

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
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

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Signed-off-by Wanpeng Li <[email protected]>
---
 mm/page-writeback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 15 lines

Quoted 7 lines

mm/page-writeback.c +0 -0

index c833bf0..099e225 100644
@@ -1032,7 +1032,7 @@ static void bdi_update_bandwidth(struct backing_dev_info *bdi,
 				 unsigned long bdi_dirty,
 				 unsigned long start_time)

Quoted 8 lines

mm/page-writeback.c +1 -1

 {
-	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,

Signature · 1 line

1.7.9.5

Wanpeng Li

author

2012-06-10 07:41 UTC #4 lore raw

Quoted 41 lines · Fengguang Wu

Quoted 15 lines · Wanpeng Li
Quoted 7 lines · Fengguang Wu
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;
Quoted 8 lines
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..
Quoted 4 lines
Quoted 4 lines · Wanpeng Li
Quoted 3 lines
From: Wanpneg Li <[email protected]>
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.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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.

Quoted 9 lines

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?

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Wanpeng Li <[email protected]>
---
 mm/page-writeback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 15 lines

Quoted 7 lines

mm/page-writeback.c +0 -0

index c833bf0..099e225 100644
@@ -1032,7 +1032,7 @@ static void bdi_update_bandwidth(struct backing_dev_info *bdi,
 				 unsigned long bdi_dirty,
 				 unsigned long start_time)

Quoted 8 lines

mm/page-writeback.c +1 -1

 {
-	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,

Signature · 1 line

1.7.9.5

Fengguang Wu

2012-06-10 07:48 UTC #5 lore raw

Quoted 21 lines

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);
}

Quoted 2 lines

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