[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [patch] netback: Xennet half die---netback driver didn't detect the jiffies wrapping correctly.
On Fri, 2012-11-30 at 10:02 +0000, Jan Beulich wrote: > >>> On 30.11.12 at 10:44, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote: > > On Fri, 2012-11-30 at 09:25 +0000, Jan Beulich wrote: > >> >>> On 30.11.12 at 09:35, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote: > >> > On Fri, 2012-11-30 at 08:09 +0000, Yi, Shunli wrote: > >> >> Netback driver use " time_after_eq()" to check the jiffies wrapping, > >> >> while this function was only called when the credit is running out. > >> >> So, if the jiffies wrapped and the credit isn't run out in first half > >> >> jiffies circle, the time_after_eq() cannot check the wrapping any > >> >> more. > >> > > >> > Which tree is this against? It doesn't appear to be mainline Linux, > >> > which is all I am really interested in these days. > >> > > >> > Also your patch is missing a Signed-off-by and is whitespace damaged. > >> > Please read Documentation/SubmittingPatches and > >> > Documentation/SubmitChecklist. > >> > > >> >> This will cause the credit_timerout.expires is set to dozens of days > >> >> in future. > >> >> > >> >> The netback will stop receiving data from netfront. > >> >> > >> >> For example: > >> >> Jiffies initialized to 0xffffff-(300*HZ), and the > >> >> credit_timeout.expires was initialized to 0xffffff00, > >> >> After dozens of days, when the jiffies grow to upper than 0x80000000, > >> >> and the time_after_eq() will cannot check for the wrapping. > >> >> > >> >> > >> >> > >> >> --- drivers/xen/netback/netback.c.org 2012-11-30 15:48:13.109039998 > >> >> -0500 > >> >> +++ drivers/xen/netback/netback.c 2012-11-30 15:48:55.212072898 > >> >> -0500 > >> >> @@ -1272,6 +1272,10 @@ static void net_tx_action(unsigned long > >> >> rmb(); /* Ensure that we see the request before we copy > >> >> it. */ > >> >> memcpy(&txreq, RING_GET_REQUEST(&netif->tx, i), > >> >> sizeof(txreq)); > >> >> > >> >> + /* Check for the jiffies wrapping */ > >> >> + if (time_after_eq(jiffies, netif->credit_timeout.expires)) > >> >> + netif->credit_timeout.expires = jiffies; > >> > > >> > Do you not need to remove the similar check from the following block? > >> > >> I don't think so, but I also can't see how that adjustment would > >> help in the first place: If it gets executed after a very long period > >> of no traffic, it would itself not be able to reliably tell whether the > >> clock wrapped. > > > > Hrm, yes, This change would help in the case of a dribble of traffic > > which never hits the limit, but not in the case of no timer at all. > > > >> That said, I agree that the code as is appears to have a problem > >> (with 32-bit jiffies at least), but I can't see how to easily deal with > >> it. > > > > Would it help to always have the pending timer armed, for either the > > next tick if credit needs replenishing or for, say MAX_JIFFIES/4 as a > > backstop to avoid wrapping issues? > > If that can be made work cleanly, that would probably be the > easiest solution. But I don't see MAX_JIFFIES being defined > anywhere, and I'm unsure ULONG_MAX/4 would be well received > as a timeout on 64-bit systems. I guess several trillion years *is* a tad extreme ;-) (or my maths is wrong). We could always choose a more practical backstop, like a day or even a just few minutes -- we'd expect in general to always be pushing the timeout ahead rather than hitting it. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |