* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 9:02 ` jianhai luan
@ 2013-10-17 9:04 ` jianhai luan
2013-10-17 9:15 ` David Vrabel
2013-10-17 9:26 ` Jan Beulich
2 siblings, 0 replies; 30+ messages in thread
From: jianhai luan @ 2013-10-17 9:04 UTC (permalink / raw)
To: Jan Beulich
Cc: david.vrabel, ian.campbell, wei.liu2, xen-devel, annie.li, netdev
[-- 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;
}
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 9:02 ` jianhai luan
2013-10-17 9:04 ` jianhai luan
@ 2013-10-17 9:15 ` David Vrabel
2013-10-17 10:19 ` jianhai luan
2013-10-17 9:26 ` Jan Beulich
2 siblings, 1 reply; 30+ messages in thread
From: David Vrabel @ 2013-10-17 9:15 UTC (permalink / raw)
To: jianhai luan
Cc: Jan Beulich, ian.campbell, wei.liu2, xen-devel, annie.li, netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 9:15 ` David Vrabel
@ 2013-10-17 10:19 ` jianhai luan
2013-10-17 10:31 ` David Vrabel
0 siblings, 1 reply; 30+ messages in thread
From: jianhai luan @ 2013-10-17 10:19 UTC (permalink / raw)
To: David Vrabel
Cc: Jan Beulich, ian.campbell, wei.liu2, xen-devel, annie.li, netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 10:19 ` jianhai luan
@ 2013-10-17 10:31 ` David Vrabel
2013-10-17 13:59 ` jianhai luan
0 siblings, 1 reply; 30+ messages in thread
From: David Vrabel @ 2013-10-17 10:31 UTC (permalink / raw)
To: jianhai luan
Cc: Jan Beulich, ian.campbell, wei.liu2, xen-devel, annie.li, netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 10:31 ` David Vrabel
@ 2013-10-17 13:59 ` jianhai luan
2013-10-17 14:06 ` Wei Liu
0 siblings, 1 reply; 30+ messages in thread
From: jianhai luan @ 2013-10-17 13:59 UTC (permalink / raw)
To: David Vrabel
Cc: Jan Beulich, ian.campbell, wei.liu2, xen-devel, annie.li, netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 13:59 ` jianhai luan
@ 2013-10-17 14:06 ` Wei Liu
2013-10-17 15:23 ` jianhai luan
0 siblings, 1 reply; 30+ messages in thread
From: Wei Liu @ 2013-10-17 14:06 UTC (permalink / raw)
To: jianhai luan
Cc: David Vrabel, Jan Beulich, ian.campbell, wei.liu2, xen-devel,
annie.li, netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 14:06 ` Wei Liu
@ 2013-10-17 15:23 ` jianhai luan
2013-10-17 15:25 ` David Vrabel
0 siblings, 1 reply; 30+ messages in thread
From: jianhai luan @ 2013-10-17 15:23 UTC (permalink / raw)
To: Wei Liu
Cc: David Vrabel, Jan Beulich, ian.campbell, xen-devel, annie.li,
netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 15:23 ` jianhai luan
@ 2013-10-17 15:25 ` David Vrabel
2013-10-17 15:41 ` jianhai luan
0 siblings, 1 reply; 30+ messages in thread
From: David Vrabel @ 2013-10-17 15:25 UTC (permalink / raw)
To: jianhai luan
Cc: Wei Liu, Jan Beulich, ian.campbell, xen-devel, annie.li, netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 15:25 ` David Vrabel
@ 2013-10-17 15:41 ` jianhai luan
2013-10-18 6:48 ` annie li
0 siblings, 1 reply; 30+ messages in thread
From: jianhai luan @ 2013-10-17 15:41 UTC (permalink / raw)
To: David Vrabel
Cc: Wei Liu, Jan Beulich, ian.campbell, xen-devel, annie.li, netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 15:41 ` jianhai luan
@ 2013-10-18 6:48 ` annie li
0 siblings, 0 replies; 30+ messages in thread
From: annie li @ 2013-10-18 6:48 UTC (permalink / raw)
To: jianhai luan
Cc: David Vrabel, Wei Liu, ian.campbell, netdev, Jan Beulich,
xen-devel
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 9:02 ` jianhai luan
2013-10-17 9:04 ` jianhai luan
2013-10-17 9:15 ` David Vrabel
@ 2013-10-17 9:26 ` Jan Beulich
2013-10-17 9:59 ` jianhai luan
2013-10-17 16:38 ` annie li
2 siblings, 2 replies; 30+ messages in thread
From: Jan Beulich @ 2013-10-17 9:26 UTC (permalink / raw)
To: jianhai luan
Cc: david.vrabel, ian.campbell, wei.liu2, xen-devel, annie.li, netdev
>>> 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 9:26 ` Jan Beulich
@ 2013-10-17 9:59 ` jianhai luan
2013-10-17 16:38 ` annie li
1 sibling, 0 replies; 30+ messages in thread
From: jianhai luan @ 2013-10-17 9:59 UTC (permalink / raw)
To: Jan Beulich
Cc: david.vrabel, ian.campbell, wei.liu2, xen-devel, annie.li, netdev
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
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 9:26 ` Jan Beulich
2013-10-17 9:59 ` jianhai luan
@ 2013-10-17 16:38 ` annie li
2013-10-17 16:41 ` Wei Liu
2013-10-18 7:43 ` Jan Beulich
1 sibling, 2 replies; 30+ messages in thread
From: annie li @ 2013-10-17 16:38 UTC (permalink / raw)
To: Jan Beulich
Cc: jianhai luan, david.vrabel, ian.campbell, wei.liu2, xen-devel,
netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 16:38 ` annie li
@ 2013-10-17 16:41 ` Wei Liu
2013-10-18 1:59 ` annie li
2013-10-18 7:43 ` Jan Beulich
1 sibling, 1 reply; 30+ messages in thread
From: Wei Liu @ 2013-10-17 16:41 UTC (permalink / raw)
To: annie li
Cc: Jan Beulich, jianhai luan, david.vrabel, ian.campbell, wei.liu2,
xen-devel, netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 16:41 ` Wei Liu
@ 2013-10-18 1:59 ` annie li
0 siblings, 0 replies; 30+ messages in thread
From: annie li @ 2013-10-18 1:59 UTC (permalink / raw)
To: Wei Liu
Cc: Jan Beulich, jianhai luan, david.vrabel, ian.campbell, xen-devel,
netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-17 16:38 ` annie li
2013-10-17 16:41 ` Wei Liu
@ 2013-10-18 7:43 ` Jan Beulich
2013-10-18 8:14 ` annie li
1 sibling, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2013-10-18 7:43 UTC (permalink / raw)
To: annie li
Cc: david.vrabel, ian.campbell, wei.liu2, xen-devel, jianhai luan,
netdev
>>> 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-18 7:43 ` Jan Beulich
@ 2013-10-18 8:14 ` annie li
2013-10-18 8:26 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: annie li @ 2013-10-18 8:14 UTC (permalink / raw)
To: Jan Beulich
Cc: wei.liu2, ian.campbell, netdev, david.vrabel, xen-devel,
jianhai luan
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-18 8:14 ` annie li
@ 2013-10-18 8:26 ` Jan Beulich
2013-10-18 8:40 ` David Laight
2013-10-18 8:55 ` annie li
0 siblings, 2 replies; 30+ messages in thread
From: Jan Beulich @ 2013-10-18 8:26 UTC (permalink / raw)
To: annie li
Cc: david.vrabel, ian.campbell, wei.liu2, xen-devel, jianhai luan,
netdev
>>> 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-18 8:26 ` Jan Beulich
@ 2013-10-18 8:40 ` David Laight
2013-10-18 11:24 ` Wei Liu
2013-10-18 8:55 ` annie li
1 sibling, 1 reply; 30+ messages in thread
From: David Laight @ 2013-10-18 8:40 UTC (permalink / raw)
To: Jan Beulich, annie li
Cc: david.vrabel, ian.campbell, wei.liu2, xen-devel, jianhai luan,
netdev
> > 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-18 8:40 ` David Laight
@ 2013-10-18 11:24 ` Wei Liu
2013-10-23 8:02 ` jianhai luan
0 siblings, 1 reply; 30+ messages in thread
From: Wei Liu @ 2013-10-18 11:24 UTC (permalink / raw)
To: David Laight
Cc: Jan Beulich, annie li, david.vrabel, ian.campbell, wei.liu2,
xen-devel, jianhai luan, netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-18 11:24 ` Wei Liu
@ 2013-10-23 8:02 ` jianhai luan
2013-10-23 16:07 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: jianhai luan @ 2013-10-23 8:02 UTC (permalink / raw)
To: Wei Liu, David Laight
Cc: Jan Beulich, annie li, david.vrabel, ian.campbell, xen-devel,
netdev
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-23 8:02 ` jianhai luan
@ 2013-10-23 16:07 ` Jan Beulich
2013-10-24 10:04 ` David Laight
2013-10-24 11:34 ` jianhai luan
0 siblings, 2 replies; 30+ messages in thread
From: Jan Beulich @ 2013-10-23 16:07 UTC (permalink / raw)
To: David.Laight, wei.liu2, jianhai.luan
Cc: david.vrabel, ian.campbell, xen-devel, annie.li, netdev
>>> 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-23 16:07 ` Jan Beulich
@ 2013-10-24 10:04 ` David Laight
2013-10-24 11:34 ` jianhai luan
1 sibling, 0 replies; 30+ messages in thread
From: David Laight @ 2013-10-24 10:04 UTC (permalink / raw)
To: Jan Beulich, wei.liu2, jianhai.luan
Cc: david.vrabel, ian.campbell, xen-devel, annie.li, netdev
> >> 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-23 16:07 ` Jan Beulich
2013-10-24 10:04 ` David Laight
@ 2013-10-24 11:34 ` jianhai luan
1 sibling, 0 replies; 30+ messages in thread
From: jianhai luan @ 2013-10-24 11:34 UTC (permalink / raw)
To: Jan Beulich, David.Laight, wei.liu2
Cc: david.vrabel, ian.campbell, xen-devel, annie.li, netdev
>>> 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
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Xen-devel] [PATCH net] xen-netback: add the scenario which now beyond the range time_after_eq().
2013-10-18 8:26 ` Jan Beulich
2013-10-18 8:40 ` David Laight
@ 2013-10-18 8:55 ` annie li
1 sibling, 0 replies; 30+ messages in thread
From: annie li @ 2013-10-18 8:55 UTC (permalink / raw)
To: Jan Beulich
Cc: wei.liu2, ian.campbell, netdev, david.vrabel, xen-devel,
jianhai luan
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
^ permalink raw reply [flat|nested] 30+ messages in thread