diff mbox

DomU's network interface will hung when Dom0 running 32bit

Message ID 525D0C41.2080407@oracle.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

jianhai luan Oct. 15, 2013, 9:34 a.m. UTC 
On 2013-10-15 16:43, Ian Campbell wrote:
> On Tue, 2013-10-15 at 10:44 +0800, jianhai luan wrote:
>> On 2013-10-14 19:19, Wei Liu wrote:
>>> On Sat, Oct 12, 2013 at 04:53:18PM +0800, jianhai luan wrote:
>>>> Hi Ian,
>>>>     I meet the DomU's network interface hung issue recently, and have
>>>> been working on the issue from that time. I find that DomU's network
>>>> interface, which send lesser package, will hung if Dom0 running
>>>> 32bit and DomU's up-time is very long.  I think that one jiffies
>>>> overflow bug exist in the function tx_credit_exceeded().
>>>>     I know the inline function time_after_eq(a,b) will process jiffies
>>>> overflow, but the function have one limit a should little that (b +
>>>> MAX_SIGNAL_LONG). If a large than the value, time_after_eq will
>>>> return false. The MAX_SINGNAL_LONG should be 0x7fffffff at 32-bit
>>>> machine.
>>>>     If DomU's network interface send lesser package (<0.5k/s if
>>>> jiffies=250 and credit_bytes=ULONG_MAX), jiffies will beyond out
>>>> (credit_timeout.expires + MAX_SIGNAL_LONG) and time_after_eq(now,
>>>> next_credit) will failure (should be true). So one timer which will
>>>> not be trigger in short time, and later process will be aborted when
>>>> timer_pending(&vif->credit_timeout) is true. The result will be
>>>> DomU's network interface will be hung in long time (> 40days).
>>>>     Please think about the below scenario:
>>>>     Condition:
>>>>       Dom0 running 32-bit and HZ = 1000
>>>>       vif->credit_timeout->expire = 0xffffffff, vif->remaining_credit
>>>> = 0xffffffff, vif->credit_usec=0 jiffies=0
>>>>       vif receive lesser package (DomU send lesser package). If the
>>>> value is litter than 2K/s, consume 4G(0xffffffff) will need 582.55
>>>> hours. jiffies will large than 0x7ffffff. we guess jiffies =
>>>> 0x800000ff, time_after_eq(0x800000ff, 0xffffffff) will failure, and
>>>> one time which expire is 0xfffffff will be pended into system. So
>>>> the interface will hung until jiffies recount 0xffffffff (that will
>>>> need very long time).
>>> If I'm not mistaken you meant time_after_eq(now, next_credit) in
>>> netback. How does next_credit become 0xffffffff?
>> I only assume the value is 0xfffffff, and the value of next_credit
>> isn't  point. If the delta between now and next_credit larger than
>> ULONG_MAX, time_after_eq will do wrong judge.
> So it sounds like we need a timer which is independent of the traffic
> being sent to keep credit_timeout.expires rolling over.
>
> Can you propose a patch?

Because credit_timeout.expire always after jiffies, i judge the value 
over the range of time_after_eq() by time_before(now, 
vif->credit_timeout.expires). please check the patch.
>
> Ian.
>
>>> Wei.
>>>
>>>>     If some error exist in above explain, please help me point it out.
>>>>
>>>> Thanks,
>>>> Jason
>
From f08c584ca1f393f6559b58b6b4c9e259c313259e Mon Sep 17 00:00:00 2001
From: Jason Luan <jianhai.luan@oracle.com>
Date: Tue, 15 Oct 2013 17:07:49 +0800
Subject: [PATCH] Process the wrong judge of time_after_eq().

If netfront send lesser package, the delta between now and next_credit will be out range of time_after_qe() and the function will do wrong judge. Because the expires always after jiffies, we judge the condition by time_before(now, vif->credit_timeout.expires).

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

Comments

