Xen devel - Oct 2013 - [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

time_after_eq() only works if the delta is < MAX_ULONG/2.

If netfront sends at a very low rate, the time between subsequent calls
to tx_credit_exceeded() may exceed MAX_ULONG/2 and the test for
timer_after_eq() will be incorrect.  Credit will not be replenished and
the guest may become unable to send (e.g., if prior to the long gap, all
credit was exhausted).

We should add the scenario which now beyond next_credit+MAX_UNLONG/2. Because
the fact now must be not before than expire, time_before(now, expire) == true
will verify the scenario.
    time_after_eq(now, next_credit) || time_before (now, expire)
    =    !time_in_range_open(now, expire, next_credit)

Signed-off-by: Jason Luan <jianhai.luan@oracle.com>
---
 drivers/net/xen-netback/netback.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index f3e591c..31eedaf 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1194,8 +1194,11 @@ static bool tx_credit_exceeded(struct xenvif *vif,
unsigned size)
 	if (timer_pending(&vif->credit_timeout))
 		return true;
 
-	/* Passed the point where we can replenish credit? */
-	if (time_after_eq(now, next_credit)) {
+	/* Credit should be replenished when now does not fall into the
+	 * range from expires to next_credit, and time_in_range_open()
+	 * is used to verify whether this case happens.
+	 */
+	if (!time_in_range_open(now, vif->credit_timeout.expires, next_credit)) {
 		vif->credit_timeout.expires = now;
 		tx_add_credit(vif);
 	}
-- 
1.7.6.5

>>> On 16.10.13 at 19:22, Jason Luan <jianhai.luan@oracle.com>
wrote:
> time_after_eq() only works if the delta is < MAX_ULONG/2.
> 
> If netfront sends at a very low rate, the time between subsequent calls
> to tx_credit_exceeded() may exceed MAX_ULONG/2 and the test for
> timer_after_eq() will be incorrect.  Credit will not be replenished and
> the guest may become unable to send (e.g., if prior to the long gap, all
> credit was exhausted).
> 
> We should add the scenario which now beyond next_credit+MAX_UNLONG/2.
Because
> the fact now must be not before than expire, time_before(now, expire) ==
true
> will verify the scenario.
>     time_after_eq(now, next_credit) || time_before (now, expire)
>     =>     !time_in_range_open(now, expire, next_credit)
So first of all this must be with a 32-bit netback. And the not
coverable gap between activity is well over 240 days long. _If_
this really needs dealing with, then why is extending this from
240+ to 480+ days sufficient? I.e. why don''t you simply
change to 64-bit jiffy values, and use time_after_eq64()?

Jan
> Signed-off-by: Jason Luan <jianhai.luan@oracle.com>
> ---
>  drivers/net/xen-netback/netback.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c 
> b/drivers/net/xen-netback/netback.c
> index f3e591c..31eedaf 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1194,8 +1194,11 @@ static bool tx_credit_exceeded(struct xenvif *vif, 
> unsigned size)
>  	if (timer_pending(&vif->credit_timeout))
>  		return true;
>  
> -	/* Passed the point where we can replenish credit? */
> -	if (time_after_eq(now, next_credit)) {
> +	/* Credit should be replenished when now does not fall into the
> +	 * range from expires to next_credit, and time_in_range_open()
> +	 * is used to verify whether this case happens.
> +	 */
> +	if (!time_in_range_open(now, vif->credit_timeout.expires,
next_credit)) {
>  		vif->credit_timeout.expires = now;
>  		tx_add_credit(vif);
>  	}
> -- 
> 1.7.6.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

On 2013-10-17 16:26, Jan Beulich wrote:
>>>> On 16.10.13 at 19:22, Jason Luan
<jianhai.luan@oracle.com> wrote:
>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>
>> If netfront sends at a very low rate, the time between subsequent calls
>> to tx_credit_exceeded() may exceed MAX_ULONG/2 and the test for
>> timer_after_eq() will be incorrect.  Credit will not be replenished and
>> the guest may become unable to send (e.g., if prior to the long gap,
all
>> credit was exhausted).
>>
>> We should add the scenario which now beyond next_credit+MAX_UNLONG/2.
Because
>> the fact now must be not before than expire, time_before(now, expire)
== true
>> will verify the scenario.
>>      time_after_eq(now, next_credit) || time_before (now, expire)
>>      =>>      !time_in_range_open(now, expire, next_credit)
> So first of all this must be with a 32-bit netback. And the not
> coverable gap between activity is well over 240 days long. _If_
> this really needs dealing with, then why is extending this from
> 240+ to 480+ days sufficient? I.e. why don''t you simply
> change to 64-bit jiffy values, and use time_after_eq64()?
Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond 
MAX_ULONG/2 in 64-bit will need long long time)

I think the gap should be think all environment even now extending 480+. 
if now fall in the gap,  one timer will be pending and replenish will be 
in time.  Please run the attachment test program.

If use time_after_eq64(), expire ,next_credit and other member will must 
be u64.
>
> Jan
>
>> Signed-off-by: Jason Luan <jianhai.luan@oracle.com>
>> ---
>>   drivers/net/xen-netback/netback.c |    7 +++++--
>>   1 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/netback.c
>> b/drivers/net/xen-netback/netback.c
>> index f3e591c..31eedaf 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -1194,8 +1194,11 @@ static bool tx_credit_exceeded(struct xenvif
*vif,
>> unsigned size)
>>   	if (timer_pending(&vif->credit_timeout))
>>   		return true;
>>   
>> -	/* Passed the point where we can replenish credit? */
>> -	if (time_after_eq(now, next_credit)) {
>> +	/* Credit should be replenished when now does not fall into the
>> +	 * range from expires to next_credit, and time_in_range_open()
>> +	 * is used to verify whether this case happens.
>> +	 */
>> +	if (!time_in_range_open(now, vif->credit_timeout.expires,
next_credit)) {
>>   		vif->credit_timeout.expires = now;
>>   		tx_add_credit(vif);
>>   	}
>> -- 
>> 1.7.6.5
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>

On 2013-10-17 17:02, jianhai luan wrote:
>
> On 2013-10-17 16:26, Jan Beulich wrote:
>>>>> On 16.10.13 at 19:22, Jason Luan
<jianhai.luan@oracle.com> wrote:
>>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>>
>>> If netfront sends at a very low rate, the time between subsequent
calls
>>> to tx_credit_exceeded() may exceed MAX_ULONG/2 and the test for
>>> timer_after_eq() will be incorrect.  Credit will not be replenished
and
>>> the guest may become unable to send (e.g., if prior to the long
gap,
>>> all
>>> credit was exhausted).
>>>
>>> We should add the scenario which now beyond 
>>> next_credit+MAX_UNLONG/2. Because
>>> the fact now must be not before than expire, time_before(now, 
>>> expire) == true
>>> will verify the scenario.
>>>      time_after_eq(now, next_credit) || time_before (now, expire)
>>>      =>>>      !time_in_range_open(now, expire,
next_credit)
>> So first of all this must be with a 32-bit netback. And the not
>> coverable gap between activity is well over 240 days long. _If_
>> this really needs dealing with, then why is extending this from
>> 240+ to 480+ days sufficient? I.e. why don''t you simply
>> change to 64-bit jiffy values, and use time_after_eq64()?
>
> Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond 
> MAX_ULONG/2 in 64-bit will need long long time)
>
> I think the gap should be think all environment even now extending 
> 480+. if now fall in the gap,  one timer will be pending and replenish 
> will be in time.  Please run the attachment test program.
>
Sorry for miss the attachment in previous letter. Please check the 
attachment.
> If use time_after_eq64(), expire ,next_credit and other member will 
> must be u64.
>>
>> Jan
>>
>>> Signed-off-by: Jason Luan <jianhai.luan@oracle.com>
>>> ---
>>>   drivers/net/xen-netback/netback.c |    7 +++++--
>>>   1 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/xen-netback/netback.c
>>> b/drivers/net/xen-netback/netback.c
>>> index f3e591c..31eedaf 100644
>>> --- a/drivers/net/xen-netback/netback.c
>>> +++ b/drivers/net/xen-netback/netback.c
>>> @@ -1194,8 +1194,11 @@ static bool tx_credit_exceeded(struct xenvif
>>> *vif,
>>> unsigned size)
>>>       if (timer_pending(&vif->credit_timeout))
>>>           return true;
>>>   -    /* Passed the point where we can replenish credit? */
>>> -    if (time_after_eq(now, next_credit)) {
>>> +    /* Credit should be replenished when now does not fall into
the
>>> +     * range from expires to next_credit, and time_in_range_open()
>>> +     * is used to verify whether this case happens.
>>> +     */
>>> +    if (!time_in_range_open(now, vif->credit_timeout.expires, 
>>> next_credit)) {
>>>           vif->credit_timeout.expires = now;
>>>           tx_add_credit(vif);
>>>       }
>>> -- 
>>> 1.7.6.5
>>>
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>>
>>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 17/10/13 10:02, jianhai luan wrote:
> 
> On 2013-10-17 16:26, Jan Beulich wrote:
>>>>> On 16.10.13 at 19:22, Jason Luan
<jianhai.luan@oracle.com> wrote:
>>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>>
>>> If netfront sends at a very low rate, the time between subsequent
calls
>>> to tx_credit_exceeded() may exceed MAX_ULONG/2 and the test for
>>> timer_after_eq() will be incorrect.  Credit will not be replenished
and
>>> the guest may become unable to send (e.g., if prior to the long
gap, all
>>> credit was exhausted).
>>>
>>> We should add the scenario which now beyond
next_credit+MAX_UNLONG/2.
>>> Because
>>> the fact now must be not before than expire, time_before(now,
expire)
>>> == true
>>> will verify the scenario.
>>>      time_after_eq(now, next_credit) || time_before (now, expire)
>>>      =>>>      !time_in_range_open(now, expire,
next_credit)
>> So first of all this must be with a 32-bit netback. And the not
>> coverable gap between activity is well over 240 days long. _If_
>> this really needs dealing with, then why is extending this from
>> 240+ to 480+ days sufficient? I.e. why don''t you simply
>> change to 64-bit jiffy values, and use time_after_eq64()?
> 
> Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond
> MAX_ULONG/2 in 64-bit will need long long time)
> 
> I think the gap should be think all environment even now extending 480+.
> if now fall in the gap,  one timer will be pending and replenish will be
> in time.  Please run the attachment test program.
> 
> If use time_after_eq64(), expire ,next_credit and other member will must
> be u64.
Yes, you''ll need to store next_credit as a u64 in vif instead of
calculating it in tx_credit_exceeded from expires (which is only an
unsigned long).

David

>>> On 17.10.13 at 11:02, jianhai luan <jianhai.luan@oracle.com>
wrote:
> On 2013-10-17 16:26, Jan Beulich wrote:
>>>>> On 16.10.13 at 19:22, Jason Luan
<jianhai.luan@oracle.com> wrote:
>>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>>
>>> If netfront sends at a very low rate, the time between subsequent
calls
>>> to tx_credit_exceeded() may exceed MAX_ULONG/2 and the test for
>>> timer_after_eq() will be incorrect.  Credit will not be replenished
and
>>> the guest may become unable to send (e.g., if prior to the long
gap, all
>>> credit was exhausted).
>>>
>>> We should add the scenario which now beyond
next_credit+MAX_UNLONG/2.
> Because
>>> the fact now must be not before than expire, time_before(now,
expire) ==
> true
>>> will verify the scenario.
>>>      time_after_eq(now, next_credit) || time_before (now, expire)
>>>      =>>>      !time_in_range_open(now, expire,
next_credit)
>> So first of all this must be with a 32-bit netback. And the not
>> coverable gap between activity is well over 240 days long. _If_
>> this really needs dealing with, then why is extending this from
>> 240+ to 480+ days sufficient? I.e. why don''t you simply
>> change to 64-bit jiffy values, and use time_after_eq64()?
> 
> Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond 
> MAX_ULONG/2 in 64-bit will need long long time)
> 
> I think the gap should be think all environment even now extending 480+. 
> if now fall in the gap,  one timer will be pending and replenish will be 
> in time.  Please run the attachment test program.
Not sure what this is supposed to tell me. I recognize that there
are overflow conditions not handled properly, but (a) I have a
hard time thinking of a sensible guest that sits idle for over 240
days (host uptime usually isn''t even coming close to that due to
maintenance requirements) and (b) if there is such a sensible
guest, then I can''t see why dealing with one being idle for over
480 days should be required too.
> If use time_after_eq64(), expire ,next_credit and other member will must 
> be u64.
Exactly - that''s what I was telling you to do.

Jan

On 2013-10-17 17:26, Jan Beulich wrote:
>>>> On 17.10.13 at 11:02, jianhai luan
<jianhai.luan@oracle.com> wrote:
>> On 2013-10-17 16:26, Jan Beulich wrote:
>>>>>> On 16.10.13 at 19:22, Jason Luan
<jianhai.luan@oracle.com> wrote:
>>>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>>>
>>>> If netfront sends at a very low rate, the time between
subsequent calls
>>>> to tx_credit_exceeded() may exceed MAX_ULONG/2 and the test for
>>>> timer_after_eq() will be incorrect.  Credit will not be
replenished and
>>>> the guest may become unable to send (e.g., if prior to the long
gap, all
>>>> credit was exhausted).
>>>>
>>>> We should add the scenario which now beyond
next_credit+MAX_UNLONG/2.
>> Because
>>>> the fact now must be not before than expire, time_before(now,
expire) =>> true
>>>> will verify the scenario.
>>>>       time_after_eq(now, next_credit) || time_before (now,
expire)
>>>>       =>>>>       !time_in_range_open(now, expire,
next_credit)
>>> So first of all this must be with a 32-bit netback. And the not
>>> coverable gap between activity is well over 240 days long. _If_
>>> this really needs dealing with, then why is extending this from
>>> 240+ to 480+ days sufficient? I.e. why don''t you simply
>>> change to 64-bit jiffy values, and use time_after_eq64()?
>> Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond
>> MAX_ULONG/2 in 64-bit will need long long time)
>>
>> I think the gap should be think all environment even now extending
480+.
>> if now fall in the gap,  one timer will be pending and replenish will
be
>> in time.  Please run the attachment test program.
> Not sure what this is supposed to tell me. I recognize that there
> are overflow conditions not handled properly, but (a) I have a
> hard time thinking of a sensible guest that sits idle for over 240
> days (host uptime usually isn''t even coming close to that due to
> maintenance requirements) and (b) if there is such a sensible
> guest, then I can''t see why dealing with one being idle for over
> 480 days should be required too.
The issue can be reproduced when now beyond MAX_ULONG/2 (if the gust 
will send lesser package).
Jiffies beyond than MAX_UNLONG/2 will need below time:
     HZ         days
    100        248.55        (((0xffffffff/2)/HZ)/3600)/24
    250        99.42          (((0xffffffff/2)/HZ)/3600)/24
   1000       24.86          (((0xffffffff/2)/HZ)/3600)/24

