[Xen-devel] Reï [PATCH] fix netfront alloc_page error handling bug, need to raise up rx_refill timer if rx request buffer not big enough for backend

[Ma JieYue <majieyue@xxxxxxxxx>]

> First thing, for a network driver related patch you should send to
> netdev@ list as well. As a bug fix you would need to prefix it with
> [PATCH net], targeting net tree.

> A good reading on how netdev@ works can be found in kernel's
> documentation directory, called netdev-FAQ.txt.


Do you mean I need to send to netdev list, at the same time cc xen-devel?


> Second, the analysis is good but your commit message is not formal.
> Could you please have a look at SubmittingPatches.txt on how commit
> message should be written.


I will fix the commit message issue and re-submit the patch, thanks Wei


> I just look at the code snippet, would it be simpler just to move the
> mod_timer before "goto refill" in that part?


Well, if that so, each alloc_page failure will raise up the rx_refill
timer. It may be a little aggressive, but still acceptable. The fix
looks more neat.


> This is the guest receive path, why does NETIF_F_TSO matter?


You are right, I'll fix it



On Thu, Nov 14, 2013 at 12:15:48AM +0800, Ma JieYue wrote:
> Hi,
>
> We found a netfront driver bug when some user downloaded a large 8G file 
> using wget, in a centOS guest on Xen. The virtual machine has only 512MB 
> memory and the system had high IO stress when wget was running. After some 
> time, wget hung and netfront could not receive any packet from netback at 
> all. We also observed netback vif device was dropping packets at that time.
>
> By debugging we found lots of alloc_page error in netfront driver occured in 
> function xennet_alloc_rx_buffers, after wget was running for a while. The 
> alloc_page error won't stop until the wget hung forever. After that, the rx 
> IO ring paramters from frontend and backend driver, indicated that netfront 
> did not provide enough rx request buffer to netback, so that netback driver 
> thought the ring is full and dropped packets.
>
> the rx IO rings looks as below, e.g.
>
> [netback]
> rsp_prod_pvt 666318, req_cons 666318, nr_ents 256
> sring req_prod 666337, req_event 666338, rsp_prod 666318, rsp_event 666319
>
> [netfront]
> rsp_cons 666318, req_prod_pvt 666337
> sring req_prod 666337, req_event 666338, rsp_prod 666318, rsp_event 666319
>
> So, the rx request buffer only has 19 requests for backend, while backend at 
> least needs 20 ( MAX_SKB_FRAGS+2 ) requests buffer in order to send a single 
> skb to netfront when SG and TSO enabled.
>

FWIW newer kernel has MAX_SKB_FRAGS = 17, not 18.

> Also we gathered some info about alloc_page failure, for the last 6 times 
> before netfront/netback stopped communicating. From the last alloc_page 
> failure, we found the rx request ring buffer had only 18 requests buffer 
> left, and it started to allocate 46 pages to refill the buffer. 
> Unfortunately, the driver failed to allocate the second page, so that after 
> this round it only refilled one page and the rx ring request buffer only had 
> 19 request buffers left.
>

I just look at the code snippet, would it be simpler just to move the
mod_timer before "goto refill" in that part?

> the debug log looks as below, e.g.
>
> alloc_rx_buffers, alloc_page failed at 2 page, try to alloc 26 pages, 
> rx_target 64, req_prod 552445, rsp_cons 552407
> alloc_rx_buffers, alloc_page failed at 0 page, try to alloc 36 pages, 
> rx_target 64, req_prod 552447, rsp_cons 552419
> alloc_rx_buffers, alloc_page failed at 0 page, try to alloc 46 pages, 
> rx_target 64, req_prod 552447, rsp_cons 552429
> alloc_rx_buffers, alloc_page failed at 0 page, try to alloc 46 pages, 
> rx_target 64, req_prod 552447, rsp_cons 552429
> alloc_rx_buffers, alloc_page failed at 4 page, try to alloc 28 pages, 
> rx_target 64, req_prod 639240, rsp_cons 639204
> alloc_rx_buffers, alloc_page failed at 1 page, try to alloc 46 pages, 
> rx_target 64, req_prod 639244, rsp_cons 639226
>
> The problem is that, if the alloc page failure not occured at the first page, 
> the rx_refill_timer won't be set, which will help extend the rx request 
> buffer later. So, our fix is to set the rx_refill_timer, if the rx request 
> buffer may be not enough. The patch works well during our stress test.
>
>
> Signed-off-by: Ma JieYue <jieyue.majy@xxxxxxxxxxxxxx>
> ---
> drivers/net/xen-netfront.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index 1db10141..680e959 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -243,10 +243,16 @@ static void xennet_alloc_rx_buffers(struct net_device 
> *dev)
> unsigned long pfn;
> void *vaddr;
> struct xen_netif_rx_request *req;
> + int needed;
>
> if (unlikely(!netif_carrier_ok(dev)))
> return;
>
> + if (dev->features & (NETIF_F_SG|NETIF_F_TSO))

This is the guest receive path, why does NETIF_F_TSO matter?

Wei.

> + needed = MAX_SKB_FRAGS + 2;
> + else
> + needed = 1;
> +
> /*
> * Allocate skbuffs greedily, even though we batch updates to the
> * receive ring. This creates a less bursty demand on the memory
> @@ -327,6 +333,9 @@ no_skb:
>
> /* Above is a suitable barrier to ensure backend will see requests. */
> np->rx.req_prod_pvt = req_prod + i;
> + if (i && (np->rx.req_prod_pvt - np->rx.sring->rsp_prod < needed))
> + mod_timer(&np->rx_refill_timer, jiffies + (HZ/10));
> +
> push:
> RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&np->rx, notify);
> if (notify)
> --
> 1.8.4
>
>
> _______________________________________________
> 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