Wei Liu Oct. 15, 2013, 10:06 a.m. UTC | #1 
On Tue, Oct 15, 2013 at 05:34:57PM +0800, jianhai luan wrote:
> 
> On 2013-10-15 16:43, Ian Campbell wrote:
> >On Tue, 2013-10-15 at 10:44 +0800, jianhai luan wrote:
> >>On 2013-10-14 19:19, Wei Liu wrote:
> >>>On Sat, Oct 12, 2013 at 04:53:18PM +0800, jianhai luan wrote:
> >>>>Hi Ian,
> >>>>    I meet the DomU's network interface hung issue recently, and have
> >>>>been working on the issue from that time. I find that DomU's network
> >>>>interface, which send lesser package, will hung if Dom0 running
> >>>>32bit and DomU's up-time is very long.  I think that one jiffies
> >>>>overflow bug exist in the function tx_credit_exceeded().
> >>>>    I know the inline function time_after_eq(a,b) will process jiffies
> >>>>overflow, but the function have one limit a should little that (b +
> >>>>MAX_SIGNAL_LONG). If a large than the value, time_after_eq will
> >>>>return false. The MAX_SINGNAL_LONG should be 0x7fffffff at 32-bit
> >>>>machine.
> >>>>    If DomU's network interface send lesser package (<0.5k/s if
> >>>>jiffies=250 and credit_bytes=ULONG_MAX), jiffies will beyond out
> >>>>(credit_timeout.expires + MAX_SIGNAL_LONG) and time_after_eq(now,
> >>>>next_credit) will failure (should be true). So one timer which will
> >>>>not be trigger in short time, and later process will be aborted when
> >>>>timer_pending(&vif->credit_timeout) is true. The result will be
> >>>>DomU's network interface will be hung in long time (> 40days).
> >>>>    Please think about the below scenario:
> >>>>    Condition:
> >>>>      Dom0 running 32-bit and HZ = 1000
> >>>>      vif->credit_timeout->expire = 0xffffffff, vif->remaining_credit
> >>>>= 0xffffffff, vif->credit_usec=0 jiffies=0
> >>>>      vif receive lesser package (DomU send lesser package). If the
> >>>>value is litter than 2K/s, consume 4G(0xffffffff) will need 582.55
> >>>>hours. jiffies will large than 0x7ffffff. we guess jiffies =
> >>>>0x800000ff, time_after_eq(0x800000ff, 0xffffffff) will failure, and
> >>>>one time which expire is 0xfffffff will be pended into system. So
> >>>>the interface will hung until jiffies recount 0xffffffff (that will
> >>>>need very long time).
> >>>If I'm not mistaken you meant time_after_eq(now, next_credit) in
> >>>netback. How does next_credit become 0xffffffff?
> >>I only assume the value is 0xfffffff, and the value of next_credit
> >>isn't  point. If the delta between now and next_credit larger than
> >>ULONG_MAX, time_after_eq will do wrong judge.
> >So it sounds like we need a timer which is independent of the traffic
> >being sent to keep credit_timeout.expires rolling over.
> >
> >Can you propose a patch?
> 
> Because credit_timeout.expire always after jiffies, i judge the
> value over the range of time_after_eq() by time_before(now,
> vif->credit_timeout.expires). please check the patch.

I don't think this really fix the issue for you. You still have chance
that now wraps around and falls between expires and next_credit. In that
case it's stalled again.

Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jianhai luan Oct. 15, 2013, 11:26 a.m. UTC | #2 
On 2013-10-15 18:06, Wei Liu wrote:
> On Tue, Oct 15, 2013 at 05:34:57PM +0800, jianhai luan wrote:
>> On 2013-10-15 16:43, Ian Campbell wrote:
>>> On Tue, 2013-10-15 at 10:44 +0800, jianhai luan wrote:
>>>> On 2013-10-14 19:19, Wei Liu wrote:
>>>>> On Sat, Oct 12, 2013 at 04:53:18PM +0800, jianhai luan wrote:
>>>>>> Hi Ian,
>>>>>>     I meet the DomU's network interface hung issue recently, and have
>>>>>> been working on the issue from that time. I find that DomU's network
>>>>>> interface, which send lesser package, will hung if Dom0 running
>>>>>> 32bit and DomU's up-time is very long.  I think that one jiffies
>>>>>> overflow bug exist in the function tx_credit_exceeded().
>>>>>>     I know the inline function time_after_eq(a,b) will process jiffies
>>>>>> overflow, but the function have one limit a should little that (b +
>>>>>> MAX_SIGNAL_LONG). If a large than the value, time_after_eq will
>>>>>> return false. The MAX_SINGNAL_LONG should be 0x7fffffff at 32-bit
>>>>>> machine.
>>>>>>     If DomU's network interface send lesser package (<0.5k/s if
>>>>>> jiffies=250 and credit_bytes=ULONG_MAX), jiffies will beyond out
>>>>>> (credit_timeout.expires + MAX_SIGNAL_LONG) and time_after_eq(now,
>>>>>> next_credit) will failure (should be true). So one timer which will
>>>>>> not be trigger in short time, and later process will be aborted when
>>>>>> timer_pending(&vif->credit_timeout) is true. The result will be
>>>>>> DomU's network interface will be hung in long time (> 40days).
>>>>>>     Please think about the below scenario:
>>>>>>     Condition:
>>>>>>       Dom0 running 32-bit and HZ = 1000
>>>>>>       vif->credit_timeout->expire = 0xffffffff, vif->remaining_credit
>>>>>> = 0xffffffff, vif->credit_usec=0 jiffies=0
>>>>>>       vif receive lesser package (DomU send lesser package). If the
>>>>>> value is litter than 2K/s, consume 4G(0xffffffff) will need 582.55
>>>>>> hours. jiffies will large than 0x7ffffff. we guess jiffies =
>>>>>> 0x800000ff, time_after_eq(0x800000ff, 0xffffffff) will failure, and
>>>>>> one time which expire is 0xfffffff will be pended into system. So
>>>>>> the interface will hung until jiffies recount 0xffffffff (that will
>>>>>> need very long time).
>>>>> If I'm not mistaken you meant time_after_eq(now, next_credit) in
>>>>> netback. How does next_credit become 0xffffffff?
>>>> I only assume the value is 0xfffffff, and the value of next_credit
>>>> isn't  point. If the delta between now and next_credit larger than
>>>> ULONG_MAX, time_after_eq will do wrong judge.
>>> So it sounds like we need a timer which is independent of the traffic
>>> being sent to keep credit_timeout.expires rolling over.
>>>
>>> Can you propose a patch?
>> Because credit_timeout.expire always after jiffies, i judge the
>> value over the range of time_after_eq() by time_before(now,
>> vif->credit_timeout.expires). please check the patch.
> I don't think this really fix the issue for you. You still have chance
> that now wraps around and falls between expires and next_credit. In that
> case it's stalled again.