Because we use 250,  the issue be found when uptime large than 100 days.

Jason
>> If use time_after_eq64(), expire ,next_credit and other member will
must
>> be u64.
> Exactly - that''s what I was telling you to do.
>
> Jan
>

On 2013-10-17 17:15, David Vrabel wrote:
> On 17/10/13 10:02, jianhai luan wrote:
>> On 2013-10-17 16:26, Jan Beulich wrote:
>>>>>> On 16.10.13 at 19:22, Jason Luan
<jianhai.luan@oracle.com> wrote:
>>>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>>>
>>>> If netfront sends at a very low rate, the time between
subsequent calls
>>>> to tx_credit_exceeded() may exceed MAX_ULONG/2 and the test for
>>>> timer_after_eq() will be incorrect.  Credit will not be
replenished and
>>>> the guest may become unable to send (e.g., if prior to the long
gap, all
>>>> credit was exhausted).
>>>>
>>>> We should add the scenario which now beyond
next_credit+MAX_UNLONG/2.
>>>> Because
>>>> the fact now must be not before than expire, time_before(now,
expire)
>>>> == true
>>>> will verify the scenario.
>>>>       time_after_eq(now, next_credit) || time_before (now,
expire)
>>>>       =>>>>       !time_in_range_open(now, expire,
next_credit)
>>> So first of all this must be with a 32-bit netback. And the not
>>> coverable gap between activity is well over 240 days long. _If_
>>> this really needs dealing with, then why is extending this from
>>> 240+ to 480+ days sufficient? I.e. why don''t you simply
>>> change to 64-bit jiffy values, and use time_after_eq64()?
>> Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond
>> MAX_ULONG/2 in 64-bit will need long long time)
>>
>> I think the gap should be think all environment even now extending
480+.
>> if now fall in the gap,  one timer will be pending and replenish will
be
>> in time.  Please run the attachment test program.
>>
>> If use time_after_eq64(), expire ,next_credit and other member will
must
>> be u64.
> Yes, you''ll need to store next_credit as a u64 in vif instead of
> calculating it in tx_credit_exceeded from expires (which is only an
> unsigned long).
I know that.  Even we use u64, time_after_eq()  will also do wrong judge 
in theory (not in reality because need long long time).
I think the two better fixed way is below:
   - By time_before() to judge if now beyond MAX_ULONG/2
   - Add another timer to check and update expire in MAX_ULONG>>2 period.

