Re: [Xen-devel] [patch] netback: Xennet half die---netback driver didn't detect the jiffies wrapping correctly.

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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.

Jan

> I'm also surprised that block doesn't need further adjustment since it
> uses netif->credit_timeout.expires as its input and so this change will
> change its behaviour.
> 
> Perhaps this new addition needs to be in an else block after if
> (txreq.size > netif->remaining_credit) ?
> 
> Why is there no need to call tx_add_credit in this case? What about
> needing to check for a pending timer?
> 
>> +
>>              /* Credit-based scheduling. */
>>              if (txreq.size > netif->remaining_credit) {
>>                      unsigned long now = jiffies;
>> 
>> 
>> 
>> 
>> 
>>  Protected by Websense Hosted Email Security -- www.websense.com 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 




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