netdev - Re: [PATCH net V2] xen-netback: don't move event pointer in TX credit timeout callback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140515153051.GJ1117@zion.uk.xensource.com>
Date:	Thu, 15 May 2014 16:30:52 +0100
From:	Wei Liu <wei.liu2@...rix.com>
To:	Zoltan Kiss <zoltan.kiss@...rix.com>
CC:	Wei Liu <wei.liu2@...rix.com>, Jacek Konieczny <jajcus@...cus.net>,
	<xen-devel@...ts.xen.org>, <netdev@...r.kernel.org>,
	Paul Durrant <paul.durrant@...rix.com>
Subject: Re: [PATCH net V2] xen-netback: don't move event pointer in TX
 credit timeout callback

On Thu, May 15, 2014 at 03:47:38PM +0100, Zoltan Kiss wrote:
> On 15/05/14 15:13, Wei Liu wrote:
> >On Thu, May 15, 2014 at 03:04:36PM +0200, Jacek Konieczny wrote:
> >>On 05/15/14 13:59, Wei Liu wrote:
> >>>... otherwise the frontend will try to send TX event all the time, even
> >>>if no progress can be made. The pointer should only be advanced by the
> >>>routine that actually processes the ring (that is, xenvif_poll).
> >>>
> >>>Reported-by: Jacek Konieczny <jajcus@...cus.net>
> >>>Signed-off-by: Wei Liu <wei.liu2@...rix.com>
> >>>Acked-by: Ian Campbell <ian.campbell@...rix.com>
> >>>Cc: Paul Durrant <paul.durrant@...rix.com>
> >>>---
> >>>  drivers/net/xen-netback/netback.c |    2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>>diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> >>>index 7666540..8e2cbeb 100644
> >>>--- a/drivers/net/xen-netback/netback.c
> >>>+++ b/drivers/net/xen-netback/netback.c
> >>>@@ -658,7 +658,7 @@ void xenvif_check_rx_xenvif(struct xenvif *vif)
> >>>  {
> >>>  	int more_to_do;
> >>>
> >>>-	RING_FINAL_CHECK_FOR_REQUESTS(&vif->tx, more_to_do);
> >>>+	more_to_do = RING_HAS_UNCONSUMED_REQUESTS(&vif->tx);
> >>>
> >>
> >>Unfortunately, this seems not enough to fix the problem I have reported
> >>here:
> >>http://lists.xenproject.org/archives/html/xen-devel/2014-05/msg01183.html
> >>
> >>The dom0 network still stalls when using rate limiting on a VIF
> >>interface after applying this patch to my 3.14.3 kernel (100% CPU#1
> >>usage in the 'soft interrupts').
> >>
> >
> >This is a patch for 3.14.4. I've tested it myself (and looking at the
> >right stats!) to confirm it works.
> >
> >---8<---
> > From a4afed6c44027afff82d6fa7503faef83b01fffe Mon Sep 17 00:00:00 2001
> >From: Wei Liu <wei.liu2@...rix.com>
> >Date: Thu, 15 May 2014 15:02:55 +0100
> >Subject: [PATCH] xen-netback: call napi_complete if vif is rate limited
> >
> >Reported-by: Jacek Konieczny <jajcus@...cus.net>
> >Signed-off-by: Wei Liu <wei.liu2@...rix.com>
> >Cc: Ian Campbell <ian.campbell@...rix.com>
> >Cc: Paul Durrant <paul.durrant@...rix.com>
> >Cc: David Vrabel <david.vrabel@...rix.com>
> >---
> >  drivers/net/xen-netback/common.h    |    2 +-
> >  drivers/net/xen-netback/interface.c |    5 +++--
> >  drivers/net/xen-netback/netback.c   |   12 ++++++++----
> >  3 files changed, 12 insertions(+), 7 deletions(-)
> >
> >diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> >index 4bf5b33..4c018de 100644
> >--- a/drivers/net/xen-netback/common.h
> >+++ b/drivers/net/xen-netback/common.h
> >@@ -219,7 +219,7 @@ void xenvif_check_rx_xenvif(struct xenvif *vif);
> >  /* Prevent the device from generating any further traffic. */
> >  void xenvif_carrier_off(struct xenvif *vif);
> >
> >-int xenvif_tx_action(struct xenvif *vif, int budget);
> >+int xenvif_tx_action(struct xenvif *vif, int budget, bool *rate_limited);
> >
> >  int xenvif_kthread(void *data);
> >  void xenvif_kick_thread(struct xenvif *vif);
> >diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
> >index 2e92d52..03cfbd6 100644
> >--- a/drivers/net/xen-netback/interface.c
> >+++ b/drivers/net/xen-netback/interface.c
> >@@ -61,6 +61,7 @@ static int xenvif_poll(struct napi_struct *napi, int budget)
> >  {
> >  	struct xenvif *vif = container_of(napi, struct xenvif, napi);
> >  	int work_done;
> >+	bool rate_limited;
> >
> >  	/* This vif is rogue, we pretend we've there is nothing to do
> >  	 * for this vif to deschedule it from NAPI. But this interface
> >@@ -71,7 +72,7 @@ static int xenvif_poll(struct napi_struct *napi, int budget)
> >  		return 0;
> >  	}
> >
> >-	work_done = xenvif_tx_action(vif, budget);
> >+	work_done = xenvif_tx_action(vif, budget, &rate_limited);
> >
> >  	if (work_done < budget) {
> >  		int more_to_do = 0;
> >@@ -96,7 +97,7 @@ static int xenvif_poll(struct napi_struct *napi, int budget)
> >  		local_irq_save(flags);
> >
> >  		RING_FINAL_CHECK_FOR_REQUESTS(&vif->tx, more_to_do);
> >-		if (!more_to_do)
> >+		if (!more_to_do || rate_limited)
> How about calling timer_pending(&vif->credit_timeout) instead?

timer_pending(&vif->credit_timeout) covers only one of two senarios of
"credit exceeded", see tx_credit_exceeded.

> Also, can this __napi_complete and the callback's napi_schedule race with
> each other? When napi_complete is between removing from the list and
> clearing the bit, and napi_schedule is just test&set the bit, the latter
> won't add the instance to the list again
> 

I think it should be fine. How is it different from what we already have
now? Is this something similar to what David once posted?

  <1395756505-21573-1-git-send-email-david.vrabel@...rix.com>

Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html