Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753392AbaATNCL (ORCPT ); Mon, 20 Jan 2014 08:02:11 -0500 Received: from smtp02.citrix.com ([66.165.176.63]:44996 "EHLO SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751917AbaATNCH convert rfc822-to-8bit (ORCPT ); Mon, 20 Jan 2014 08:02:07 -0500 X-IronPort-AV: E=Sophos;i="4.95,690,1384300800"; d="scan'208";a="92387188" From: Paul Durrant To: Zoltan Kiss , Ian Campbell , Wei Liu , "xen-devel@lists.xenproject.org" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "Jonathan Davies" Subject: RE: [PATCH net-next v2] xen-netback: Rework rx_work_todo Thread-Topic: [PATCH net-next v2] xen-netback: Rework rx_work_todo Thread-Index: AQHPEhT6QaxFYMtfNkKJhENPI4F3rpqNf+oAgAAbJBA= Date: Mon, 20 Jan 2014 13:02:04 +0000 Message-ID: <9AAE0902D5BC7E449B7C8E4E778ABCD020BC91@AMSPEX01CL01.citrite.net> References: <1389805867-22409-1-git-send-email-zoltan.kiss@citrix.com> <52DD1540.7000503@citrix.com> In-Reply-To: <52DD1540.7000503@citrix.com> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.80.2.29] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-DLP: MIA2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > -----Original Message----- > From: Zoltan Kiss > Sent: 20 January 2014 12:23 > To: Ian Campbell; Wei Liu; xen-devel@lists.xenproject.org; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Jonathan Davies > Cc: Paul Durrant > Subject: Re: [PATCH net-next v2] xen-netback: Rework rx_work_todo > > Any reviews on this one? It fixes an important lockup situation, so > either this or some other fix should go in soon. > > On 15/01/14 17:11, Zoltan Kiss wrote: > > The recent patch to fix receive side flow control (11b57f) solved the > spinning > > thread problem, however caused an another one. The receive side can > stall, if: > > - [THREAD] xenvif_rx_action sets rx_queue_stopped to true > > - [INTERRUPT] interrupt happens, and sets rx_event to true > > - [THREAD] then xenvif_kthread sets rx_event to false > > - [THREAD] rx_work_todo doesn't return true anymore > > > > Also, if interrupt sent but there is still no room in the ring, it take quite a > > long time until xenvif_rx_action realize it. This patch ditch that two variable, > > and rework rx_work_todo. If the thread finds it can't fit more skb's into the > > ring, it saves the last slot estimation into rx_last_skb_slots, otherwise it's > > kept as 0. Then rx_work_todo will check if: > > - there is something to send to the ring (like before) > > - there is space for the topmost packet in the queue > > > > I think that's more natural and optimal thing to test than two bool which are > > set somewhere else. > > > > Signed-off-by: Zoltan Kiss > > --- > > drivers/net/xen-netback/common.h | 6 +----- > > drivers/net/xen-netback/interface.c | 1 - > > drivers/net/xen-netback/netback.c | 16 ++++++---------- > > 3 files changed, 7 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen- > netback/common.h > > index 4c76bcb..ae413a2 100644 > > --- a/drivers/net/xen-netback/common.h > > +++ b/drivers/net/xen-netback/common.h > > @@ -143,11 +143,7 @@ struct xenvif { > > char rx_irq_name[IFNAMSIZ+4]; /* DEVNAME-rx */ > > struct xen_netif_rx_back_ring rx; > > struct sk_buff_head rx_queue; > > - bool rx_queue_stopped; > > - /* Set when the RX interrupt is triggered by the frontend. > > - * The worker thread may need to wake the queue. > > - */ > > - bool rx_event; > > + RING_IDX rx_last_skb_slots; > > > > /* This array is allocated seperately as it is large */ > > struct gnttab_copy *grant_copy_op; > > diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen- > netback/interface.c > > index b9de31e..7669d49 100644 > > --- a/drivers/net/xen-netback/interface.c > > +++ b/drivers/net/xen-netback/interface.c > > @@ -100,7 +100,6 @@ static irqreturn_t xenvif_rx_interrupt(int irq, void > *dev_id) > > { > > struct xenvif *vif = dev_id; > > > > - vif->rx_event = true; > > xenvif_kick_thread(vif); > > > > return IRQ_HANDLED; > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen- > netback/netback.c > > index 2738563..bb241d0 100644 > > --- a/drivers/net/xen-netback/netback.c > > +++ b/drivers/net/xen-netback/netback.c > > @@ -477,7 +477,6 @@ static void xenvif_rx_action(struct xenvif *vif) > > unsigned long offset; > > struct skb_cb_overlay *sco; > > bool need_to_notify = false; > > - bool ring_full = false; > > > > struct netrx_pending_operations npo = { > > .copy = vif->grant_copy_op, > > @@ -487,7 +486,7 @@ static void xenvif_rx_action(struct xenvif *vif) > > skb_queue_head_init(&rxq); > > > > while ((skb = skb_dequeue(&vif->rx_queue)) != NULL) { > > - int max_slots_needed; > > + RING_IDX max_slots_needed; > > int i; > > > > /* We need a cheap worse case estimate for the number of > > @@ -510,9 +509,10 @@ static void xenvif_rx_action(struct xenvif *vif) > > if (!xenvif_rx_ring_slots_available(vif, max_slots_needed)) { > > skb_queue_head(&vif->rx_queue, skb); > > need_to_notify = true; > > - ring_full = true; > > + vif->rx_last_skb_slots = max_slots_needed; > > break; > > - } > > + } else > > + vif->rx_last_skb_slots = 0; > > > > sco = (struct skb_cb_overlay *)skb->cb; > > sco->meta_slots_used = xenvif_gop_skb(skb, &npo); > > @@ -523,8 +523,6 @@ static void xenvif_rx_action(struct xenvif *vif) > > > > BUG_ON(npo.meta_prod > ARRAY_SIZE(vif->meta)); > > > > - vif->rx_queue_stopped = !npo.copy_prod && ring_full; > > - > > if (!npo.copy_prod) > > goto done; > > > > @@ -1727,8 +1725,8 @@ static struct xen_netif_rx_response > *make_rx_response(struct xenvif *vif, > > > > static inline int rx_work_todo(struct xenvif *vif) > > { > > - return (!skb_queue_empty(&vif->rx_queue) && !vif- > >rx_queue_stopped) || > > - vif->rx_event; > > + return !skb_queue_empty(&vif->rx_queue) && > > + xenvif_rx_ring_slots_available(vif, vif->rx_last_skb_slots); > > } > > > > static inline int tx_work_todo(struct xenvif *vif) > > @@ -1814,8 +1812,6 @@ int xenvif_kthread(void *data) > > if (!skb_queue_empty(&vif->rx_queue)) > > xenvif_rx_action(vif); > > > > - vif->rx_event = false; > > - The minimal patch is to simply move this line up above the previous if clause, but I'm happy with your patch as it stands so Reviewed-by: Paul Durrant > > if (skb_queue_empty(&vif->rx_queue) && > > netif_queue_stopped(vif->dev)) > > xenvif_start_queue(vif); > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/