if time_before(now, vif->credit_timeout.expires) is true, time wrap and 
do operation. Otherwise time_before(now, vif->credit_timeout.expires) 
isn't true, now - vif->credit_timeout.expires should be letter than 
ULONG_MAX/2. Because next_credit large than vif->credit_timeout.expires 
(next_crdit = vif->credit_timeout.expires + 
msecs_to_jiffies(vif->credit_usec/1000)), the delta between now and 
next_credit should be in range of time_after_eq().  So time_after_eq() 
do correctly judge.

Jason
>
> Wei.

--
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
Wei Liu Oct. 15, 2013, 12:58 p.m. UTC | #3 
On Tue, Oct 15, 2013 at 07:26:31PM +0800, jianhai luan wrote:
[...]
> >>>Can you propose a patch?
> >>Because credit_timeout.expire always after jiffies, i judge the
> >>value over the range of time_after_eq() by time_before(now,
> >>vif->credit_timeout.expires). please check the patch.
> >I don't think this really fix the issue for you. You still have chance
> >that now wraps around and falls between expires and next_credit. In that
> >case it's stalled again.
> 
> if time_before(now, vif->credit_timeout.expires) is true, time wrap
> and do operation. Otherwise time_before(now,
> vif->credit_timeout.expires) isn't true, now -
> vif->credit_timeout.expires should be letter than ULONG_MAX/2.
> Because next_credit large than vif->credit_timeout.expires
> (next_crdit = vif->credit_timeout.expires +
> msecs_to_jiffies(vif->credit_usec/1000)), the delta between now and
> next_credit should be in range of time_after_eq().  So
> time_after_eq() do correctly judge.
> 

Not sure I understand you. Consider "now" is placed like this:

   expires   now   next_credit
   ----time increases this direction--->

* time_after_eq(now, next_credit) -> false
* time_before(now, expires) -> false

Then it's stuck again. You're merely narrowing the window, not fixing
the real problem.

Wei.

> Jason
> >
> >Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jianhai luan Oct. 15, 2013, 2:29 p.m. UTC | #4 
On 2013-10-15 20:58, Wei Liu wrote:
> On Tue, Oct 15, 2013 at 07:26:31PM +0800, jianhai luan wrote:
> [...]
>>>>> Can you propose a patch?
>>>> Because credit_timeout.expire always after jiffies, i judge the
>>>> value over the range of time_after_eq() by time_before(now,
>>>> vif->credit_timeout.expires). please check the patch.
>>> I don't think this really fix the issue for you. You still have chance
>>> that now wraps around and falls between expires and next_credit. In that
>>> case it's stalled again.
>> if time_before(now, vif->credit_timeout.expires) is true, time wrap
>> and do operation. Otherwise time_before(now,
>> vif->credit_timeout.expires) isn't true, now -
>> vif->credit_timeout.expires should be letter than ULONG_MAX/2.
>> Because next_credit large than vif->credit_timeout.expires
>> (next_crdit = vif->credit_timeout.expires +
>> msecs_to_jiffies(vif->credit_usec/1000)), the delta between now and
>> next_credit should be in range of time_after_eq().  So
>> time_after_eq() do correctly judge.
>>
> Not sure I understand you. Consider "now" is placed like this:
>
>     expires   now   next_credit
>     ----time increases this direction--->
>
> * time_after_eq(now, next_credit) -> false
> * time_before(now, expires) -> false

If now is placed in above environment, the result will be correct 
(Sending package will be not allowed until next_credit).
* time_after_eq(now, next_credit)  --> false will include two environment:
   expires     now   next_credit
   -----------time increases this direction ---->

Or
   expires      next_credit             next_credit + MAX_LONG/2 now
   -----------time increases this direction ---->


the first environment should be correct to control transmit. the second 
environment is our included environment.

Jason
>
> Then it's stuck again. You're merely narrowing the window, not fixing
> the real problem.
>
> Wei.
>
>> Jason
>>> Wei.

