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