| Message ID | 1400155158-13527-1-git-send-email-wei.liu2@citrix.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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@jajcus.net> > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > Acked-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Paul Durrant <paul.durrant@citrix.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'). Greets, Jacek -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@jajcus.net> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > Cc: Paul Durrant <paul.durrant@citrix.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'). > Hmm... I will look into it further. Wei. > Greets, > Jacek -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/05/14 12: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). No it does not. RING_FINAL_CHECK_FOR_REQUESTS() only advances the event index if the ring is empty. This will also result in xenvif_up() failing to properly enable the event. I think Jacek's bug may be that netback fails to call napi_complete() when credit is exceeded and there still outstanding requests on the from-guest ring and thus napi repeatedly polls. David > > Reported-by: Jacek Konieczny <jajcus@jajcus.net> > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > Acked-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Paul Durrant <paul.durrant@citrix.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); > > if (more_to_do) > napi_schedule(&vif->napi); > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@jajcus.net> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > Cc: Paul Durrant <paul.durrant@citrix.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'). > Oh I was looking at the aggregated stats that's why the number looked normal to me. :-/ > Greets, > Jacek -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 15, 2014 at 02:40:58PM +0100, David Vrabel wrote: > On 15/05/14 12: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). > > No it does not. RING_FINAL_CHECK_FOR_REQUESTS() only advances the event > index if the ring is empty. > > This will also result in xenvif_up() failing to properly enable the event. > > I think Jacek's bug may be that netback fails to call napi_complete() > when credit is exceeded and there still outstanding requests on the > from-guest ring and thus napi repeatedly polls. > Correct. We should call napi_complete if this vif is rate limited. Wei. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); if (more_to_do) napi_schedule(&vif->napi);