--
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
Wei Liu Oct. 15, 2013, 2:49 p.m. UTC | #5 
On Tue, Oct 15, 2013 at 10:29:15PM +0800, jianhai luan wrote:
> 
> On 2013-10-15 20:58, Wei Liu wrote:
> >On Tue, Oct 15, 2013 at 07:26:31PM +0800, jianhai luan wrote:
> >[...]
> >>>>>Can you propose a patch?
> >>>>Because credit_timeout.expire always after jiffies, i judge the
> >>>>value over the range of time_after_eq() by time_before(now,
> >>>>vif->credit_timeout.expires). please check the patch.
> >>>I don't think this really fix the issue for you. You still have chance
> >>>that now wraps around and falls between expires and next_credit. In that
> >>>case it's stalled again.
> >>if time_before(now, vif->credit_timeout.expires) is true, time wrap
> >>and do operation. Otherwise time_before(now,
> >>vif->credit_timeout.expires) isn't true, now -
> >>vif->credit_timeout.expires should be letter than ULONG_MAX/2.
> >>Because next_credit large than vif->credit_timeout.expires
> >>(next_crdit = vif->credit_timeout.expires +
> >>msecs_to_jiffies(vif->credit_usec/1000)), the delta between now and
> >>next_credit should be in range of time_after_eq().  So
> >>time_after_eq() do correctly judge.
> >>
> >Not sure I understand you. Consider "now" is placed like this:
> >
> >    expires   now   next_credit
> >    ----time increases this direction--->
> >
> >* time_after_eq(now, next_credit) -> false
> >* time_before(now, expires) -> false
> 
> If now is placed in above environment, the result will be correct
> (Sending package will be not allowed until next_credit).

No, it is not necessarily correct. Keep in mind that "now" wraps around,
which is the issue you try to fix. You still have a window to stall your
frontend.

> * time_after_eq(now, next_credit)  --> false will include two environment:
>   expires     now   next_credit
>   -----------time increases this direction ---->
> 
> Or
>   expires      next_credit             next_credit + MAX_LONG/2 now
>   -----------time increases this direction ---->
> 
> 
> the first environment should be correct to control transmit. the
> second environment is our included environment.
> 
> Jason
> >
> >Then it's stuck again. You're merely narrowing the window, not fixing
> >the real problem.
> >
> >Wei.
> >
> >>Jason
> >>>Wei.
--
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
Ian Campbell Oct. 15, 2013, 2:50 p.m. UTC | #6 
On Tue, 2013-10-15 at 15:49 +0100, Wei Liu wrote:
> On Tue, Oct 15, 2013 at 10:29:15PM +0800, jianhai luan wrote:
> > 
> > On 2013-10-15 20:58, Wei Liu wrote:
> > >On Tue, Oct 15, 2013 at 07:26:31PM +0800, jianhai luan wrote:
> > >[...]
> > >>>>>Can you propose a patch?
> > >>>>Because credit_timeout.expire always after jiffies, i judge the
> > >>>>value over the range of time_after_eq() by time_before(now,
> > >>>>vif->credit_timeout.expires). please check the patch.
> > >>>I don't think this really fix the issue for you. You still have chance
> > >>>that now wraps around and falls between expires and next_credit. In that
> > >>>case it's stalled again.
> > >>if time_before(now, vif->credit_timeout.expires) is true, time wrap
> > >>and do operation. Otherwise time_before(now,
> > >>vif->credit_timeout.expires) isn't true, now -
> > >>vif->credit_timeout.expires should be letter than ULONG_MAX/2.
> > >>Because next_credit large than vif->credit_timeout.expires
> > >>(next_crdit = vif->credit_timeout.expires +
> > >>msecs_to_jiffies(vif->credit_usec/1000)), the delta between now and
> > >>next_credit should be in range of time_after_eq().  So
> > >>time_after_eq() do correctly judge.
> > >>
> > >Not sure I understand you. Consider "now" is placed like this:
> > >
> > >    expires   now   next_credit
> > >    ----time increases this direction--->
> > >
> > >* time_after_eq(now, next_credit) -> false
> > >* time_before(now, expires) -> false
> > 
> > If now is placed in above environment, the result will be correct
> > (Sending package will be not allowed until next_credit).
> 
> No, it is not necessarily correct. Keep in mind that "now" wraps around,
> which is the issue you try to fix. You still have a window to stall your
> frontend.

Remember that time_after_eq is supposed to work even with wraparound
occurring, so long as the two times are less than MAX_LONG/2 apart.

> 
> > * time_after_eq(now, next_credit)  --> false will include two environment:
> >   expires     now   next_credit
> >   -----------time increases this direction ---->
> > 
> > Or
> >   expires      next_credit             next_credit + MAX_LONG/2 now
> >   -----------time increases this direction ---->
> > 
> > 
> > the first environment should be correct to control transmit. the
> > second environment is our included environment.
> > 
> > Jason
> > >
> > >Then it's stuck again. You're merely narrowing the window, not fixing
> > >the real problem.
> > >
> > >Wei.
> > >
> > >>Jason
> > >>>Wei.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jianhai luan Oct. 15, 2013, 3:19 p.m. UTC | #7 
On 2013-10-15 22:50, Ian Campbell wrote:
> On Tue, 2013-10-15 at 15:49 +0100, Wei Liu wrote:
>> On Tue, Oct 15, 2013 at 10:29:15PM +0800, jianhai luan wrote:
>>> On 2013-10-15 20:58, Wei Liu wrote:
>>>> On Tue, Oct 15, 2013 at 07:26:31PM +0800, jianhai luan wrote:
>>>> [...]
>>>>>>>> Can you propose a patch?
>>>>>>> Because credit_timeout.expire always after jiffies, i judge the
>>>>>>> value over the range of time_after_eq() by time_before(now,
>>>>>>> vif->credit_timeout.expires). please check the patch.
>>>>>> I don't think this really fix the issue for you. You still have chance
>>>>>> that now wraps around and falls between expires and next_credit. In that
>>>>>> case it's stalled again.
>>>>> if time_before(now, vif->credit_timeout.expires) is true, time wrap
>>>>> and do operation. Otherwise time_before(now,
>>>>> vif->credit_timeout.expires) isn't true, now -
>>>>> vif->credit_timeout.expires should be letter than ULONG_MAX/2.
>>>>> Because next_credit large than vif->credit_timeout.expires
>>>>> (next_crdit = vif->credit_timeout.expires +
>>>>> msecs_to_jiffies(vif->credit_usec/1000)), the delta between now and
>>>>> next_credit should be in range of time_after_eq().  So
>>>>> time_after_eq() do correctly judge.
>>>>>
>>>> Not sure I understand you. Consider "now" is placed like this:
>>>>
>>>>     expires   now   next_credit
>>>>     ----time increases this direction--->
>>>>
>>>> * time_after_eq(now, next_credit) -> false
>>>> * time_before(now, expires) -> false
>>> If now is placed in above environment, the result will be correct
>>> (Sending package will be not allowed until next_credit).
>> No, it is not necessarily correct. Keep in mind that "now" wraps around,
>> which is the issue you try to fix. You still have a window to stall your
>> frontend.
> Remember that time_after_eq is supposed to work even with wraparound
> occurring, so long as the two times are less than MAX_LONG/2 apart.

