Message ID | 525D0C41.2080407@oracle.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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
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
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
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
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
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
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
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
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
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
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; }
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
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
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 --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); }