Because second way isn''t  be verified in practical (need more time to 
waiting jiffes increase),  I chose the first.
>
> David

On 17/10/13 11:19, jianhai luan wrote:
> 
> On 2013-10-17 17:15, David Vrabel wrote:
>> On 17/10/13 10:02, jianhai luan wrote:
>>> On 2013-10-17 16:26, Jan Beulich wrote:
>>>>>>> On 16.10.13 at 19:22, Jason Luan
<jianhai.luan@oracle.com> wrote:
>>>>> time_after_eq() only works if the delta is <
MAX_ULONG/2.
>>>>>
>>>>> If netfront sends at a very low rate, the time between
subsequent
>>>>> calls
>>>>> to tx_credit_exceeded() may exceed MAX_ULONG/2 and the test
for
>>>>> timer_after_eq() will be incorrect.  Credit will not be
replenished
>>>>> and
>>>>> the guest may become unable to send (e.g., if prior to the
long
>>>>> gap, all
>>>>> credit was exhausted).
>>>>>
>>>>> We should add the scenario which now beyond
next_credit+MAX_UNLONG/2.
>>>>> Because
>>>>> the fact now must be not before than expire,
time_before(now, expire)
>>>>> == true
>>>>> will verify the scenario.
>>>>>       time_after_eq(now, next_credit) || time_before (now,
expire)
>>>>>       =>>>>>       !time_in_range_open(now,
expire, next_credit)
>>>> So first of all this must be with a 32-bit netback. And the not
>>>> coverable gap between activity is well over 240 days long. _If_
>>>> this really needs dealing with, then why is extending this from
>>>> 240+ to 480+ days sufficient? I.e. why don''t you
simply
>>>> change to 64-bit jiffy values, and use time_after_eq64()?
>>> Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond
>>> MAX_ULONG/2 in 64-bit will need long long time)
>>>
>>> I think the gap should be think all environment even now extending
480+.
>>> if now fall in the gap,  one timer will be pending and replenish
will be
>>> in time.  Please run the attachment test program.
>>>
>>> If use time_after_eq64(), expire ,next_credit and other member will
must
>>> be u64.
>> Yes, you''ll need to store next_credit as a u64 in vif instead
of
>> calculating it in tx_credit_exceeded from expires (which is only an
>> unsigned long).
> 
> I know that.  Even we use u64, time_after_eq()  will also do wrong judge
> in theory (not in reality because need long long time).
If jiffies_64 has millisecond resolution that would be more than
500,000,000 years.
> I think the two better fixed way is below:
>   - By time_before() to judge if now beyond MAX_ULONG/2
This is broken, so no.

