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

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

[Jason Luan]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[Jan Beulich]

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

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

[Jan Beulich]

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

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

[jianhai luan]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[jianhai luan]

[-- Attachment #1: Type: text/plain, Size: 2985 bytes --]


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


[-- Attachment #2: main.c --]
[-- Type: text/plain, Size: 961 bytes --]

#include <stdio.h>

#define typecheck(type,x) \
({	type __dummy; \
	typeof(x) __dummy2; \
	(void)(&__dummy == &__dummy2); \
	1; \
})

#define time_after(a, b)		\
	(typecheck(unsigned char, a) && \
	 typecheck(unsigned char, b) && \
	 ((char)((b) - (a)) < 0))
#define time_before(a,b)	time_after(b,a)

#define time_after_eq(a,b)		\
	(typecheck(unsigned char, a) && \
	 typecheck(unsigned char, b) && \
	 ((char)((a) -(b)) >= 0))
#define time_before_eq(a, b) time_after_eq(b,a)

void do_nothing()
{
	return;
}

int main()
{
	unsigned char expire, now, next;
	unsigned char delta = 10;
	int i, j;

	for(i = 0; i < 256; i++) {
		expire = i;
		next = expire + delta;

		printf("\n\n\n[%u ... %u]\n", expire, next);
		now = expire;
		for(j=0; j < 1024; j++, now++) {	
			if(j%256 == 0) printf("\n");

			if (time_after_eq(now, next) ||
				time_before(now, expire)) {
				do_nothing();
			}
			else {
				printf("    now=%d\n", (char)now);
			}
		}
	}
	
	return 0;
}

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[jianhai luan]

[-- Attachment #1: Type: text/plain, Size: 2985 bytes --]


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


[-- Attachment #2: main.c --]
[-- Type: text/plain, Size: 961 bytes --]

#include <stdio.h>

#define typecheck(type,x) \
({	type __dummy; \
	typeof(x) __dummy2; \
	(void)(&__dummy == &__dummy2); \
	1; \
})

#define time_after(a, b)		\
	(typecheck(unsigned char, a) && \
	 typecheck(unsigned char, b) && \
	 ((char)((b) - (a)) < 0))
#define time_before(a,b)	time_after(b,a)

#define time_after_eq(a,b)		\
	(typecheck(unsigned char, a) && \
	 typecheck(unsigned char, b) && \
	 ((char)((a) -(b)) >= 0))
#define time_before_eq(a, b) time_after_eq(b,a)

void do_nothing()
{
	return;
}

int main()
{
	unsigned char expire, now, next;
	unsigned char delta = 10;
	int i, j;

	for(i = 0; i < 256; i++) {
		expire = i;
		next = expire + delta;

		printf("\n\n\n[%u ... %u]\n", expire, next);
		now = expire;
		for(j=0; j < 1024; j++, now++) {	
			if(j%256 == 0) printf("\n");

			if (time_after_eq(now, next) ||
				time_before(now, expire)) {
				do_nothing();
			}
			else {
				printf("    now=%d\n", (char)now);
			}
		}
	}
	
	return 0;
}

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

[David Vrabel]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[jianhai luan]

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

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

[jianhai luan]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[David Vrabel]

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

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

[David Vrabel]

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

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

[jianhai luan]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[Wei Liu]

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

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

[Wei Liu]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[jianhai luan]

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

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

[jianhai luan]

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

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

[David Vrabel]

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

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

[jianhai luan]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[annie li]

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

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

[annie li]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[jianhai luan]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[David Vrabel]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[jianhai luan]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[David Vrabel]

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

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

[Jan Beulich]

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

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

[jianhai luan]

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
>

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[jianhai luan]

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
>

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

[annie li]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[Wei Liu]

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

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

[Wei Liu]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[annie li]

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

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

[annie li]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[Jan Beulich]

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

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

[Jan Beulich]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[annie li]

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

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

[annie li]

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

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

[Jan Beulich]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[David Laight]

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

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

[David Laight]

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

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

[Wei Liu]

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

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

[jianhai luan]

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

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

[Jan Beulich]

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

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

[David Laight]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[David Laight]

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

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

[jianhai luan]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[jianhai luan]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[Jan Beulich]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[jianhai luan]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[Wei Liu]

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

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

[annie li]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[annie li]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[Jan Beulich]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[annie li]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[Jan Beulich]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[jianhai luan]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[annie li]

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  
>

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

[annie li]

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  
>

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

[Jan Beulich]

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

Re: [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().

[Jan Beulich]

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