Sorry for my misunderstand explanation. I mean that
   * time_after_eq()/time_before_eq() fix the jiffies wraparound, so 
please think about  jiffies in line increasing.
   * time_after_eq()/time_before_eq() have the range (0, MAX_LONG/2), 
the judge will be wrong if out of the range.

So please think about three kind environment
   -  expires        now        next_credit
      --------time increases this direction ---------->

   -  expires        [next_credit        now next_credit+MAX_LONG/2
      --------time increase this direction ----------->

   - expires        next_credit        next_credit+MAX_LONG/2 now
      --------time increadse this direction ---------->

The first environment should be netfront consume all credit_byte before 
next_credit, So we should pending one timer to calculator the new 
credit_byte, and don't transmit until next_credit.

the second environment should be calculator the credit_byte because 
netfront don't consume all credit_byte before next_credit, and 
time_after_eq() do correct judge.

the third environment should be calculator in time because netfront 
don't consume all credit_byte until next_credit.But time_after_eq do 
error judge (time_after_eq(now, next_credit) is false), so the 
remaining_byte isn't be increased.

and I work on the third environment.  You know now > 
next_credit+MAX_LONG/2, time_before(now, expire) should be 
true(time_before(now, expire) is false in first environment)
>
>>> * time_after_eq(now, next_credit)  --> false will include two environment:
>>>    expires     now   next_credit
>>>    -----------time increases this direction ---->
>>>
>>> Or
>>>    expires      next_credit             next_credit + MAX_LONG/2 now
>>>    -----------time increases this direction ---->
>>>
>>>
>>> the first environment should be correct to control transmit. the
>>> second environment is our included environment.
>>>
>>> Jason
>>>> Then it's stuck again. You're merely narrowing the window, not fixing
>>>> the real problem.
>>>>
>>>> Wei.
>>>>
>>>>> Jason
>>>>>> Wei.
>

--
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
Wei Liu Oct. 15, 2013, 4:03 p.m. UTC | #8 
On Tue, Oct 15, 2013 at 11:19:42PM +0800, jianhai luan wrote:
[...]
> >>>>
> >>>>* time_after_eq(now, next_credit) -> false
> >>>>* time_before(now, expires) -> false
> >>>If now is placed in above environment, the result will be correct
> >>>(Sending package will be not allowed until next_credit).
> >>No, it is not necessarily correct. Keep in mind that "now" wraps around,
> >>which is the issue you try to fix. You still have a window to stall your
> >>frontend.
> >Remember that time_after_eq is supposed to work even with wraparound
> >occurring, so long as the two times are less than MAX_LONG/2 apart.
> 
> Sorry for my misunderstand explanation. I mean that
>   * time_after_eq()/time_before_eq() fix the jiffies wraparound, so
> please think about  jiffies in line increasing.
>   * time_after_eq()/time_before_eq() have the range (0, MAX_LONG/2),
> the judge will be wrong if out of the range.
> 
> So please think about three kind environment
>   -  expires        now        next_credit
>      --------time increases this direction ---------->
> 
>   -  expires        [next_credit        now next_credit+MAX_LONG/2
>      --------time increase this direction ----------->
> 
>   - expires        next_credit        next_credit+MAX_LONG/2 now
>      --------time increadse this direction ---------->
> 
> The first environment should be netfront consume all credit_byte
> before next_credit, So we should pending one timer to calculator the
> new credit_byte, and don't transmit until next_credit.
> 
> the second environment should be calculator the credit_byte because
> netfront don't consume all credit_byte before next_credit, and
> time_after_eq() do correct judge.
> 
> the third environment should be calculator in time because netfront
> don't consume all credit_byte until next_credit.But time_after_eq do
> error judge (time_after_eq(now, next_credit) is false), so the
> remaining_byte isn't be increased.
> 
> and I work on the third environment.  You know now >
> next_credit+MAX_LONG/2, time_before(now, expire) should be
> true(time_before(now, expire) is false in first environment)
> >

Thanks for staighten this out for me. I'm just too dumb for this, please
be patient with me. :-)

Could you prove that time_before(now, expire) is always true in third
case? That's where my main cencern lies. Is it because msecs_to_jiffies
always returns MAX_JIFFY_OFFSET (which is ((LONG_MAX >> 1)-1) ) at most?

Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jianhai luan Oct. 15, 2013, 4:23 p.m. UTC | #9 
On 2013-10-16 0:03, Wei Liu wrote:
> On Tue, Oct 15, 2013 at 11:19:42PM +0800, jianhai luan wrote:
> [...]
>>>>>> * time_after_eq(now, next_credit) -> false
>>>>>> * time_before(now, expires) -> false
>>>>> If now is placed in above environment, the result will be correct
>>>>> (Sending package will be not allowed until next_credit).
>>>> No, it is not necessarily correct. Keep in mind that "now" wraps around,
>>>> which is the issue you try to fix. You still have a window to stall your
>>>> frontend.
>>> Remember that time_after_eq is supposed to work even with wraparound
>>> occurring, so long as the two times are less than MAX_LONG/2 apart.
>> Sorry for my misunderstand explanation. I mean that
>>    * time_after_eq()/time_before_eq() fix the jiffies wraparound, so
>> please think about  jiffies in line increasing.
>>    * time_after_eq()/time_before_eq() have the range (0, MAX_LONG/2),
>> the judge will be wrong if out of the range.
>>
>> So please think about three kind environment
>>    -  expires        now        next_credit
>>       --------time increases this direction ---------->
>>
>>    -  expires        [next_credit        now next_credit+MAX_LONG/2
>>       --------time increase this direction ----------->
>>
>>    - expires        next_credit        next_credit+MAX_LONG/2 now
>>       --------time increadse this direction ---------->
>>
>> The first environment should be netfront consume all credit_byte
>> before next_credit, So we should pending one timer to calculator the
>> new credit_byte, and don't transmit until next_credit.
>>
>> the second environment should be calculator the credit_byte because
>> netfront don't consume all credit_byte before next_credit, and
>> time_after_eq() do correct judge.
>>
>> the third environment should be calculator in time because netfront
>> don't consume all credit_byte until next_credit.But time_after_eq do
>> error judge (time_after_eq(now, next_credit) is false), so the
>> remaining_byte isn't be increased.
>>
>> and I work on the third environment.  You know now >
>> next_credit+MAX_LONG/2, time_before(now, expire) should be
>> true(time_before(now, expire) is false in first environment)
> Thanks for staighten this out for me. I'm just too dumb for this, please
> be patient with me. :-)
>
> Could you prove that time_before(now, expire) is always true in third
> case? That's where my main cencern lies. Is it because msecs_to_jiffies
> always returns MAX_JIFFY_OFFSET (which is ((LONG_MAX >> 1)-1) ) at most?

I have wrong judge in third environment. If now large than expires + 
MAX_UNLONG, time_before(now, expires) will be false.
   expires    next_credit    next_credit+MAX_UNLONG/2    expires + 
MAX_UNLONG    now    next_credit+MAX_UNLONG
   --------------------------------------------------------- time 
increadse this direction  ---------------------------------->

   In the above environment, time_before(now, expires) will return 
false. But the jiffies elapsed more time and next_credit will be 
reachable in soon(time_after_eq(now, next_credit) will be true).
>
> Wei.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jianhai luan Oct. 16, 2013, 12:15 a.m. UTC | #10 
On 2013-10-16 0:23, jianhai luan wrote:
>
> On 2013-10-16 0:03, Wei Liu wrote:
>> On Tue, Oct 15, 2013 at 11:19:42PM +0800, jianhai luan wrote:
>> [...]
>>>>>>> * time_after_eq(now, next_credit) -> false
>>>>>>> * time_before(now, expires) -> false
>>>>>> If now is placed in above environment, the result will be correct
>>>>>> (Sending package will be not allowed until next_credit).
>>>>> No, it is not necessarily correct. Keep in mind that "now" wraps 
>>>>> around,
>>>>> which is the issue you try to fix. You still have a window to 
>>>>> stall your
>>>>> frontend.
>>>> Remember that time_after_eq is supposed to work even with wraparound
>>>> occurring, so long as the two times are less than MAX_LONG/2 apart.
>>> Sorry for my misunderstand explanation. I mean that
>>>    * time_after_eq()/time_before_eq() fix the jiffies wraparound, so
>>> please think about  jiffies in line increasing.
>>>    * time_after_eq()/time_before_eq() have the range (0, MAX_LONG/2),
>>> the judge will be wrong if out of the range.
>>>
>>> So please think about three kind environment
>>>    -  expires        now        next_credit
>>>       --------time increases this direction ---------->
>>>
>>>    -  expires        [next_credit        now next_credit+MAX_LONG/2
>>>       --------time increase this direction ----------->
>>>
>>>    - expires        next_credit        next_credit+MAX_LONG/2 now
>>>       --------time increadse this direction ---------->
>>>
>>> The first environment should be netfront consume all credit_byte
>>> before next_credit, So we should pending one timer to calculator the
>>> new credit_byte, and don't transmit until next_credit.
>>>
>>> the second environment should be calculator the credit_byte because
>>> netfront don't consume all credit_byte before next_credit, and
>>> time_after_eq() do correct judge.
>>>
>>> the third environment should be calculator in time because netfront
>>> don't consume all credit_byte until next_credit.But time_after_eq do
>>> error judge (time_after_eq(now, next_credit) is false), so the
>>> remaining_byte isn't be increased.
>>>
>>> and I work on the third environment.  You know now >
>>> next_credit+MAX_LONG/2, time_before(now, expire) should be
>>> true(time_before(now, expire) is false in first environment)
>> Thanks for staighten this out for me. I'm just too dumb for this, please
>> be patient with me. :-)
>>
>> Could you prove that time_before(now, expire) is always true in third
>> case? That's where my main cencern lies. Is it because msecs_to_jiffies
>> always returns MAX_JIFFY_OFFSET (which is ((LONG_MAX >> 1)-1) ) at most?
>
> I have wrong judge in third environment. If now large than expires + 
> MAX_UNLONG, time_before(now, expires) will be false.
>   expires    next_credit    next_credit+MAX_UNLONG/2    expires + 
> MAX_UNLONG    now    next_credit+MAX_UNLONG
>   --------------------------------------------------------- time 
> increadse this direction  ---------------------------------->
>
>   In the above environment, time_before(now, expires) will return 
> false. But the jiffies elapsed more time and next_credit will be 
> reachable in soon(time_after_eq(now, next_credit) will be true).

After above talk, the window should be exist (expire+MAX_ULONG 
next_credit+MAX_ULONG, expire + 2MAX_ULONF next_credit+MAX_ULONG 
....,expire+<n>ULONG next_credit+<n>MAX_ULONG). Other time window should 
be not exist (maybe i don't think about).
  If so, please think about the below:
   * If no speed control, vif->credit_usec should be zero. expire = 
next_credit and the window is zero
   * If we have done speed control, the scenario should be very likely 
than first environment except the value is larger (the delta is 
<n>MAX_UNLONG)
      - If do speed control. the window should have been think about
                      speed            worse time
                      100M/s          40s
                      1000M/s       4s
      - the time should be acceptable.
>>
>> Wei.
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jianhai luan Oct. 16, 2013, 7:35 a.m. UTC | #11 
On 2013-10-15 20:58, Wei Liu wrote:
> On Tue, Oct 15, 2013 at 07:26:31PM +0800, jianhai luan wrote:
> [...]
>>>>> Can you propose a patch?
>>>> Because credit_timeout.expire always after jiffies, i judge the
>>>> value over the range of time_after_eq() by time_before(now,
>>>> vif->credit_timeout.expires). please check the patch.
>>> I don't think this really fix the issue for you. You still have chance
>>> that now wraps around and falls between expires and next_credit. In that
>>> case it's stalled again.
>> if time_before(now, vif->credit_timeout.expires) is true, time wrap
>> and do operation. Otherwise time_before(now,
>> vif->credit_timeout.expires) isn't true, now -
>> vif->credit_timeout.expires should be letter than ULONG_MAX/2.
>> Because next_credit large than vif->credit_timeout.expires
>> (next_crdit = vif->credit_timeout.expires +
>> msecs_to_jiffies(vif->credit_usec/1000)), the delta between now and
>> next_credit should be in range of time_after_eq().  So
>> time_after_eq() do correctly judge.
>>
> Not sure I understand you. Consider "now" is placed like this:
>
>     expires   now   next_credit
>     ----time increases this direction--->
>
> * time_after_eq(now, next_credit) -> false
> * time_before(now, expires) -> false
>
> Then it's stuck again. You're merely narrowing the window, not fixing
> the real problem.

The above environment isn't stack again. The netback will pending one 
timer to process the environment.

The attachment program will prove if !(time_after_eq(now, next_credit) 
|| time_before(now, vif->credit_timeout.expires)), now will only be 
placed in  above environment [ expires next_credit),  and the above 
environment will be processed by timer in soon.
>
> Wei.
>
>> Jason
>>> Wei.
Jason.
#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;
}
Annie.li Oct. 16, 2013, 9:39 a.m. UTC | #12 
On 2013-10-16 15:35, jianhai luan wrote:
>
> On 2013-10-15 20:58, Wei Liu wrote:
>> On Tue, Oct 15, 2013 at 07:26:31PM +0800, jianhai luan wrote:
>> [...]
>>>>>> Can you propose a patch?
>>>>> Because credit_timeout.expire always after jiffies, i judge the
>>>>> value over the range of time_after_eq() by time_before(now,
>>>>> vif->credit_timeout.expires). please check the patch.
>>>> I don't think this really fix the issue for you. You still have chance
>>>> that now wraps around and falls between expires and next_credit. In 
>>>> that
>>>> case it's stalled again.
>>> if time_before(now, vif->credit_timeout.expires) is true, time wrap
>>> and do operation. Otherwise time_before(now,
>>> vif->credit_timeout.expires) isn't true, now -
>>> vif->credit_timeout.expires should be letter than ULONG_MAX/2.
>>> Because next_credit large than vif->credit_timeout.expires
>>> (next_crdit = vif->credit_timeout.expires +
>>> msecs_to_jiffies(vif->credit_usec/1000)), the delta between now and
>>> next_credit should be in range of time_after_eq().  So
>>> time_after_eq() do correctly judge.
>>>
>> Not sure I understand you. Consider "now" is placed like this:
>>
>>     expires   now   next_credit
>>     ----time increases this direction--->
>>
>> * time_after_eq(now, next_credit) -> false
>> * time_before(now, expires) -> false
>>
>> Then it's stuck again. You're merely narrowing the window, not fixing
>> the real problem.
>
> The above environment isn't stack again. The netback will pending one 
> timer to process the environment.
>
> The attachment program will prove if !(time_after_eq(now, next_credit) 
> || time_before(now, vif->credit_timeout.expires)), now will only be 
> placed in  above environment [ expires next_credit),  and the above 
> environment will be processed by timer in soon.

Or check following to see what the if condition really do,

----------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
>>
>> Wei.
>>
>>> Jason
>>>> Wei.
> Jason.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jianhai luan Oct. 16, 2013, 1:08 p.m. UTC | #13 
On 2013-10-16 17:39, annie li wrote:
>
> On 2013-10-16 15:35, jianhai luan wrote:
>>
>> On 2013-10-15 20:58, Wei Liu wrote:
>>> On Tue, Oct 15, 2013 at 07:26:31PM +0800, jianhai luan wrote:
>>> [...]
>>>>>>> Can you propose a patch?
>>>>>> Because credit_timeout.expire always after jiffies, i judge the
>>>>>> value over the range of time_after_eq() by time_before(now,
>>>>>> vif->credit_timeout.expires). please check the patch.
>>>>> I don't think this really fix the issue for you. You still have 
>>>>> chance
>>>>> that now wraps around and falls between expires and next_credit. 
>>>>> In that
>>>>> case it's stalled again.
>>>> if time_before(now, vif->credit_timeout.expires) is true, time wrap
>>>> and do operation. Otherwise time_before(now,
>>>> vif->credit_timeout.expires) isn't true, now -
>>>> vif->credit_timeout.expires should be letter than ULONG_MAX/2.
>>>> Because next_credit large than vif->credit_timeout.expires
>>>> (next_crdit = vif->credit_timeout.expires +
>>>> msecs_to_jiffies(vif->credit_usec/1000)), the delta between now and
>>>> next_credit should be in range of time_after_eq().  So
>>>> time_after_eq() do correctly judge.
>>>>
>>> Not sure I understand you. Consider "now" is placed like this:
>>>
>>>     expires   now   next_credit
>>>     ----time increases this direction--->
>>>
>>> * time_after_eq(now, next_credit) -> false
>>> * time_before(now, expires) -> false
>>>
>>> Then it's stuck again. You're merely narrowing the window, not fixing
>>> the real problem.
>>
>> The above environment isn't stack again. The netback will pending one 
>> timer to process the environment.
>>
>> The attachment program will prove if !(time_after_eq(now, 
>> next_credit) || time_before(now, vif->credit_timeout.expires)), now 
>> will only be placed in above environment [ expires next_credit),  and 
>> the above environment will be processed by timer in soon.
>
> Or check following to see what the if condition really do,
>
> ----------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's express, my option is consistent with Annie.

Jason
>
> Thanks
> Annie
>>>
>>> Wei.
>>>
>>>> Jason
>>>>> Wei.
>> Jason.
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>

--
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
Wei Liu Oct. 16, 2013, 1:47 p.m. UTC | #14 
On Wed, Oct 16, 2013 at 09:08:03PM +0800, jianhai luan wrote:
[...]
> >>>
> >>>    expires   now   next_credit
> >>>    ----time increases this direction--->
> >>>
> >>>* time_after_eq(now, next_credit) -> false
> >>>* time_before(now, expires) -> false
> >>>
> >>>Then it's stuck again. You're merely narrowing the window, not fixing
> >>>the real problem.
> >>
> >>The above environment isn't stack again. The netback will
> >>pending one timer to process the environment.
> >>
> >>The attachment program will prove if !(time_after_eq(now,
> >>next_credit) || time_before(now, vif->credit_timeout.expires)),
> >>now will only be placed in above environment [ expires
> >>next_credit),  and the above environment will be processed by
> >>timer in soon.
> >
> >Or check following to see what the if condition really do,
> >
> >----------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's express, my option is consistent with Annie.
> 

OK, thanks for the explanation. I think I get the idea.

In any case, could you use !time_in_range / !time_in_range_open instead
of open coded one-liner? Though I presume one-liner open coding would
not hurt.

Wei.
--
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
diff mbox

Patch

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index f3e591c..8036ce6 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1195,7 +1195,8 @@  static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
 		return true;
 
 	/* Passed the point where we can replenish credit? */
-	if (time_after_eq(now, next_credit)) {
+	if (time_after_eq(now, next_credit) ||
+		unlikely(time_before(now, vif->credit_timeout.expires))) {
 		vif->credit_timeout.expires = now;
 		tx_add_credit(vif);
 	}