David

On 2013-10-17 18:31, David Vrabel wrote:
> On 17/10/13 11:19, jianhai luan wrote:
>> On 2013-10-17 17:15, David Vrabel wrote:
>>> On 17/10/13 10:02, jianhai luan wrote:
>>>> On 2013-10-17 16:26, Jan Beulich wrote:
>>>>>>>> On 16.10.13 at 19:22, Jason Luan
<jianhai.luan@oracle.com> wrote:
>>>>>> time_after_eq() only works if the delta is <
MAX_ULONG/2.
>>>>>>
>>>>>> If netfront sends at a very low rate, the time between
subsequent
>>>>>> calls
>>>>>> to tx_credit_exceeded() may exceed MAX_ULONG/2 and the
test for
>>>>>> timer_after_eq() will be incorrect.  Credit will not be
replenished
>>>>>> and
>>>>>> the guest may become unable to send (e.g., if prior to
the long
>>>>>> gap, all
>>>>>> credit was exhausted).
>>>>>>
>>>>>> We should add the scenario which now beyond
next_credit+MAX_UNLONG/2.
>>>>>> Because
>>>>>> the fact now must be not before than expire,
time_before(now, expire)
>>>>>> == true
>>>>>> will verify the scenario.
>>>>>>        time_after_eq(now, next_credit) || time_before
(now, expire)
>>>>>>        =>>>>>>       
!time_in_range_open(now, expire, next_credit)
>>>>> So first of all this must be with a 32-bit netback. And the
not
>>>>> coverable gap between activity is well over 240 days long.
_If_
>>>>> this really needs dealing with, then why is extending this
from
>>>>> 240+ to 480+ days sufficient? I.e. why don''t you
simply
>>>>> change to 64-bit jiffy values, and use time_after_eq64()?
>>>> Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond
>>>> MAX_ULONG/2 in 64-bit will need long long time)
>>>>
>>>> I think the gap should be think all environment even now
extending 480+.
>>>> if now fall in the gap,  one timer will be pending and
replenish will be
>>>> in time.  Please run the attachment test program.
>>>>
>>>> If use time_after_eq64(), expire ,next_credit and other member
will must
>>>> be u64.
>>> Yes, you''ll need to store next_credit as a u64 in vif
instead of
>>> calculating it in tx_credit_exceeded from expires (which is only an
>>> unsigned long).
>> I know that.  Even we use u64, time_after_eq()  will also do wrong
judge
>> in theory (not in reality because need long long time).
> If jiffies_64 has millisecond resolution that would be more than
> 500,000,000 years.
Yes, I agree the fact.
>
>> I think the two better fixed way is below:
>>    - By time_before() to judge if now beyond MAX_ULONG/2
> This is broken, so no.
Where is broken?  would you like to help me point it
out.
>
> David

On Thu, Oct 17, 2013 at 09:59:30PM +0800, jianhai luan wrote:
[...]
> >>>>If use time_after_eq64(), expire ,next_credit and other
member will must
> >>>>be u64.
> >>>Yes, you''ll need to store next_credit as a u64 in vif
instead of
> >>>calculating it in tx_credit_exceeded from expires (which is
only an
> >>>unsigned long).
> >>I know that.  Even we use u64, time_after_eq()  will also do wrong
judge
> >>in theory (not in reality because need long long time).
> >If jiffies_64 has millisecond resolution that would be more than
> >500,000,000 years.
> 
> Yes, I agree the fact.
> >
> >>I think the two better fixed way is below:
> >>   - By time_before() to judge if now beyond MAX_ULONG/2
> >This is broken, so no.
> 
> Where is broken?  would you like to help me point it out.
I think David means you didn''t actually fix the problem. Your solution
is
merely a workaround.
> >
> >David

On 2013-10-17 22:06, Wei Liu wrote:
> On Thu, Oct 17, 2013 at 09:59:30PM +0800, jianhai luan wrote:
> [...]
>>>>>> If use time_after_eq64(), expire ,next_credit and other
member will must
>>>>>> be u64.
>>>>> Yes, you''ll need to store next_credit as a u64 in
vif instead of
>>>>> calculating it in tx_credit_exceeded from expires (which is
only an
>>>>> unsigned long).
>>>> I know that.  Even we use u64, time_after_eq()  will also do
wrong judge
>>>> in theory (not in reality because need long long time).
>>> If jiffies_64 has millisecond resolution that would be more than
>>> 500,000,000 years.
>> Yes, I agree the fact.
>>>> I think the two better fixed way is below:
>>>>    - By time_before() to judge if now beyond MAX_ULONG/2
>>> This is broken, so no.
>> Where is broken?  would you like to help me point it out.
> I think David means you didn''t actually fix the problem. Your
solution is
> merely a workaround.
I have think  about using u64, but more code need to be modified and 
that is not all.  Key point is how to change the element of struct 
time_list (expires)  and don''t affect other thing?

