writeback: prevent bandwidth calculation overflow

PATCH In review Wu Fengguang 4 participants 13 messages Nov 18, 2010 – Nov 22, 2010

Subsystems (4)

Memory management VFS Page cache MM misc

lore

Share & tools

lore

Wu Fengguang

author

2010-11-18 06:57 UTC #0 lore raw

PATCH 1/1 writeback: prevent bandwidth calculation overflow

On 32bit kernel, bdi->write_bandwidth can express at most 4GB/s.

However the current calculation code can overflow when disk bandwidth reaches 800MB/s. Fix it by using "long long" and swapping the order of multiplication/division. And further, change its unit to pages/second rather than bytes/second. That allows up to 16TB/s bandwidth in 32bit kernel.

Signed-off-by Wu Fengguang <[email protected]>

---
 mm/backing-dev.c    |    4 ++--
 mm/page-writeback.c |   13 ++++++-------
 2 files changed, 8 insertions(+), 9 deletions(-)

mm/page-writeback.c +6 -7

@@ -494,7 +494,7 @@ void bdi_update_write_bandwidth(struct b
 	unsigned long written;
 	unsigned long elapsed;
 	unsigned long bw;
-	unsigned long w;
+	unsigned long long w;
 
 	if (*bw_written == 0)
 		goto snapshot;

@@ -513,7 +513,7 @@ void bdi_update_write_bandwidth(struct b
 		goto snapshot;
 
 	written = percpu_counter_read(&bdi->bdi_stat[BDI_WRITTEN]) - *bw_written;
-	bw = (HZ * PAGE_CACHE_SIZE * written + elapsed/2) / elapsed;
+	bw = (HZ * written + elapsed/2) / elapsed;
 	w = min(elapsed / unit_time, 128UL);
 	bdi->write_bandwidth = (bdi->write_bandwidth * (1024-w) + bw * w) >> 10;
 	bdi->write_bandwidth_update_time = jiffies;

@@ -602,8 +602,7 @@ static void balance_dirty_pages(struct a
 		 * of dirty pages have been cleaned during our pause time.
 		 */
 		if (nr_dirty < dirty_thresh &&
-		    bdi_prev_dirty - bdi_dirty >
-		    bdi->write_bandwidth >> (PAGE_CACHE_SHIFT + 2))
+		    bdi_prev_dirty - bdi_dirty > bdi->write_bandwidth / 4)
 			break;
 		bdi_prev_dirty = bdi_dirty;

@@ -620,13 +619,13 @@ static void balance_dirty_pages(struct a
 		 * time when there are lots of dirtiers.
 		 */
 		bw = bdi->write_bandwidth;
-		bw = bw * (bdi_thresh - bdi_dirty);
 		bw = bw / (bdi_thresh / BDI_SOFT_DIRTY_LIMIT + 1);
+		bw = bw * (bdi_thresh - bdi_dirty);
 
-		bw = bw * (task_thresh - bdi_dirty);
 		bw = bw / (bdi_thresh / TASK_SOFT_DIRTY_LIMIT + 1);
+		bw = bw * (task_thresh - bdi_dirty);
 
-		pause = HZ * (pages_dirtied << PAGE_CACHE_SHIFT) / (bw + 1);
+		pause = HZ * pages_dirtied / (bw + 1);
 		pause = clamp_val(pause, 1, HZ/10);
 
 pause:

mm/backing-dev.c +2 -2

@@ -103,7 +103,7 @@ static int bdi_debug_stats_show(struct s
 		   (unsigned long) K(bdi_stat(bdi, BDI_RECLAIMABLE)),
 		   K(bdi_thresh), K(dirty_thresh), K(background_thresh),
 		   (unsigned long) K(bdi_stat(bdi, BDI_WRITTEN)),
-		   (unsigned long) bdi->write_bandwidth >> 10,
+		   (unsigned long) K(bdi->write_bandwidth),
 		   nr_dirty, nr_io, nr_more_io,
 		   !list_empty(&bdi->bdi_list), bdi->state);
 #undef K

@@ -662,7 +662,7 @@ int bdi_init(struct backing_dev_info *bd
 			goto err;
 	}
 
-	bdi->write_bandwidth = 100 << 20;
+	bdi->write_bandwidth = (100 << 20) / PAGE_CACHE_SIZE;
 	bdi->dirty_exceeded = 0;
 	err = prop_local_init_percpu(&bdi->completions);

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Rik van Riel

Acked-by on v1 1/1

2010-11-18 14:27 UTC #1 lore raw

Quoted 9 lines · Wu Fengguang

On 32bit kernel, bdi->write_bandwidth can express at most 4GB/s.
However the current calculation code can overflow when disk bandwidth reaches 800MB/s. Fix it by using "long long" and swapping the order of multiplication/division. And further, change its unit to pages/second rather than bytes/second. That allows up to 16TB/s bandwidth in 32bit kernel.
Signed-off-by Wu Fengguang<[email protected]>

Acked-by Rik van Riel <[email protected]>

Signature · 8 lines

All rights reversed

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Wu Fengguang

author Acked-by Rik van Riel

2010-11-18 15:44 UTC #2 lore raw

Quoted 9 lines · Rik van Riel

Quoted 8 lines · Wu Fengguang
On 32bit kernel, bdi->write_bandwidth can express at most 4GB/s.
However the current calculation code can overflow when disk bandwidth reaches 800MB/s. Fix it by using "long long" and swapping the order of multiplication/division. And further, change its unit to pages/second rather than bytes/second. That allows up to 16TB/s bandwidth in 32bit kernel.

Quoted 3 lines

Quoted 1 line
Signed-off-by Wu Fengguang<[email protected]>
Acked-by Rik van Riel <[email protected]>

Thanks. I find that the swap of multiplication/division leads to underflow, so changed to use "long long". It's free for 64bit gcc anyway :)

Thanks,
Fengguang
--
Subject: writeback: prevent bandwidth calculation overflow
Date: Thu Nov 18 12:55:42 CST 2010

On 32bit kernel, bdi->write_bandwidth can express at most 4GB/s.

However the current calculation code can overflow when disk bandwidth reaches 800MB/s. Fix it by using "long long" when doing calculations.

And further change its unit from bytes/second to pages/second. That allows up to 16TB/s bandwidth in 32bit kernel.

Acked-by Rik van Riel <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 mm/backing-dev.c    |    4 ++--
 mm/page-writeback.c |   11 +++++------
 2 files changed, 7 insertions(+), 8 deletions(-)

mm/page-writeback.c +5 -6

@@ -494,7 +494,7 @@ void bdi_update_write_bandwidth(struct b
 	unsigned long written;
 	unsigned long elapsed;
 	unsigned long bw;
-	unsigned long w;
+	unsigned long long w;
 
 	if (*bw_written == 0)
 		goto snapshot;

@@ -513,7 +513,7 @@ void bdi_update_write_bandwidth(struct b
 		goto snapshot;
 
 	written = percpu_counter_read(&bdi->bdi_stat[BDI_WRITTEN]) - *bw_written;
-	bw = (HZ * PAGE_CACHE_SIZE * written + elapsed/2) / elapsed;
+	bw = (HZ * written + elapsed/2) / elapsed;
 	w = min(elapsed / unit_time, 128UL);
 	bdi->write_bandwidth = (bdi->write_bandwidth * (1024-w) + bw * w) >> 10;
 	bdi->write_bandwidth_update_time = jiffies;

@@ -539,7 +539,7 @@ static void balance_dirty_pages(struct a
 	unsigned long dirty_thresh;
 	unsigned long bdi_thresh;
 	unsigned long task_thresh;
-	unsigned long bw;
+	unsigned long long bw;
 	unsigned long pause = 0;
 	bool dirty_exceeded = false;
 	struct backing_dev_info *bdi = mapping->backing_dev_info;

@@ -602,8 +602,7 @@ static void balance_dirty_pages(struct a
 		 * of dirty pages have been cleaned during our pause time.
 		 */
 		if (nr_dirty < dirty_thresh &&
-		    bdi_prev_dirty - bdi_dirty >
-		    bdi->write_bandwidth >> (PAGE_CACHE_SHIFT + 2))
+		    bdi_prev_dirty - bdi_dirty > bdi->write_bandwidth / 4)
 			break;
 		bdi_prev_dirty = bdi_dirty;

@@ -626,7 +625,7 @@ static void balance_dirty_pages(struct a
 		bw = bw * (task_thresh - bdi_dirty);
 		bw = bw / (bdi_thresh / TASK_SOFT_DIRTY_LIMIT + 1);
 
-		pause = HZ * (pages_dirtied << PAGE_CACHE_SHIFT) / (bw + 1);
+		pause = HZ * pages_dirtied / (bw + 1);
 		pause = clamp_val(pause, 1, HZ/10);
 
 pause:

mm/backing-dev.c +2 -2

@@ -103,7 +103,7 @@ static int bdi_debug_stats_show(struct s
 		   (unsigned long) K(bdi_stat(bdi, BDI_RECLAIMABLE)),
 		   K(bdi_thresh), K(dirty_thresh), K(background_thresh),
 		   (unsigned long) K(bdi_stat(bdi, BDI_WRITTEN)),
-		   (unsigned long) bdi->write_bandwidth >> 10,
+		   (unsigned long) K(bdi->write_bandwidth),
 		   nr_dirty, nr_io, nr_more_io,
 		   !list_empty(&bdi->bdi_list), bdi->state);
 #undef K

@@ -662,7 +662,7 @@ int bdi_init(struct backing_dev_info *bd
 			goto err;
 	}
 
-	bdi->write_bandwidth = 100 << 20;
+	bdi->write_bandwidth = (100 << 20) / PAGE_CACHE_SIZE;
 	bdi->dirty_exceeded = 0;
 	err = prop_local_init_percpu(&bdi->completions);

Peter Zijlstra

2010-11-18 16:02 UTC #3 lore raw

Quoted 1 line · Wu Fengguang

+ pause = HZ * pages_dirtied / (bw + 1);

Shouldn't that be using something like div64_u64 ?

Wu Fengguang

author

2010-11-18 16:06 UTC #4 lore raw

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Wu Fengguang
+ pause = HZ * pages_dirtied / (bw + 1);
Shouldn't that be using something like div64_u64 ?

OK, but a dumb question: gcc cannot handle this implicitly?

Thanks,
Fengguang

Wu Fengguang

author Acked-by Rik van Riel

2010-11-18 16:13 UTC #9 lore raw

In reply to Peter Zijlstra

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Wu Fengguang
+ pause = HZ * pages_dirtied / (bw + 1);
Shouldn't that be using something like div64_u64 ?

Thanks for review. Here is the updated patch using div64_u64().

---
Subject: writeback: prevent bandwidth calculation overflow
Date: Thu Nov 18 12:55:42 CST 2010

On 32bit kernel, bdi->write_bandwidth can express at most 4GB/s.

However the current calculation code can overflow when disk bandwidth reaches 800MB/s. Fix it by using "long long" and div64_u64() in the calculations.

And further change its unit from bytes/second to pages/second. That allows up to 16TB/s bandwidth in 32bit kernel.

CC Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 mm/backing-dev.c    |    4 ++--
 mm/page-writeback.c |   11 +++++------
 2 files changed, 7 insertions(+), 8 deletions(-)

mm/page-writeback.c +5 -6

@@ -494,7 +494,7 @@ void bdi_update_write_bandwidth(struct b
 	unsigned long written;
 	unsigned long elapsed;
 	unsigned long bw;
-	unsigned long w;
+	unsigned long long w;
 
 	if (*bw_written == 0)
 		goto snapshot;

@@ -513,7 +513,7 @@ void bdi_update_write_bandwidth(struct b
 		goto snapshot;
 
 	written = percpu_counter_read(&bdi->bdi_stat[BDI_WRITTEN]) - *bw_written;
-	bw = (HZ * PAGE_CACHE_SIZE * written + elapsed/2) / elapsed;
+	bw = (HZ * written + elapsed/2) / elapsed;
 	w = min(elapsed / unit_time, 128UL);
 	bdi->write_bandwidth = (bdi->write_bandwidth * (1024-w) + bw * w) >> 10;
 	bdi->write_bandwidth_update_time = jiffies;

@@ -539,7 +539,7 @@ static void balance_dirty_pages(struct a
 	unsigned long dirty_thresh;
 	unsigned long bdi_thresh;
 	unsigned long task_thresh;
-	unsigned long bw;
+	unsigned long long bw;
 	unsigned long pause = 0;
 	bool dirty_exceeded = false;
 	struct backing_dev_info *bdi = mapping->backing_dev_info;

@@ -602,8 +602,7 @@ static void balance_dirty_pages(struct a
 		 * of dirty pages have been cleaned during our pause time.
 		 */
 		if (nr_dirty < dirty_thresh &&
-		    bdi_prev_dirty - bdi_dirty >
-		    bdi->write_bandwidth >> (PAGE_CACHE_SHIFT + 2))
+		    bdi_prev_dirty - bdi_dirty > bdi->write_bandwidth / 4)
 			break;
 		bdi_prev_dirty = bdi_dirty;

@@ -626,7 +625,7 @@ static void balance_dirty_pages(struct a
 		bw = bw * (task_thresh - bdi_dirty);
 		bw = bw / (bdi_thresh / TASK_SOFT_DIRTY_LIMIT + 1);
 
-		pause = HZ * (pages_dirtied << PAGE_CACHE_SHIFT) / (bw + 1);
+		pause = div64_u64(HZ * pages_dirtied, bw + 1);
 		pause = clamp_val(pause, 1, HZ/10);
 
 pause:

mm/backing-dev.c +2 -2

@@ -103,7 +103,7 @@ static int bdi_debug_stats_show(struct s
 		   (unsigned long) K(bdi_stat(bdi, BDI_RECLAIMABLE)),
 		   K(bdi_thresh), K(dirty_thresh), K(background_thresh),
 		   (unsigned long) K(bdi_stat(bdi, BDI_WRITTEN)),
-		   (unsigned long) bdi->write_bandwidth >> 10,
+		   (unsigned long) K(bdi->write_bandwidth),
 		   nr_dirty, nr_io, nr_more_io,
 		   !list_empty(&bdi->bdi_list), bdi->state);
 #undef K

@@ -662,7 +662,7 @@ int bdi_init(struct backing_dev_info *bd
 			goto err;
 	}
 
-	bdi->write_bandwidth = 100 << 20;
+	bdi->write_bandwidth = (100 << 20) / PAGE_CACHE_SIZE;
 	bdi->dirty_exceeded = 0;
 	err = prop_local_init_percpu(&bdi->completions);

Wu Fengguang

author

2010-11-18 16:22 UTC #5 lore raw

In reply to Wu Fengguang

Quoted 7 lines · Wu Fengguang

Quoted 4 lines · Peter Zijlstra
Quoted 1 line · Wu Fengguang
+ pause = HZ * pages_dirtied / (bw + 1);
Shouldn't that be using something like div64_u64 ?
OK, but a dumb question: gcc cannot handle this implicitly?

Hmm, I'm tempting to do

        pause = HZ * pages_dirtied / ((unsigned long)bw + 1);

because here "bw" is guaranteed to be smaller than ULONG_MAX.

Thanks,
Fengguang

Peter Zijlstra

2010-11-18 16:29 UTC #7 lore raw

In reply to Wu Fengguang

Quoted 7 lines · Wu Fengguang

Quoted 4 lines · Peter Zijlstra
Quoted 1 line · Wu Fengguang
+ pause = HZ * pages_dirtied / (bw + 1);
Shouldn't that be using something like div64_u64 ?
OK, but a dumb question: gcc cannot handle this implicitly?

it could, but we chose not to implement the symbol it emits for these things so as to cause pain.. that was still assuming the world of 32bit computing was relevant and 64bit divides were expensive ;-)

Wu Fengguang

author

2010-11-18 16:34 UTC #8 lore raw

Quoted 5 lines · Peter Zijlstra

Quoted 4 lines · Wu Fengguang
Quoted 3 lines · Peter Zijlstra
Quoted 1 line · Wu Fengguang
+ pause = HZ * pages_dirtied / (bw + 1);

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Shouldn't that be using something like div64_u64 ?
OK, but a dumb question: gcc cannot handle this implicitly?
it could, but we chose not to implement the symbol it emits for these things so as to cause pain.. that was still assuming the world of 32bit computing was relevant and 64bit divides were expensive ;-)

Good to know that, thanks! So let's avoid it totally :)

Wu Fengguang

author Acked-by Rik van Riel

2010-11-18 16:36 UTC #6 lore raw

In reply to Wu Fengguang

Quoted 7 lines · Wu Fengguang

Quoted 6 lines · Wu Fengguang
Quoted 4 lines · Peter Zijlstra
Quoted 1 line · Wu Fengguang
+ pause = HZ * pages_dirtied / (bw + 1);
Shouldn't that be using something like div64_u64 ?

Quoted 7 lines

Quoted 1 line
OK, but a dumb question: gcc cannot handle this implicitly?
Hmm, I'm tempting to do
        pause = HZ * pages_dirtied / ((unsigned long)bw + 1);
because here "bw" is guaranteed to be smaller than ULONG_MAX.

Here it is :)

---
Subject: writeback: prevent bandwidth calculation overflow
Date: Thu Nov 18 12:55:42 CST 2010

On 32bit kernel, bdi->write_bandwidth can express at most 4GB/s.

However the current calculation code can overflow when disk bandwidth reaches 800MB/s. Fix it by using "long long" and div64_u64() in the calculations.

And further change its unit from bytes/second to pages/second. That allows up to 16TB/s bandwidth in 32bit kernel.

CC Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 mm/backing-dev.c    |    4 ++--
 mm/page-writeback.c |   11 +++++------
 2 files changed, 7 insertions(+), 8 deletions(-)

mm/page-writeback.c +5 -6

@@ -494,7 +494,7 @@ void bdi_update_write_bandwidth(struct b
 	unsigned long written;
 	unsigned long elapsed;
 	unsigned long bw;
-	unsigned long w;
+	unsigned long long w;
 
 	if (*bw_written == 0)
 		goto snapshot;

@@ -513,7 +513,7 @@ void bdi_update_write_bandwidth(struct b
 		goto snapshot;
 
 	written = percpu_counter_read(&bdi->bdi_stat[BDI_WRITTEN]) - *bw_written;
-	bw = (HZ * PAGE_CACHE_SIZE * written + elapsed/2) / elapsed;
+	bw = (HZ * written + elapsed/2) / elapsed;
 	w = min(elapsed / unit_time, 128UL);
 	bdi->write_bandwidth = (bdi->write_bandwidth * (1024-w) + bw * w) >> 10;
 	bdi->write_bandwidth_update_time = jiffies;

@@ -539,7 +539,7 @@ static void balance_dirty_pages(struct a
 	unsigned long dirty_thresh;
 	unsigned long bdi_thresh;
 	unsigned long task_thresh;
-	unsigned long bw;
+	unsigned long long bw;
 	unsigned long pause = 0;
 	bool dirty_exceeded = false;
 	struct backing_dev_info *bdi = mapping->backing_dev_info;

@@ -602,8 +602,7 @@ static void balance_dirty_pages(struct a
 		 * of dirty pages have been cleaned during our pause time.
 		 */
 		if (nr_dirty < dirty_thresh &&
-		    bdi_prev_dirty - bdi_dirty >
-		    bdi->write_bandwidth >> (PAGE_CACHE_SHIFT + 2))
+		    bdi_prev_dirty - bdi_dirty > bdi->write_bandwidth / 4)
 			break;
 		bdi_prev_dirty = bdi_dirty;

@@ -626,7 +625,7 @@ static void balance_dirty_pages(struct a
 		bw = bw * (task_thresh - bdi_dirty);
 		bw = bw / (bdi_thresh / TASK_SOFT_DIRTY_LIMIT + 1);
 
-		pause = HZ * (pages_dirtied << PAGE_CACHE_SHIFT) / (bw + 1);
+		pause = HZ * pages_dirtied / ((unsigned long)bw + 1);
 		pause = clamp_val(pause, 1, HZ/10);
 
 pause:

mm/backing-dev.c +2 -2

@@ -103,7 +103,7 @@ static int bdi_debug_stats_show(struct s
 		   (unsigned long) K(bdi_stat(bdi, BDI_RECLAIMABLE)),
 		   K(bdi_thresh), K(dirty_thresh), K(background_thresh),
 		   (unsigned long) K(bdi_stat(bdi, BDI_WRITTEN)),
-		   (unsigned long) bdi->write_bandwidth >> 10,
+		   (unsigned long) K(bdi->write_bandwidth),
 		   nr_dirty, nr_io, nr_more_io,
 		   !list_empty(&bdi->bdi_list), bdi->state);
 #undef K

@@ -662,7 +662,7 @@ int bdi_init(struct backing_dev_info *bd
 			goto err;
 	}
 
-	bdi->write_bandwidth = 100 << 20;
+	bdi->write_bandwidth = (100 << 20) / PAGE_CACHE_SIZE;
 	bdi->dirty_exceeded = 0;
 	err = prop_local_init_percpu(&bdi->completions);

Michal Hocko

2010-11-19 16:06 UTC #10 lore raw

In reply to Wu Fengguang

Quoted 41 lines · Wu Fengguang

Quoted 4 lines · Peter Zijlstra
Quoted 1 line · Wu Fengguang
+ pause = HZ * pages_dirtied / (bw + 1);
Shouldn't that be using something like div64_u64 ?
Thanks for review. Here is the updated patch using div64_u64().
---
Subject: writeback: prevent bandwidth calculation overflow
Date: Thu Nov 18 12:55:42 CST 2010
On 32bit kernel, bdi->write_bandwidth can express at most 4GB/s.
However the current calculation code can overflow when disk bandwidth reaches 800MB/s. Fix it by using "long long" and div64_u64() in the calculations.
And further change its unit from bytes/second to pages/second. That allows up to 16TB/s bandwidth in 32bit kernel.
CC Peter Zijlstra <[email protected]>
Acked-by Rik van Riel <[email protected]>
Signed-off-by Wu Fengguang <[email protected]>
---
 mm/backing-dev.c    |    4 ++--
 mm/page-writeback.c |   11 +++++------
 2 files changed, 7 insertions(+), 8 deletions(-)
mm/page-writeback.c +1 -1
@@ -494,7 +494,7 @@ void bdi_update_write_bandwidth(struct b
 	unsigned long written;
 	unsigned long elapsed;
 	unsigned long bw;
-	unsigned long w;
+	unsigned long long w;
 
 	if (*bw_written == 0)
 		goto snapshot;

mm/page-writeback.c void bdi_update_write_bandwidth(struct b · line 516

Quoted 5 lines

mm/page-writeback.c +1 -1

@@ -513,7 +513,7 @@ void bdi_update_write_bandwidth(struct b
 		goto snapshot;
 
 	written = percpu_counter_read(&bdi->bdi_stat[BDI_WRITTEN]) - *bw_written;
-	bw = (HZ * PAGE_CACHE_SIZE * written + elapsed/2) / elapsed;
+	bw = (HZ * written + elapsed/2) / elapsed;

Sorry for a dumb question, but where did PAGE_CACHE_SIZE part go?

Quoted 3 lines

mm/page-writeback.c +0 -0

 	w = min(elapsed / unit_time, 128UL);
 	bdi->write_bandwidth = (bdi->write_bandwidth * (1024-w) + bw * w) >> 10;
 	bdi->write_bandwidth_update_time = jiffies;

Signature · 13 lines

Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Wu Fengguang

author Acked-by Rik van Riel

2010-11-19 18:44 UTC #11 lore raw

Quoted 44 lines · Michal Hocko

Quoted 43 lines · Wu Fengguang
Quoted 4 lines · Peter Zijlstra
Quoted 1 line · Wu Fengguang
+ pause = HZ * pages_dirtied / (bw + 1);
Shouldn't that be using something like div64_u64 ?
Thanks for review. Here is the updated patch using div64_u64().
---
Subject: writeback: prevent bandwidth calculation overflow
Date: Thu Nov 18 12:55:42 CST 2010
On 32bit kernel, bdi->write_bandwidth can express at most 4GB/s.
However the current calculation code can overflow when disk bandwidth reaches 800MB/s. Fix it by using "long long" and div64_u64() in the calculations.
And further change its unit from bytes/second to pages/second. That allows up to 16TB/s bandwidth in 32bit kernel.
CC Peter Zijlstra <[email protected]>
Acked-by Rik van Riel <[email protected]>
Signed-off-by Wu Fengguang <[email protected]>
---
 mm/backing-dev.c    |    4 ++--
 mm/page-writeback.c |   11 +++++------
 2 files changed, 7 insertions(+), 8 deletions(-)
mm/page-writeback.c +1 -1
@@ -494,7 +494,7 @@ void bdi_update_write_bandwidth(struct b
 	unsigned long written;
 	unsigned long elapsed;
 	unsigned long bw;
-	unsigned long w;
+	unsigned long long w;
 
 	if (*bw_written == 0)
 		goto snapshot;
@@ -513,7 +513,7 @@ void bdi_update_write_bandwidth(struct b
 		goto snapshot;
 

Quoted 5 lines

mm/page-writeback.c void bdi_update_write_bandwidth(struct b · line 516
Quoted 3 lines
mm/page-writeback.c +1 -1
 	written = percpu_counter_read(&bdi->bdi_stat[BDI_WRITTEN]) - *bw_written;
-	bw = (HZ * PAGE_CACHE_SIZE * written + elapsed/2) / elapsed;
+	bw = (HZ * written + elapsed/2) / elapsed;
Sorry for a dumb question, but where did PAGE_CACHE_SIZE part go?

Because write_bandwidth's unit is bumped from bytes/s to pages/s, so that it can express much higher bandwidth.

I've updated the patch again, and will submit more fixes tomorrow.

Thanks,
Fengguang
---
Subject: writeback: prevent bandwidth calculation overflow
Date: Thu Nov 18 12:55:42 CST 2010

On 32bit kernel, bdi->write_bandwidth can express at most 4GB/s.

However the current calculation code can overflow when disk bandwidth reaches 800MB/s. Fix it by using "long long" and div64_u64() in the calculations.

And further change its unit from bytes/second to pages/second. That allows up to 16TB/s bandwidth in 32bit kernel.

CC Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 include/linux/backing-dev.h |    5 +++--
 mm/backing-dev.c            |    4 ++--
 mm/page-writeback.c         |   14 +++++++-------
 3 files changed, 12 insertions(+), 11 deletions(-)

mm/page-writeback.c +7 -7

@@ -494,7 +494,7 @@ void bdi_update_write_bandwidth(struct b
 	unsigned long written;
 	unsigned long elapsed;
 	unsigned long bw;
-	unsigned long w;
+	unsigned long long w;
 
 	if (*bw_written == 0)
 		goto snapshot;

@@ -513,9 +513,10 @@ void bdi_update_write_bandwidth(struct b
 		goto snapshot;
 
 	written = percpu_counter_read(&bdi->bdi_stat[BDI_WRITTEN]) - *bw_written;
-	bw = (HZ * PAGE_CACHE_SIZE * written + elapsed/2) / elapsed;
+	bw = (HZ * written + elapsed / 2) / elapsed;
 	w = min(elapsed / unit_time, 128UL);
-	bdi->write_bandwidth = (bdi->write_bandwidth * (1024-w) + bw * w) >> 10;
+	bdi->write_bandwidth = (bdi->write_bandwidth * (1024-w) +
+				bw * w + 1023) >> 10;
 	bdi->write_bandwidth_update_time = jiffies;
 snapshot:
 	*bw_written = percpu_counter_read(&bdi->bdi_stat[BDI_WRITTEN]);

@@ -539,7 +540,7 @@ static void balance_dirty_pages(struct a
 	unsigned long dirty_thresh;
 	unsigned long bdi_thresh;
 	unsigned long task_thresh;
-	unsigned long bw;
+	unsigned long long bw;
 	unsigned long pause = 0;
 	bool dirty_exceeded = false;
 	struct backing_dev_info *bdi = mapping->backing_dev_info;

@@ -602,8 +603,7 @@ static void balance_dirty_pages(struct a
 		 * of dirty pages have been cleaned during our pause time.
 		 */
 		if (nr_dirty < dirty_thresh &&
-		    bdi_prev_dirty - bdi_dirty >
-		    bdi->write_bandwidth >> (PAGE_CACHE_SHIFT + 2))
+		    bdi_prev_dirty - bdi_dirty > (long)bdi->write_bandwidth / 4)
 			break;
 		bdi_prev_dirty = bdi_dirty;

@@ -626,7 +626,7 @@ static void balance_dirty_pages(struct a
 		bw = bw * (task_thresh - bdi_dirty);
 		bw = bw / (bdi_thresh / TASK_SOFT_DIRTY_LIMIT + 1);
 
-		pause = HZ * (pages_dirtied << PAGE_CACHE_SHIFT) / (bw + 1);
+		pause = HZ * pages_dirtied / ((unsigned long)bw + 1);
 		pause = clamp_val(pause, 1, HZ/10);
 
 pause:

mm/backing-dev.c +2 -2

@@ -103,7 +103,7 @@ static int bdi_debug_stats_show(struct s
 		   (unsigned long) K(bdi_stat(bdi, BDI_RECLAIMABLE)),
 		   K(bdi_thresh), K(dirty_thresh), K(background_thresh),
 		   (unsigned long) K(bdi_stat(bdi, BDI_WRITTEN)),
-		   (unsigned long) bdi->write_bandwidth >> 10,
+		   (unsigned long) K(bdi->write_bandwidth),
 		   nr_dirty, nr_io, nr_more_io,
 		   !list_empty(&bdi->bdi_list), bdi->state);
 #undef K

@@ -662,7 +662,7 @@ int bdi_init(struct backing_dev_info *bd
 			goto err;
 	}
 
-	bdi->write_bandwidth = 100 << 20;
+	bdi->write_bandwidth = (100 << 20) / PAGE_CACHE_SIZE;
 	bdi->dirty_exceeded = 0;
 	err = prop_local_init_percpu(&bdi->completions);

include/linux/backing-dev.h +3 -2

@@ -74,9 +74,10 @@ struct backing_dev_info {
 
 	struct percpu_counter bdi_stat[NR_BDI_STAT_ITEMS];
 
-	struct prop_local_percpu completions;
+	unsigned long write_bandwidth;
 	unsigned long write_bandwidth_update_time;
-	int write_bandwidth;
+
+	struct prop_local_percpu completions;
 	int dirty_exceeded;
 
 	unsigned int min_ratio;

Michal Hocko

2010-11-22 09:54 UTC #12 lore raw

Quoted 45 lines · Wu Fengguang

Quoted 44 lines · Michal Hocko
Quoted 43 lines · Wu Fengguang
Quoted 4 lines · Peter Zijlstra
Quoted 1 line · Wu Fengguang
+ pause = HZ * pages_dirtied / (bw + 1);
Shouldn't that be using something like div64_u64 ?
Thanks for review. Here is the updated patch using div64_u64().
---
Subject: writeback: prevent bandwidth calculation overflow
Date: Thu Nov 18 12:55:42 CST 2010
On 32bit kernel, bdi->write_bandwidth can express at most 4GB/s.
However the current calculation code can overflow when disk bandwidth reaches 800MB/s. Fix it by using "long long" and div64_u64() in the calculations.
And further change its unit from bytes/second to pages/second. That allows up to 16TB/s bandwidth in 32bit kernel.
CC Peter Zijlstra <[email protected]>
Acked-by Rik van Riel <[email protected]>
Signed-off-by Wu Fengguang <[email protected]>
---
 mm/backing-dev.c    |    4 ++--
 mm/page-writeback.c |   11 +++++------
 2 files changed, 7 insertions(+), 8 deletions(-)
mm/page-writeback.c +1 -1
@@ -494,7 +494,7 @@ void bdi_update_write_bandwidth(struct b
 	unsigned long written;
 	unsigned long elapsed;
 	unsigned long bw;
-	unsigned long w;
+	unsigned long long w;
 
 	if (*bw_written == 0)
 		goto snapshot;
@@ -513,7 +513,7 @@ void bdi_update_write_bandwidth(struct b
 		goto snapshot;
 

Quoted 8 lines

Quoted 5 lines
mm/page-writeback.c void bdi_update_write_bandwidth(struct b
Quoted 3 lines
mm/page-writeback.c +1 -1
 	written = percpu_counter_read(&bdi->bdi_stat[BDI_WRITTEN]) - *bw_written;
-	bw = (HZ * PAGE_CACHE_SIZE * written + elapsed/2) / elapsed;
+	bw = (HZ * written + elapsed/2) / elapsed;
Sorry for a dumb question, but where did PAGE_CACHE_SIZE part go?
Because write_bandwidth's unit is bumped from bytes/s to pages/s, so that it can express much higher bandwidth.

Ahh, I can see it now.
Thanks

Signature · 13 lines

Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>