Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

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

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


liwp.linux at gmail

Jun 9, 2012, 9:20 PM

Post #1 of 6 (81 views)
Permalink
[PATCH] page-writeback.c: fix update bandwidth time judgment error

From: Wanpneg Li <liwp [at] linux>

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 [at] linux>
---
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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


fengguang.wu at intel

Jun 9, 2012, 9:36 PM

Post #2 of 6 (81 views)
Permalink
Re: [PATCH] page-writeback.c: fix update bandwidth time judgment error [In reply to]

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 [at] linux>
>
> 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 [at] linux>
> ---
> 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


liwp.linux at gmail

Jun 9, 2012, 9:54 PM

Post #3 of 6 (80 views)
Permalink
Re: [PATCH] page-writeback.c: fix update bandwidth time judgment error [In reply to]

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 [at] linux>
>>
>> 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 [at] linux>
>> ---
>> 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


fengguang.wu at intel

Jun 10, 2012, 12:24 AM

Post #4 of 6 (74 views)
Permalink
Re: [PATCH] page-writeback.c: fix update bandwidth time judgment error [In reply to]

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 [at] linux>
> >>
> >> 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 [at] linux>
> >> ---
> >> 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


liwp.linux at gmail

Jun 10, 2012, 12:41 AM

Post #5 of 6 (74 views)
Permalink
Re: [PATCH] page-writeback.c: fix update bandwidth time judgment error [In reply to]

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 [at] linux>
>> >>
>> >> 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 [at] linux>
>> >> ---
>> >> 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


fengguang.wu at intel

Jun 10, 2012, 12:47 AM

Post #6 of 6 (78 views)
Permalink
Re: [PATCH] page-writeback.c: fix update bandwidth time judgment error [In reply to]

> 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.