David && Wei, would you good idea about modification  of expire (which 
is element of struct time_list)?

Jason
>
>>> David

On 17/10/13 16:23, jianhai luan wrote:
> 
> On 2013-10-17 22:06, Wei Liu wrote:
>> On Thu, Oct 17, 2013 at 09:59:30PM +0800, jianhai luan wrote:
>> [...]
>>>>>>> If use time_after_eq64(), expire ,next_credit and
other member
>>>>>>> will must
>>>>>>> be u64.
>>>>>> Yes, you''ll need to store next_credit as a u64
in vif instead of
>>>>>> calculating it in tx_credit_exceeded from expires
(which is only an
>>>>>> unsigned long).
>>>>> I know that.  Even we use u64, time_after_eq()  will also
do wrong
>>>>> judge
>>>>> in theory (not in reality because need long long time).
>>>> If jiffies_64 has millisecond resolution that would be more
than
>>>> 500,000,000 years.
>>> Yes, I agree the fact.
>>>>> I think the two better fixed way is below:
>>>>>    - By time_before() to judge if now beyond MAX_ULONG/2
>>>> This is broken, so no.
>>> Where is broken?  would you like to help me point it out.
>> I think David means you didn''t actually fix the problem. Your
solution is
>> merely a workaround.
> 
> I have think  about using u64, but more code need to be modified and
> that is not all.  Key point is how to change the element of struct
> time_list (expires)  and don''t affect other thing?
I already suggested a way that didn''t require changing the timer
structure -- calculate and store next_credit in advanced.

David

On 2013-10-17 23:25, David Vrabel wrote:
> On 17/10/13 16:23, jianhai luan wrote:
>> On 2013-10-17 22:06, Wei Liu wrote:
>>> On Thu, Oct 17, 2013 at 09:59:30PM +0800, jianhai luan wrote:
>>> [...]
>>>>>>>> If use time_after_eq64(), expire ,next_credit
and other member
>>>>>>>> will must
>>>>>>>> be u64.
>>>>>>> Yes, you''ll need to store next_credit as a
u64 in vif instead of
>>>>>>> calculating it in tx_credit_exceeded from expires
(which is only an
>>>>>>> unsigned long).
>>>>>> I know that.  Even we use u64, time_after_eq()  will
also do wrong
>>>>>> judge
>>>>>> in theory (not in reality because need long long time).
>>>>> If jiffies_64 has millisecond resolution that would be more
than
>>>>> 500,000,000 years.
>>>> Yes, I agree the fact.
>>>>>> I think the two better fixed way is below:
>>>>>>     - By time_before() to judge if now beyond
MAX_ULONG/2
>>>>> This is broken, so no.
>>>> Where is broken?  would you like to help me point it out.
>>> I think David means you didn''t actually fix the problem.
Your solution is
>>> merely a workaround.
>> I have think  about using u64, but more code need to be modified and
>> that is not all.  Key point is how to change the element of struct
>> time_list (expires)  and don''t affect other thing?
> I already suggested a way that didn''t require changing the timer
> structure -- calculate and store next_credit in advanced.
I think that  modify next_credit only  will not fix the issue. please 
think about:
   - If jiffies have beyond 32 bit. i assume expire  is 0, jiffies_64 
is  0x1000000ff.
     next_credit = 0 +  <always 32-bit value >

     time_after_eq64(jiffies_64, next_credit ) will always true. 
replenish will always do, rate control will lost their function.

Jason
>
> David

On 2013-10-17 16:26, Jan Beulich wrote:
>>>> On 16.10.13 at 19:22, Jason Luan<jianhai.luan@oracle.com>
wrote:
>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>
>> If netfront sends at a very low rate, the time between subsequent calls
>> to tx_credit_exceeded() may exceed MAX_ULONG/2 and the test for
>> timer_after_eq() will be incorrect.  Credit will not be replenished and
>> the guest may become unable to send (e.g., if prior to the long gap,
all
>> credit was exhausted).
>>
>> We should add the scenario which now beyond next_credit+MAX_UNLONG/2.
Because
>> the fact now must be not before than expire, time_before(now, expire)
== true
>> will verify the scenario.
>>      time_after_eq(now, next_credit) || time_before (now, expire)
>>      =>>      !time_in_range_open(now, expire, next_credit)
> So first of all this must be with a 32-bit netback. And the not
> coverable gap between activity is well over 240 days long. _If_
> this really needs dealing with, then why is extending this from
> 240+ to 480+ days sufficient?
I am not so sure your mean about extending from 240+ to 480+. Do you 
mean "now" wrapped case happens and falls into the range of from
expires
to next_credit? If this happens, the timer would be set with value based 
on next_credit, which is actually implements the rate control.

Thanks
Annie
> I.e. why don''t you simply
> change to 64-bit jiffy values, and use time_after_eq64()?
>
> Jan
>
>> Signed-off-by: Jason Luan<jianhai.luan@oracle.com>
>> ---
>>   drivers/net/xen-netback/netback.c |    7 +++++--
>>   1 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/netback.c
>> b/drivers/net/xen-netback/netback.c
>> index f3e591c..31eedaf 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -1194,8 +1194,11 @@ static bool tx_credit_exceeded(struct xenvif
*vif,
>> unsigned size)
>>   	if (timer_pending(&vif->credit_timeout))
>>   		return true;
>>   
>> -	/* Passed the point where we can replenish credit? */
>> -	if (time_after_eq(now, next_credit)) {
>> +	/* Credit should be replenished when now does not fall into the
>> +	 * range from expires to next_credit, and time_in_range_open()
>> +	 * is used to verify whether this case happens.
>> +	 */
>> +	if (!time_in_range_open(now, vif->credit_timeout.expires,
next_credit)) {
>>   		vif->credit_timeout.expires = now;
>>   		tx_add_credit(vif);
>>   	}
>> -- 
>> 1.7.6.5
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org  
>> http://lists.xen.org/xen-devel  
>

On 2013-10-17 17:26, Jan Beulich wrote:
>> Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond
>> MAX_ULONG/2 in 64-bit will need long long time)
>>
>> I think the gap should be think all environment even now extending
480+.
>> if now fall in the gap,  one timer will be pending and replenish will
be
>> in time.  Please run the attachment test program.
> Not sure what this is supposed to tell me. I recognize that there
> are overflow conditions not handled properly, but (a) I have a
> hard time thinking of a sensible guest that sits idle for over 240
> days (host uptime usually isn''t even coming close to that due to
> maintenance requirements) and (b) if there is such a sensible
> guest, then I can''t see why dealing with one being idle for over
> 480 days should be required too.
>
If the guest contains multiple NICs, that situation probably happens 
when one NIC keeps idle and others work under load. BTW, how do you get 
the 240?

