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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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