| Message ID | 1413983335-8307-2-git-send-email-david.vrabel@citrix.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Oct 22, 2014 at 02:08:53PM +0100, David Vrabel wrote: > Frontends that do not provide feature-rx-notify may stall because > netback depends on the notification from frontend to wake the guest Rx > thread (even if can_queue is false). > > This could be fixed but feature-rx-notify was introduced in 2006 and I > am not aware of any frontends that do not implement this. > > Signed-off-by: David Vrabel <david.vrabel@citrix.com> Acked-by: Wei Liu <wei.liu2@citrix.com> -- 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, Oct 23, 2014 at 12:16:29PM +0100, Wei Liu wrote: > On Wed, Oct 22, 2014 at 02:08:53PM +0100, David Vrabel wrote: > > Frontends that do not provide feature-rx-notify may stall because > > netback depends on the notification from frontend to wake the guest Rx > > thread (even if can_queue is false). > > > > This could be fixed but feature-rx-notify was introduced in 2006 and I > > am not aware of any frontends that do not implement this. > > > > Signed-off-by: David Vrabel <david.vrabel@citrix.com> > > Acked-by: Wei Liu <wei.liu2@citrix.com> While I can understand this patch by itself, can you elaborate a little bit on how it affects later patches? Because what I'm thinking is that this patch is not for stable while other two should go to stable. 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
On 23/10/14 12:32, Wei Liu wrote: > On Thu, Oct 23, 2014 at 12:16:29PM +0100, Wei Liu wrote: >> On Wed, Oct 22, 2014 at 02:08:53PM +0100, David Vrabel wrote: >>> Frontends that do not provide feature-rx-notify may stall because >>> netback depends on the notification from frontend to wake the guest Rx >>> thread (even if can_queue is false). >>> >>> This could be fixed but feature-rx-notify was introduced in 2006 and I >>> am not aware of any frontends that do not implement this. >>> >>> Signed-off-by: David Vrabel <david.vrabel@citrix.com> >> >> Acked-by: Wei Liu <wei.liu2@citrix.com> > > While I can understand this patch by itself, can you elaborate a little > bit on how it affects later patches? Because what I'm thinking is that > this patch is not for stable while other two should go to stable. From the cover letter: "The first patch is a prerequite. Removing support for frontends with feature-rx-notify makes it easier to reason about the correctness of netback since it no longer has to support this outdated and broken mode." The other patches do not meet the stable kernel requirements (they're too long one thing). David -- 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, Oct 23, 2014 at 12:37:54PM +0100, David Vrabel wrote: > On 23/10/14 12:32, Wei Liu wrote: > > On Thu, Oct 23, 2014 at 12:16:29PM +0100, Wei Liu wrote: > >> On Wed, Oct 22, 2014 at 02:08:53PM +0100, David Vrabel wrote: > >>> Frontends that do not provide feature-rx-notify may stall because > >>> netback depends on the notification from frontend to wake the guest Rx > >>> thread (even if can_queue is false). > >>> > >>> This could be fixed but feature-rx-notify was introduced in 2006 and I > >>> am not aware of any frontends that do not implement this. > >>> > >>> Signed-off-by: David Vrabel <david.vrabel@citrix.com> > >> > >> Acked-by: Wei Liu <wei.liu2@citrix.com> > > > > While I can understand this patch by itself, can you elaborate a little > > bit on how it affects later patches? Because what I'm thinking is that > > this patch is not for stable while other two should go to stable. > > >From the cover letter: > > "The first patch is a prerequite. Removing support for frontends with > feature-rx-notify makes it easier to reason about the correctness of > netback since it no longer has to support this outdated and broken > mode." > I saw that. I think you should make it a little bit clearer. The queue is not guaranteed to stop if we keep this feature. > The other patches do not meet the stable kernel requirements (they're > too long one thing). > Does length matter? I surely had written long patch for stable. Wei. > David -- 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 23/10/14 12:44, Wei Liu wrote: > On Thu, Oct 23, 2014 at 12:37:54PM +0100, David Vrabel wrote: >> On 23/10/14 12:32, Wei Liu wrote: >>> On Thu, Oct 23, 2014 at 12:16:29PM +0100, Wei Liu wrote: >>>> On Wed, Oct 22, 2014 at 02:08:53PM +0100, David Vrabel wrote: >>>>> Frontends that do not provide feature-rx-notify may stall because >>>>> netback depends on the notification from frontend to wake the guest Rx >>>>> thread (even if can_queue is false). >>>>> >>>>> This could be fixed but feature-rx-notify was introduced in 2006 and I >>>>> am not aware of any frontends that do not implement this. >>>>> >>>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com> >>>> >>>> Acked-by: Wei Liu <wei.liu2@citrix.com> >>> >>> While I can understand this patch by itself, can you elaborate a little >>> bit on how it affects later patches? Because what I'm thinking is that >>> this patch is not for stable while other two should go to stable. >> >> >From the cover letter: >> >> "The first patch is a prerequite. Removing support for frontends with >> feature-rx-notify makes it easier to reason about the correctness of >> netback since it no longer has to support this outdated and broken >> mode." >> > > I saw that. > > I think you should make it a little bit clearer. I'm not sure how I can make this any clearer. Perhaps you should wander over to my desk to discuss this in person? > The queue is not guaranteed to stop if we keep this feature. > >> The other patches do not meet the stable kernel requirements (they're >> too long one thing). >> > > Does length matter? I surely had written long patch for stable. From Documentation/stable_kernel_rules.txt: "- It cannot be bigger than 100 lines, with context." David -- 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/common.h b/drivers/net/xen-netback/common.h index d4eb8d2..93ca77c 100644 --- a/drivers/net/xen-netback/common.h +++ b/drivers/net/xen-netback/common.h @@ -228,9 +228,6 @@ struct xenvif { u8 ip_csum:1; u8 ipv6_csum:1; - /* Internal feature information. */ - u8 can_queue:1; /* can queue packets for receiver? */ - /* Is this interface disabled? True when backend discovers * frontend is rogue. */ @@ -272,8 +269,6 @@ void xenvif_xenbus_fini(void); int xenvif_schedulable(struct xenvif *vif); -int xenvif_must_stop_queue(struct xenvif_queue *queue); - int xenvif_queue_stopped(struct xenvif_queue *queue); void xenvif_wake_queue(struct xenvif_queue *queue); diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c index f379689..c6759b1 100644 --- a/drivers/net/xen-netback/interface.c +++ b/drivers/net/xen-netback/interface.c @@ -60,16 +60,6 @@ void xenvif_skb_zerocopy_complete(struct xenvif_queue *queue) atomic_dec(&queue->inflight_packets); } -static inline void xenvif_stop_queue(struct xenvif_queue *queue) -{ - struct net_device *dev = queue->vif->dev; - - if (!queue->vif->can_queue) - return; - - netif_tx_stop_queue(netdev_get_tx_queue(dev, queue->id)); -} - int xenvif_schedulable(struct xenvif *vif) { return netif_running(vif->dev) && @@ -209,7 +199,7 @@ static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev) if (!xenvif_rx_ring_slots_available(queue, min_slots_needed)) { queue->rx_stalled.function = xenvif_rx_stalled; queue->rx_stalled.data = (unsigned long)queue; - xenvif_stop_queue(queue); + netif_tx_stop_queue(netdev_get_tx_queue(dev, queue->id)); mod_timer(&queue->rx_stalled, jiffies + rx_drain_timeout_jiffies); } diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c index 8079c31..9060857 100644 --- a/drivers/net/xen-netback/xenbus.c +++ b/drivers/net/xen-netback/xenbus.c @@ -873,15 +873,10 @@ static int read_xenbus_vif_flags(struct backend_info *be) if (!rx_copy) return -EOPNOTSUPP; - if (vif->dev->tx_queue_len != 0) { - if (xenbus_scanf(XBT_NIL, dev->otherend, - "feature-rx-notify", "%d", &val) < 0) - val = 0; - if (val) - vif->can_queue = 1; - else - /* Must be non-zero for pfifo_fast to work. */ - vif->dev->tx_queue_len = 1; + if (xenbus_scanf(XBT_NIL, dev->otherend, + "feature-rx-notify", "%d", &val) < 0 || val == 0) { + xenbus_dev_fatal(dev, -EINVAL, "feature-rx-notify is mandatory"); + return -EINVAL; } if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-sg",
Frontends that do not provide feature-rx-notify may stall because netback depends on the notification from frontend to wake the guest Rx thread (even if can_queue is false). This could be fixed but feature-rx-notify was introduced in 2006 and I am not aware of any frontends that do not implement this. Signed-off-by: David Vrabel <david.vrabel@citrix.com> --- drivers/net/xen-netback/common.h | 5 ----- drivers/net/xen-netback/interface.c | 12 +----------- drivers/net/xen-netback/xenbus.c | 13 ++++--------- 3 files changed, 5 insertions(+), 25 deletions(-)