Thanks
Annie

On Fri, Oct 18, 2013 at 12:38:12AM +0800, annie li
wrote:
> 
> On 2013-10-17 17:26, Jan Beulich wrote:
> >>Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond
> >>MAX_ULONG/2 in 64-bit will need long long time)
> >>
> >>I think the gap should be think all environment even now extending
480+.
> >>if now fall in the gap,  one timer will be pending and replenish
will be
> >>in time.  Please run the attachment test program.
> >Not sure what this is supposed to tell me. I recognize that there
> >are overflow conditions not handled properly, but (a) I have a
> >hard time thinking of a sensible guest that sits idle for over 240
> >days (host uptime usually isn''t even coming close to that due
to
> >maintenance requirements) and (b) if there is such a sensible
> >guest, then I can''t see why dealing with one being idle for
over
> >480 days should be required too.
> >
> 
> If the guest contains multiple NICs, that situation probably happens
> when one NIC keeps idle and others work under load. BTW, how do you
> get the 240?
> 
I think Jan got this number with HZ=100. It take ~240 days for jiffies
to overflow in 32 bit machine when HZ=100.

Wei.
> Thanks
> Annie

On 2013-10-18 0:41, Wei Liu wrote:
> On Fri, Oct 18, 2013 at 12:38:12AM +0800, annie li wrote:
>> On 2013-10-17 17:26, Jan Beulich wrote:
>>>> Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond
>>>> MAX_ULONG/2 in 64-bit will need long long time)
>>>>
>>>> I think the gap should be think all environment even now
extending 480+.
>>>> if now fall in the gap,  one timer will be pending and
replenish will be
>>>> in time.  Please run the attachment test program.
>>> Not sure what this is supposed to tell me. I recognize that there
>>> are overflow conditions not handled properly, but (a) I have a
>>> hard time thinking of a sensible guest that sits idle for over 240
>>> days (host uptime usually isn''t even coming close to that
due to
>>> maintenance requirements) and (b) if there is such a sensible
>>> guest, then I can''t see why dealing with one being idle
for over
>>> 480 days should be required too.
>>>
>> If the guest contains multiple NICs, that situation probably happens
>> when one NIC keeps idle and others work under load. BTW, how do you
>> get the 240?
>>
> I think Jan got this number with HZ=100. It take ~240 days for jiffies
> to overflow in 32 bit machine when HZ=100.
If HZ is larger then the days would be less, for example, HZ=250, the 
days would be ~99. It is possible to hit overflow in environment where 
multiple NICs coexist and one NIC keeps idle all the time. Moreover, 
this patch does not extend to the days doubled, the timer is set with 
value based on next_credit.

Thanks
Annie

On 2013-10-17 23:41, jianhai luan wrote:
>
> On 2013-10-17 23:25, David Vrabel wrote:
>> On 17/10/13 16:23, jianhai luan wrote:
>>> On 2013-10-17 22:06, Wei Liu wrote:
>>>> On Thu, Oct 17, 2013 at 09:59:30PM +0800, jianhai luan wrote:
>>>> [...]
>>>>>>>>> If use time_after_eq64(), expire
,next_credit and other member
>>>>>>>>> will must
>>>>>>>>> be u64.
>>>>>>>> Yes, you''ll need to store next_credit
as a u64 in vif instead of
>>>>>>>> calculating it in tx_credit_exceeded from
expires (which is
>>>>>>>> only an
>>>>>>>> unsigned long).
>>>>>>> I know that.  Even we use u64, time_after_eq() 
will also do wrong
>>>>>>> judge
>>>>>>> in theory (not in reality because need long long
time).
>>>>>> If jiffies_64 has millisecond resolution that would be
more than
>>>>>> 500,000,000 years.
>>>>> Yes, I agree the fact.
>>>>>>> I think the two better fixed way is below:
>>>>>>>     - By time_before() to judge if now beyond
MAX_ULONG/2
>>>>>> This is broken, so no.
>>>>> Where is broken?  would you like to help me point it out.
>>>> I think David means you didn''t actually fix the
problem. Your
>>>> solution is
>>>> merely a workaround.
>>> I have think  about using u64, but more code need to be modified
and
>>> that is not all.  Key point is how to change the element of struct
>>> time_list (expires)  and don''t affect other thing?
>> I already suggested a way that didn''t require changing the
timer
>> structure -- calculate and store next_credit in advanced.
> I think that  modify next_credit only  will not fix the issue. please 
> think about:
>   - If jiffies have beyond 32 bit. i assume expire  is 0, jiffies_64 
> is  0x1000000ff.
>     next_credit = 0 +  <always 32-bit value >
>
>     time_after_eq64(jiffies_64, next_credit ) will always true. 
> replenish will always do, rate control will lost their function.
At first, the case above only exists when the network device keep idle 
for a long time, not frequently happens. If this case really happens, it 
means lots of jiffies are available for the credit, so there is no 
necessary to add the timer. The code operates correctly and rate control 
does not lose. This case can be shown with following config,
------old_next_credit(expires replaced)----------next_credit--------now----

So till now, two solutions are available: one is the current one to 
change if condition, another is to change all connected variant to 64. I 
incline to the former one since it involves less code change than the 
latter one.

Thanks
Annie

>>> On 17.10.13 at 18:21, annie li <annie.li@oracle.com> wrote:
> On 2013-10-17 16:26, Jan Beulich wrote:
>> So first of all this must be with a 32-bit netback. And the not
>> coverable gap between activity is well over 240 days long. _If_
>> this really needs dealing with, then why is extending this from
>> 240+ to 480+ days sufficient?
> 
> I am not so sure your mean about extending from 240+ to 480+. Do you 
> mean "now" wrapped case happens and falls into the range of from
expires
> to next_credit? If this happens, the timer would be set with value based 
> on next_credit, which is actually implements the rate control.
My point was simply that doubling the span the code can cover
is pointless - either 240 days is long enough, of 480 days isn''t
either.

Jan

>>> On 17.10.13 at 18:38, annie li <annie.li@oracle.com> wrote:
> On 2013-10-17 17:26, Jan Beulich wrote:
>>> Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond
>>> MAX_ULONG/2 in 64-bit will need long long time)
>>>
>>> I think the gap should be think all environment even now extending
480+.
>>> if now fall in the gap,  one timer will be pending and replenish
will be
>>> in time.  Please run the attachment test program.
>> Not sure what this is supposed to tell me. I recognize that there
>> are overflow conditions not handled properly, but (a) I have a
>> hard time thinking of a sensible guest that sits idle for over 240
>> days (host uptime usually isn''t even coming close to that due
to
>> maintenance requirements) and (b) if there is such a sensible
>> guest, then I can''t see why dealing with one being idle for
over
>> 480 days should be required too.
>>
> 
> If the guest contains multiple NICs, that situation probably happens 
> when one NIC keeps idle and others work under load. BTW, how do you get 
> the 240?
2^31 / 100 / 60 / 60 / 24

Obviously with HZ=1000 the span would be smaller by a factor
of 10, which would make it even more clear that doubling the
span doesn''t really help.

Jan

On 2013-10-18 15:43, Jan Beulich wrote:
>>>> On 17.10.13 at 18:38, annie li <annie.li@oracle.com>
wrote:
>> On 2013-10-17 17:26, Jan Beulich wrote:
>>>> Yes, the issue only can be  reproduced in 32-bit Dom0 (Beyond
>>>> MAX_ULONG/2 in 64-bit will need long long time)
>>>>
>>>> I think the gap should be think all environment even now
extending 480+.
>>>> if now fall in the gap,  one timer will be pending and
replenish will be
>>>> in time.  Please run the attachment test program.
>>> Not sure what this is supposed to tell me. I recognize that there
>>> are overflow conditions not handled properly, but (a) I have a
>>> hard time thinking of a sensible guest that sits idle for over 240
>>> days (host uptime usually isn''t even coming close to that
due to
>>> maintenance requirements) and (b) if there is such a sensible
>>> guest, then I can''t see why dealing with one being idle
for over
>>> 480 days should be required too.
>>>
>> If the guest contains multiple NICs, that situation probably happens
>> when one NIC keeps idle and others work under load. BTW, how do you get
>> the 240?
> 2^31 / 100 / 60 / 60 / 24
>
> Obviously with HZ=1000 the span would be smaller by a factor
> of 10, which would make it even more clear that doubling the
> span doesn''t really help.
My understanding is this patch does not simply double the span, it is 
just stricter than the original one. Please check my previous comments, 
I paste it here.

The main change of this patch is copied here too,
if (!time_in_range_open(now, vif->credit_timeout.expires, next_credit))

comments:

----------expires-------now-------credit----------    is the only case 
where we need to add a timer.

Other cases like following would match the if condition above, then no 
timer is added.
----------expires----------credit------now------
-----now-----expires----------credit----------

Or we can consider the extreme condition, when the rate control does not 
exist, "credit_usec" is zero, and "next_credit" is equal to
"expires".
The above if condition would cover all conditions, and no rate control 
really happens. If credit_usec is not zero, the "if condition" would 
cover the range outside of that from expires to next_credit.

Even if "now" is wrapped again into the range from "expires"
to
"next_credit", the "next_credit" that is set in __mod_timer
is
reasonable value(this can be gotten from credit_usec), and the timer 
would be hit soon.

Thanks
Annie

>>> On 18.10.13 at 10:14, annie li <annie.li@oracle.com> wrote:
> On 2013-10-18 15:43, Jan Beulich wrote:
>>>>> On 17.10.13 at 18:38, annie li <annie.li@oracle.com>
wrote:
>>> On 2013-10-17 17:26, Jan Beulich wrote:
>>>>> Yes, the issue only can be  reproduced in 32-bit Dom0
(Beyond
>>>>> MAX_ULONG/2 in 64-bit will need long long time)
>>>>>
>>>>> I think the gap should be think all environment even now
extending 480+.
>>>>> if now fall in the gap,  one timer will be pending and
replenish will be
>>>>> in time.  Please run the attachment test program.
>>>> Not sure what this is supposed to tell me. I recognize that
there
>>>> are overflow conditions not handled properly, but (a) I have a
>>>> hard time thinking of a sensible guest that sits idle for over
240
>>>> days (host uptime usually isn''t even coming close to
that due to
>>>> maintenance requirements) and (b) if there is such a sensible
>>>> guest, then I can''t see why dealing with one being
idle for over
>>>> 480 days should be required too.
>>>>
>>> If the guest contains multiple NICs, that situation probably
happens
>>> when one NIC keeps idle and others work under load. BTW, how do you
get
>>> the 240?
>> 2^31 / 100 / 60 / 60 / 24
>>
>> Obviously with HZ=1000 the span would be smaller by a factor
>> of 10, which would make it even more clear that doubling the
>> span doesn''t really help.
> 
> My understanding is this patch does not simply double the span, it is 
> just stricter than the original one. Please check my previous comments, 
> I paste it here.
No, the code (on a 32-bit arch) just _can''t_ handle jiffies differences
beyond 2^32, no matter how cleverly you use the respective macros.
All arithmetic there is done modulo 2^32.
> ----------expires-------now-------credit----------    is the only case 
> where we need to add a timer.
> 
> Other cases like following would match the if condition above, then no 
> timer is added.
> ----------expires----------credit------now------
> -----now-----expires----------credit----------
The problem with these graphs is that you fail to take the finite
number range (and hence the modulo arithmetic) into account.

Jan

> > My understanding is this patch does not simply double the span, it is
> > just stricter than the original one. Please check my previous
comments,
> > I paste it here.
> 
> No, the code (on a 32-bit arch) just _can''t_ handle jiffies
differences
> beyond 2^32, no matter how cleverly you use the respective macros.
> All arithmetic there is done modulo 2^32.
I haven''t followed this discussion very closely but it might be
possible
to arrange that the ''incorrect lack of credit'' only occurs for
a few
seconds every time ''jiffies'' wraps - instead of half of the
time.
Then you''d have to be extremely unlucky to hit the timing window.

	David

On 2013-10-18 16:26, Jan Beulich wrote:
>>>> On 18.10.13 at 10:14, annie li <annie.li@oracle.com>
wrote:
>> On 2013-10-18 15:43, Jan Beulich wrote:
>>>>>> On 17.10.13 at 18:38, annie li
<annie.li@oracle.com> wrote:
>>>> On 2013-10-17 17:26, Jan Beulich wrote:
>>>>>> Yes, the issue only can be  reproduced in 32-bit Dom0
(Beyond
>>>>>> MAX_ULONG/2 in 64-bit will need long long time)
>>>>>>
>>>>>> I think the gap should be think all environment even
now extending 480+.
>>>>>> if now fall in the gap,  one timer will be pending and
replenish will be
>>>>>> in time.  Please run the attachment test program.
>>>>> Not sure what this is supposed to tell me. I recognize that
there
>>>>> are overflow conditions not handled properly, but (a) I
have a
>>>>> hard time thinking of a sensible guest that sits idle for
over 240
>>>>> days (host uptime usually isn''t even coming close
to that due to
>>>>> maintenance requirements) and (b) if there is such a
sensible
>>>>> guest, then I can''t see why dealing with one being
idle for over
>>>>> 480 days should be required too.
>>>>>
>>>> If the guest contains multiple NICs, that situation probably
happens
>>>> when one NIC keeps idle and others work under load. BTW, how do
you get
>>>> the 240?
>>> 2^31 / 100 / 60 / 60 / 24
>>>
>>> Obviously with HZ=1000 the span would be smaller by a factor
>>> of 10, which would make it even more clear that doubling the
>>> span doesn''t really help.
>> My understanding is this patch does not simply double the span, it is
>> just stricter than the original one. Please check my previous comments,
>> I paste it here.
> No, the code (on a 32-bit arch) just _can''t_ handle jiffies
differences
> beyond 2^32, no matter how cleverly you use the respective macros.
> All arithmetic there is done modulo 2^32.
On 32-bit arch, the jiffies difference beyond 2^32 is only in theory, 
and the real value of jiffies would wrap around after 2^32. Then it will 
still be verified by time_in_range_open, the code will either replenish 
the credit or hit the timer soon.

Thanks
Annie

On Fri, Oct 18, 2013 at 09:40:33AM +0100, David Laight
wrote:
> > > My understanding is this patch does not simply double the span,
it is
> > > just stricter than the original one. Please check my previous
comments,
> > > I paste it here.
> > 
> > No, the code (on a 32-bit arch) just _can''t_ handle jiffies
differences
> > beyond 2^32, no matter how cleverly you use the respective macros.
> > All arithmetic there is done modulo 2^32.
> 
> I haven''t followed this discussion very closely but it might be
possible
> to arrange that the ''incorrect lack of credit'' only
occurs for a few
> seconds every time ''jiffies'' wraps - instead of half of
the time.
> Then you''d have to be extremely unlucky to hit the timing window.
> 
As I understand it, this is the idea of this patch -- to narrow down the
timing window.

Wei.
> 	David
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev"
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 2013-10-18 19:24, Wei Liu wrote:
> On Fri, Oct 18, 2013 at 09:40:33AM +0100, David Laight wrote:
>>>> My understanding is this patch does not simply double the span,
it is
>>>> just stricter than the original one. Please check my previous
comments,
>>>> I paste it here.
>>> No, the code (on a 32-bit arch) just _can''t_ handle
jiffies differences
>>> beyond 2^32, no matter how cleverly you use the respective macros.
>>> All arithmetic there is done modulo 2^32.
>> I haven''t followed this discussion very closely but it might
be possible
>> to arrange that the ''incorrect lack of credit'' only
occurs for a few
>> seconds every time ''jiffies'' wraps - instead of half
of the time.
>> Then you''d have to be extremely unlucky to hit the timing
window.
>>
> As I understand it, this is the idea of this patch -- to narrow down the
> timing window.
Jan,  do you agree the idea or have better suggestion to me.

Thanks,
Jason
>
> Wei.
>
>> 	David
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

>>> jianhai luan <jianhai.luan@oracle.com> 10/23/13 10:02 AM
>>>
>On 2013-10-18 19:24, Wei Liu wrote:
>> On Fri, Oct 18, 2013 at 09:40:33AM +0100, David Laight wrote:
>>>>> My understanding is this patch does not simply double the
span, it is
>>>>> just stricter than the original one. Please check my
previous comments,
>>>>> I paste it here.
>>>> No, the code (on a 32-bit arch) just _can''t_ handle
jiffies differences
>>>> beyond 2^32, no matter how cleverly you use the respective
macros.
>>>> All arithmetic there is done modulo 2^32.
>>> I haven''t followed this discussion very closely but it
might be possible
>>> to arrange that the ''incorrect lack of credit''
only occurs for a few
>>> seconds every time ''jiffies'' wraps - instead of
half of the time.
>>> Then you''d have to be extremely unlucky to hit the timing
window.
>>>
>> As I understand it, this is the idea of this patch -- to narrow down
the
>> timing window.
>Jan,  do you agree the idea or have better suggestion to me.
As said before - I disagree (reducing a timing window is never a solution,
only eliminating it is), and I pointed at the alternative (using 64-bit
calculations) before.

Jan

> >> As I understand it, this is the idea of this patch -- to narrow
down the
> >> timing window.
> >Jan,  do you agree the idea or have better suggestion to me.
> 
> As said before - I disagree (reducing a timing window is never a solution,
> only eliminating it is), and I pointed at the alternative (using 64-bit
> calculations) before.
If the error is reduced to a few seconds in 48 days (at 1kHz HZ) and the
system has to be idle for the 48 days I wouldn''t be too worried.
Especially if this is ''time based credit'' and the effect is
just that
of the link (or whatever) being busy rather than idle.

	David

>>> As I understand it, this is the idea of this patch -- to narrow
down the
>>> timing window.
>> Jan,  do you agree the idea or have better suggestion to me.
> As said before - I disagree (reducing a timing window is never a solution,
> only eliminating it is), and I pointed at the alternative (using 64-bit
> calculations) before.
Your mean is that add u64 member into struct xenvif or add static 
variable to keep expire?

Jason
>
> Jan
>