| Message ID | 1403100558-12866-1-git-send-email-wei.liu2@citrix.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 06/18/2014 10:09 AM, Wei Liu wrote: > The original code uses netdev->real_num_tx_queues to bookkeep number of > queues and invokes netif_set_real_num_tx_queues to set the number of > queues. However, netif_set_real_num_tx_queues doesn't allow > real_num_tx_queues to be smaller than 1, which means setting the number > to 0 will not work and real_num_tx_queues is untouched. > > This is bogus when xenvif_free is invoked before any number of queues is > allocated. That function needs to iterate through all queues to free > resources. Using the wrong number of queues results in NULL pointer > dereference. > > So we bookkeep the number of queues in xen-netback to solve this > problem. The usage of real_num_tx_queues in core driver is to cap queue > index to a valid value. In start_xmit we've already guarded against out > of range queue index so we should be fine. > > This fixes a regression introduced by multiqueue patchset in 3.16-rc1. David sent a couple of patches earlier today that I have been testing and they appear to fix both netfront and netback. (I am waiting for 32-bit to finish) http://lists.xenproject.org/archives/html/xen-devel/2014-06/msg02308.html -boris > > Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > CC: Ian Campbell <ian.campbell@citrix.com> > --- > drivers/net/xen-netback/common.h | 1 + > drivers/net/xen-netback/interface.c | 30 +++++++++++++----------------- > drivers/net/xen-netback/xenbus.c | 20 ++++++-------------- > 3 files changed, 20 insertions(+), 31 deletions(-) > > diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h > index 4dd7c4a..93602a6 100644 > --- a/drivers/net/xen-netback/common.h > +++ b/drivers/net/xen-netback/common.h > @@ -222,6 +222,7 @@ struct xenvif { > > /* Queues */ > struct xenvif_queue *queues; > + unsigned int num_queues; > > /* Miscellaneous private stuff. */ > struct net_device *dev; > diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c > index 852da34..7daf64c 100644 > --- a/drivers/net/xen-netback/interface.c > +++ b/drivers/net/xen-netback/interface.c > @@ -140,7 +140,8 @@ static void xenvif_wake_queue_callback(unsigned long data) > static u16 xenvif_select_queue(struct net_device *dev, struct sk_buff *skb, > void *accel_priv, select_queue_fallback_t fallback) > { > - unsigned int num_queues = dev->real_num_tx_queues; > + struct xenvif *vif = netdev_priv(dev); > + unsigned int num_queues = vif->num_queues; > u32 hash; > u16 queue_index; > > @@ -162,7 +163,7 @@ static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev) > { > struct xenvif *vif = netdev_priv(dev); > struct xenvif_queue *queue = NULL; > - unsigned int num_queues = dev->real_num_tx_queues; > + unsigned int num_queues = vif->num_queues; > u16 index; > int min_slots_needed; > > @@ -225,7 +226,7 @@ static struct net_device_stats *xenvif_get_stats(struct net_device *dev) > { > struct xenvif *vif = netdev_priv(dev); > struct xenvif_queue *queue = NULL; > - unsigned int num_queues = dev->real_num_tx_queues; > + unsigned int num_queues = vif->num_queues; > unsigned long rx_bytes = 0; > unsigned long rx_packets = 0; > unsigned long tx_bytes = 0; > @@ -256,7 +257,7 @@ out: > static void xenvif_up(struct xenvif *vif) > { > struct xenvif_queue *queue = NULL; > - unsigned int num_queues = vif->dev->real_num_tx_queues; > + unsigned int num_queues = vif->num_queues; > unsigned int queue_index; > > for (queue_index = 0; queue_index < num_queues; ++queue_index) { > @@ -272,7 +273,7 @@ static void xenvif_up(struct xenvif *vif) > static void xenvif_down(struct xenvif *vif) > { > struct xenvif_queue *queue = NULL; > - unsigned int num_queues = vif->dev->real_num_tx_queues; > + unsigned int num_queues = vif->num_queues; > unsigned int queue_index; > > for (queue_index = 0; queue_index < num_queues; ++queue_index) { > @@ -379,7 +380,7 @@ static void xenvif_get_ethtool_stats(struct net_device *dev, > struct ethtool_stats *stats, u64 * data) > { > struct xenvif *vif = netdev_priv(dev); > - unsigned int num_queues = dev->real_num_tx_queues; > + unsigned int num_queues = vif->num_queues; > int i; > unsigned int queue_index; > struct xenvif_stats *vif_stats; > @@ -436,10 +437,7 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid, > char name[IFNAMSIZ] = {}; > > snprintf(name, IFNAMSIZ - 1, "vif%u.%u", domid, handle); > - /* Allocate a netdev with the max. supported number of queues. > - * When the guest selects the desired number, it will be updated > - * via netif_set_real_num_tx_queues(). > - */ > + /* Allocate a netdev with the max. supported number of queues. */ > dev = alloc_netdev_mq(sizeof(struct xenvif), name, ether_setup, > xenvif_max_queues); > if (dev == NULL) { > @@ -458,11 +456,9 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid, > vif->dev = dev; > vif->disabled = false; > > - /* Start out with no queues. The call below does not require > - * rtnl_lock() as it happens before register_netdev(). > - */ > + /* Start out with no queues. */ > vif->queues = NULL; > - netif_set_real_num_tx_queues(dev, 0); > + vif->num_queues = 0; > > dev->netdev_ops = &xenvif_netdev_ops; > dev->hw_features = NETIF_F_SG | > @@ -677,7 +673,7 @@ static void xenvif_wait_unmap_timeout(struct xenvif_queue *queue, > void xenvif_disconnect(struct xenvif *vif) > { > struct xenvif_queue *queue = NULL; > - unsigned int num_queues = vif->dev->real_num_tx_queues; > + unsigned int num_queues = vif->num_queues; > unsigned int queue_index; > > if (netif_carrier_ok(vif->dev)) > @@ -724,7 +720,7 @@ void xenvif_deinit_queue(struct xenvif_queue *queue) > void xenvif_free(struct xenvif *vif) > { > struct xenvif_queue *queue = NULL; > - unsigned int num_queues = vif->dev->real_num_tx_queues; > + unsigned int num_queues = vif->num_queues; > unsigned int queue_index; > /* Here we want to avoid timeout messages if an skb can be legitimately > * stuck somewhere else. Realistically this could be an another vif's > @@ -751,9 +747,9 @@ void xenvif_free(struct xenvif *vif) > /* Free the array of queues. The call below does not require > * rtnl_lock() because it happens after unregister_netdev(). > */ > - netif_set_real_num_tx_queues(vif->dev, 0); > vfree(vif->queues); > vif->queues = NULL; > + vif->num_queues = 0; > > free_netdev(vif->dev); > > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c > index 96c63dc2..c724538 100644 > --- a/drivers/net/xen-netback/xenbus.c > +++ b/drivers/net/xen-netback/xenbus.c > @@ -527,9 +527,7 @@ static void connect(struct backend_info *be) > /* Use the number of queues requested by the frontend */ > be->vif->queues = vzalloc(requested_num_queues * > sizeof(struct xenvif_queue)); > - rtnl_lock(); > - netif_set_real_num_tx_queues(be->vif->dev, requested_num_queues); > - rtnl_unlock(); > + be->vif->num_queues = requested_num_queues; > > for (queue_index = 0; queue_index < requested_num_queues; ++queue_index) { > queue = &be->vif->queues[queue_index]; > @@ -546,9 +544,7 @@ static void connect(struct backend_info *be) > * earlier queues can be destroyed using the regular > * disconnect logic. > */ > - rtnl_lock(); > - netif_set_real_num_tx_queues(be->vif->dev, queue_index); > - rtnl_unlock(); > + be->vif->num_queues = queue_index; > goto err; > } > > @@ -561,9 +557,7 @@ static void connect(struct backend_info *be) > * and also clean up any previously initialised queues. > */ > xenvif_deinit_queue(queue); > - rtnl_lock(); > - netif_set_real_num_tx_queues(be->vif->dev, queue_index); > - rtnl_unlock(); > + be->vif->num_queues = queue_index; > goto err; > } > } > @@ -582,13 +576,11 @@ static void connect(struct backend_info *be) > return; > > err: > - if (be->vif->dev->real_num_tx_queues > 0) > + if (be->vif->num_queues > 0) > xenvif_disconnect(be->vif); /* Clean up existing queues */ > vfree(be->vif->queues); > be->vif->queues = NULL; > - rtnl_lock(); > - netif_set_real_num_tx_queues(be->vif->dev, 0); > - rtnl_unlock(); > + be->vif->num_queues = 0; > return; > } > > @@ -596,7 +588,7 @@ err: > static int connect_rings(struct backend_info *be, struct xenvif_queue *queue) > { > struct xenbus_device *dev = be->dev; > - unsigned int num_queues = queue->vif->dev->real_num_tx_queues; > + unsigned int num_queues = queue->vif->num_queues; > unsigned long tx_ring_ref, rx_ring_ref; > unsigned int tx_evtchn, rx_evtchn; > int err; -- 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 Wed, Jun 18, 2014 at 10:18:50AM -0400, Boris Ostrovsky wrote: > On 06/18/2014 10:09 AM, Wei Liu wrote: > >The original code uses netdev->real_num_tx_queues to bookkeep number of > >queues and invokes netif_set_real_num_tx_queues to set the number of > >queues. However, netif_set_real_num_tx_queues doesn't allow > >real_num_tx_queues to be smaller than 1, which means setting the number > >to 0 will not work and real_num_tx_queues is untouched. > > > >This is bogus when xenvif_free is invoked before any number of queues is > >allocated. That function needs to iterate through all queues to free > >resources. Using the wrong number of queues results in NULL pointer > >dereference. > > > >So we bookkeep the number of queues in xen-netback to solve this > >problem. The usage of real_num_tx_queues in core driver is to cap queue > >index to a valid value. In start_xmit we've already guarded against out > >of range queue index so we should be fine. > > > >This fixes a regression introduced by multiqueue patchset in 3.16-rc1. > > > David sent a couple of patches earlier today that I have been testing and > they appear to fix both netfront and netback. (I am waiting for 32-bit to > finish) > > http://lists.xenproject.org/archives/html/xen-devel/2014-06/msg02308.html > I saw that, but they don't fix this backend bug. Try crashing the guest before it connects to backend. As I said in commit message: > >This is bogus when xenvif_free is invoked before any number of queues is > >allocated. That function needs to iterate through all queues to free netif_set_real_num_tx_queues will need to be removed anyway. 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 06/18/2014 10:21 AM, Wei Liu wrote: > On Wed, Jun 18, 2014 at 10:18:50AM -0400, Boris Ostrovsky wrote: >> On 06/18/2014 10:09 AM, Wei Liu wrote: >>> The original code uses netdev->real_num_tx_queues to bookkeep number of >>> queues and invokes netif_set_real_num_tx_queues to set the number of >>> queues. However, netif_set_real_num_tx_queues doesn't allow >>> real_num_tx_queues to be smaller than 1, which means setting the number >>> to 0 will not work and real_num_tx_queues is untouched. >>> >>> This is bogus when xenvif_free is invoked before any number of queues is >>> allocated. That function needs to iterate through all queues to free >>> resources. Using the wrong number of queues results in NULL pointer >>> dereference. >>> >>> So we bookkeep the number of queues in xen-netback to solve this >>> problem. The usage of real_num_tx_queues in core driver is to cap queue >>> index to a valid value. In start_xmit we've already guarded against out >>> of range queue index so we should be fine. >>> >>> This fixes a regression introduced by multiqueue patchset in 3.16-rc1. >> >> David sent a couple of patches earlier today that I have been testing and >> they appear to fix both netfront and netback. (I am waiting for 32-bit to >> finish) >> >> http://lists.xenproject.org/archives/html/xen-devel/2014-06/msg02308.html >> > I saw that, but they don't fix this backend bug. Try crashing the guest > before it connects to backend. As I said in commit message: Apparently it doesn't indeed since 32-bit just crashed on me in xenvif_free() (the moment I hit Send on my response to you). But 64-bit run completed without failures. And even 32-bit test ran fine for a while. -boris > >>> This is bogus when xenvif_free is invoked before any number of queues is >>> allocated. That function needs to iterate through all queues to free > netif_set_real_num_tx_queues will need to be removed anyway. > > 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
Wednesday, June 18, 2014, 4:21:53 PM, you wrote: > On Wed, Jun 18, 2014 at 10:18:50AM -0400, Boris Ostrovsky wrote: >> On 06/18/2014 10:09 AM, Wei Liu wrote: >> >The original code uses netdev->real_num_tx_queues to bookkeep number of >> >queues and invokes netif_set_real_num_tx_queues to set the number of >> >queues. However, netif_set_real_num_tx_queues doesn't allow >> >real_num_tx_queues to be smaller than 1, which means setting the number >> >to 0 will not work and real_num_tx_queues is untouched. >> > >> >This is bogus when xenvif_free is invoked before any number of queues is >> >allocated. That function needs to iterate through all queues to free >> >resources. Using the wrong number of queues results in NULL pointer >> >dereference. >> > >> >So we bookkeep the number of queues in xen-netback to solve this >> >problem. The usage of real_num_tx_queues in core driver is to cap queue >> >index to a valid value. In start_xmit we've already guarded against out >> >of range queue index so we should be fine. >> > >> >This fixes a regression introduced by multiqueue patchset in 3.16-rc1. >> >> >> David sent a couple of patches earlier today that I have been testing and >> they appear to fix both netfront and netback. (I am waiting for 32-bit to >> finish) >> >> http://lists.xenproject.org/archives/html/xen-devel/2014-06/msg02308.html >> > I saw that, but they don't fix this backend bug. Try crashing the guest > before it connects to backend. As I said in commit message: Hmm i guess that was the bug_on i hit some time ago with testing the multiqueue patches, from what i remember i indeed had guest that crashed earlier on. >> >This is bogus when xenvif_free is invoked before any number of queues is >> >allocated. That function needs to iterate through all queues to free > netif_set_real_num_tx_queues will need to be removed anyway. > 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 Wed, Jun 18, 2014 at 04:34:47PM +0200, Sander Eikelenboom wrote: [...] > >> David sent a couple of patches earlier today that I have been testing and > >> they appear to fix both netfront and netback. (I am waiting for 32-bit to > >> finish) > >> > >> http://lists.xenproject.org/archives/html/xen-devel/2014-06/msg02308.html > >> > > > I saw that, but they don't fix this backend bug. Try crashing the guest > > before it connects to backend. As I said in commit message: > > Hmm i guess that was the bug_on i hit some time ago with testing the multiqueue > patches, from what i remember i indeed had guest that crashed earlier on. > Probably. The oops message in your report looked very much alike. Just that I didn't have convenient mean to reproduce so I couldn't fix it earlier. :-) 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
From: Wei Liu <wei.liu2@citrix.com> Date: Wed, 18 Jun 2014 15:09:18 +0100 > The original code uses netdev->real_num_tx_queues to bookkeep number of > queues and invokes netif_set_real_num_tx_queues to set the number of > queues. However, netif_set_real_num_tx_queues doesn't allow > real_num_tx_queues to be smaller than 1, which means setting the number > to 0 will not work and real_num_tx_queues is untouched. > > This is bogus when xenvif_free is invoked before any number of queues is > allocated. That function needs to iterate through all queues to free > resources. Using the wrong number of queues results in NULL pointer > dereference. > > So we bookkeep the number of queues in xen-netback to solve this > problem. The usage of real_num_tx_queues in core driver is to cap queue > index to a valid value. In start_xmit we've already guarded against out > of range queue index so we should be fine. > > This fixes a regression introduced by multiqueue patchset in 3.16-rc1. > > Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Signed-off-by: Wei Liu <wei.liu2@citrix.com> I say you should have a select queue method at all. You're essentially providing a half-assed version of __netdev_pick_tx() except that: 1) You're _completely_ ignoring the socket hash, if any. 2) You're not allowing XPS to work, _at all_. I think you need to serious reevaluate providing any select queue method at all, just let netdev_pick_tx() do all the work. If you have some issue maintaining the release of queue resources, maintain that privately and keep those details in the queue resource allocation and freeing code _only_. Don't make it an issue that interferes at all with the normal mechanisms for SKB tx queue selection. Thanks. -- 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 Sun, Jun 22, 2014 at 12:13:55AM -0700, David Miller wrote: > From: Wei Liu <wei.liu2@citrix.com> > Date: Wed, 18 Jun 2014 15:09:18 +0100 > > > The original code uses netdev->real_num_tx_queues to bookkeep number of > > queues and invokes netif_set_real_num_tx_queues to set the number of > > queues. However, netif_set_real_num_tx_queues doesn't allow > > real_num_tx_queues to be smaller than 1, which means setting the number > > to 0 will not work and real_num_tx_queues is untouched. > > > > This is bogus when xenvif_free is invoked before any number of queues is > > allocated. That function needs to iterate through all queues to free > > resources. Using the wrong number of queues results in NULL pointer > > dereference. > > > > So we bookkeep the number of queues in xen-netback to solve this > > problem. The usage of real_num_tx_queues in core driver is to cap queue > > index to a valid value. In start_xmit we've already guarded against out > > of range queue index so we should be fine. > > > > This fixes a regression introduced by multiqueue patchset in 3.16-rc1. > > > > Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > I say you should have a select queue method at all. > > You're essentially providing a half-assed version of __netdev_pick_tx() > except that: > > 1) You're _completely_ ignoring the socket hash, if any. > > 2) You're not allowing XPS to work, _at all_. > > I think you need to serious reevaluate providing any select queue > method at all, just let netdev_pick_tx() do all the work. > Looking at the core driver code in more details I think you're right. I will remove the select queue method. > If you have some issue maintaining the release of queue resources, > maintain that privately and keep those details in the queue resource > allocation and freeing code _only_. Don't make it an issue that > interferes at all with the normal mechanisms for SKB tx queue > selection. > Sure. This is exactly the main idea of this patch, just that it interfered the queue selection logic. :-( Will send an updated version soon. Wei. > Thanks. -- 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
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev- > owner@vger.kernel.org] On Behalf Of Wei Liu > Sent: 22 June 2014 12:17 > To: David Miller > Cc: Wei Liu; xen-devel@lists.xen.org; netdev@vger.kernel.org; > boris.ostrovsky@oracle.com; Ian Campbell > Subject: Re: [PATCH net] xen-netback: bookkeep number of queues in our > own module > > On Sun, Jun 22, 2014 at 12:13:55AM -0700, David Miller wrote: > > From: Wei Liu <wei.liu2@citrix.com> > > Date: Wed, 18 Jun 2014 15:09:18 +0100 > > > > > The original code uses netdev->real_num_tx_queues to bookkeep > number of > > > queues and invokes netif_set_real_num_tx_queues to set the number > of > > > queues. However, netif_set_real_num_tx_queues doesn't allow > > > real_num_tx_queues to be smaller than 1, which means setting the > number > > > to 0 will not work and real_num_tx_queues is untouched. > > > > > > This is bogus when xenvif_free is invoked before any number of queues > is > > > allocated. That function needs to iterate through all queues to free > > > resources. Using the wrong number of queues results in NULL pointer > > > dereference. > > > > > > So we bookkeep the number of queues in xen-netback to solve this > > > problem. The usage of real_num_tx_queues in core driver is to cap > queue > > > index to a valid value. In start_xmit we've already guarded against out > > > of range queue index so we should be fine. > > > > > > This fixes a regression introduced by multiqueue patchset in 3.16-rc1. > > > > > > Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > > I say you should have a select queue method at all. > > > > You're essentially providing a half-assed version of __netdev_pick_tx() > > except that: > > > > 1) You're _completely_ ignoring the socket hash, if any. > > > > 2) You're not allowing XPS to work, _at all_. > > > > I think you need to serious reevaluate providing any select queue > > method at all, just let netdev_pick_tx() do all the work. > > > > Looking at the core driver code in more details I think you're right. I > will remove the select queue method. > Bear in mind that the original intention of the multi-queue patches was to allow the queue selection algorithm to be negotiated with the frontend (see http://lists.xen.org/archives/html/xen-devel/2013-06/msg02654.html). Particularly, if the frontend is Windows then netback will need to use a Toeplitz hash to steer traffic since this is stipulated by Microsoft's RSS (Receive Side Scaling) interfaces. So, IMO netback should always implement a select queue method, otherwise any (theoretical) algorithm change in __netdev_pick_tx() would be immediately imposed on frontends, possibly causing them to misbehave. Paul > > If you have some issue maintaining the release of queue resources, > > maintain that privately and keep those details in the queue resource > > allocation and freeing code _only_. Don't make it an issue that > > interferes at all with the normal mechanisms for SKB tx queue > > selection. > > > > Sure. This is exactly the main idea of this patch, just that it > interfered the queue selection logic. :-( > > Will send an updated version soon. > > Wei. > > > Thanks. > -- > 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 -- 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 Mon, Jun 23, 2014 at 08:59:16AM +0100, Paul Durrant wrote: [...] > > > > So we bookkeep the number of queues in xen-netback to solve this > > > > problem. The usage of real_num_tx_queues in core driver is to cap > > queue > > > > index to a valid value. In start_xmit we've already guarded against out > > > > of range queue index so we should be fine. > > > > > > > > This fixes a regression introduced by multiqueue patchset in 3.16-rc1. > > > > > > > > Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > > > > I say you should have a select queue method at all. > > > > > > You're essentially providing a half-assed version of __netdev_pick_tx() > > > except that: > > > > > > 1) You're _completely_ ignoring the socket hash, if any. > > > > > > 2) You're not allowing XPS to work, _at all_. > > > > > > I think you need to serious reevaluate providing any select queue > > > method at all, just let netdev_pick_tx() do all the work. > > > > > > > Looking at the core driver code in more details I think you're right. I > > will remove the select queue method. > > > > Bear in mind that the original intention of the multi-queue patches > was to allow the queue selection algorithm to be negotiated with the > frontend (see > http://lists.xen.org/archives/html/xen-devel/2013-06/msg02654.html). > Particularly, if the frontend is Windows then netback will need to use > a Toeplitz hash to steer traffic since this is stipulated by > Microsoft's RSS (Receive Side Scaling) interfaces. So, IMO netback > should always implement a select queue method, otherwise any > (theoretical) algorithm change in __netdev_pick_tx() would be > immediately imposed on frontends, possibly causing them to misbehave. > In that case we can easily add back this select queue routine when the implementation comes, right? On the other hand, we can exploit the fact that current select queue method takes a "fallback" parameter and it points to __netdev_pick_tx, we can have something like: static u16 xenvif_select_queue(struct net_device *dev, struct sk_buff *skb, void *accel_priv, select_queue_fallback_t fallback) { return fallback(dev, skb); } But that doesn't make much difference and is a bit redundant IMHO. Wei. > Paul > > > > If you have some issue maintaining the release of queue resources, > > > maintain that privately and keep those details in the queue resource > > > allocation and freeing code _only_. Don't make it an issue that > > > interferes at all with the normal mechanisms for SKB tx queue > > > selection. > > > > > > > Sure. This is exactly the main idea of this patch, just that it > > interfered the queue selection logic. :-( > > > > Will send an updated version soon. > > > > Wei. > > > > > Thanks. > > -- > > 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 -- 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
From: Paul Durrant <Paul.Durrant@citrix.com> Date: Mon, 23 Jun 2014 07:59:16 +0000 > Bear in mind that the original intention of the multi-queue patches > was to allow the queue selection algorithm to be negotiated with the > frontend (see > http://lists.xen.org/archives/html/xen-devel/2013-06/msg02654.html). Particularly, > if the frontend is Windows then netback will need to use a Toeplitz > hash to steer traffic since this is stipulated by Microsoft's RSS > (Receive Side Scaling) interfaces. So, IMO netback should always > implement a select queue method, otherwise any (theoretical) > algorithm change in __netdev_pick_tx() would be immediately imposed > on frontends, possibly causing them to misbehave. I do not think you are obligated to use Toeplitz or whatever scheme the other side wants, especially if the user on the xen-netback side asked for XPS or similar to be used. -- 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
> -----Original Message----- > From: Wei Liu [mailto:wei.liu2@citrix.com] > Sent: 23 June 2014 09:49 > To: Paul Durrant > Cc: Wei Liu; David Miller; xen-devel@lists.xen.org; netdev@vger.kernel.org; > boris.ostrovsky@oracle.com; Ian Campbell > Subject: Re: [PATCH net] xen-netback: bookkeep number of queues in our > own module > > On Mon, Jun 23, 2014 at 08:59:16AM +0100, Paul Durrant wrote: > [...] > > > > > So we bookkeep the number of queues in xen-netback to solve this > > > > > problem. The usage of real_num_tx_queues in core driver is to cap > > > queue > > > > > index to a valid value. In start_xmit we've already guarded against out > > > > > of range queue index so we should be fine. > > > > > > > > > > This fixes a regression introduced by multiqueue patchset in 3.16-rc1. > > > > > > > > > > Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > > > > > > I say you should have a select queue method at all. > > > > > > > > You're essentially providing a half-assed version of __netdev_pick_tx() > > > > except that: > > > > > > > > 1) You're _completely_ ignoring the socket hash, if any. > > > > > > > > 2) You're not allowing XPS to work, _at all_. > > > > > > > > I think you need to serious reevaluate providing any select queue > > > > method at all, just let netdev_pick_tx() do all the work. > > > > > > > > > > Looking at the core driver code in more details I think you're right. I > > > will remove the select queue method. > > > > > > > Bear in mind that the original intention of the multi-queue patches > > was to allow the queue selection algorithm to be negotiated with the > > frontend (see > > http://lists.xen.org/archives/html/xen-devel/2013-06/msg02654.html). > > Particularly, if the frontend is Windows then netback will need to use > > a Toeplitz hash to steer traffic since this is stipulated by > > Microsoft's RSS (Receive Side Scaling) interfaces. So, IMO netback > > should always implement a select queue method, otherwise any > > (theoretical) algorithm change in __netdev_pick_tx() would be > > immediately imposed on frontends, possibly causing them to misbehave. > > > > In that case we can easily add back this select queue routine when the > implementation comes, right? > Ok, for the addition of a new algorithm that may be the case but what about the existent algorithm stability issue? Who's going to watch the implementation of __netdev_pick_tx() and make sure there is no semantic change? I'm still of the opinion that the implementation should be kept within netback so that we can guarantee stability, even if it means duplication. Either that or we need some guarantee that the semantic of __netdev_pick_tx() will never change. Paul > On the other hand, we can exploit the fact that current select queue > method takes a "fallback" parameter and it points to __netdev_pick_tx, > we can have something like: > > static u16 xenvif_select_queue(struct net_device *dev, struct sk_buff *skb, > void *accel_priv, select_queue_fallback_t fallback) > { > return fallback(dev, skb); > } > > But that doesn't make much difference and is a bit redundant IMHO. > > > Wei. > > > Paul > > > > > > If you have some issue maintaining the release of queue resources, > > > > maintain that privately and keep those details in the queue resource > > > > allocation and freeing code _only_. Don't make it an issue that > > > > interferes at all with the normal mechanisms for SKB tx queue > > > > selection. > > > > > > > > > > Sure. This is exactly the main idea of this patch, just that it > > > interfered the queue selection logic. :-( > > > > > > Will send an updated version soon. > > > > > > Wei. > > > > > > > Thanks. > > > -- > > > 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 -- 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
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: 23 June 2014 10:09 > To: Paul Durrant > Cc: Wei Liu; xen-devel@lists.xen.org; netdev@vger.kernel.org; > boris.ostrovsky@oracle.com; Ian Campbell > Subject: Re: [PATCH net] xen-netback: bookkeep number of queues in our > own module > > From: Paul Durrant <Paul.Durrant@citrix.com> > Date: Mon, 23 Jun 2014 07:59:16 +0000 > > > Bear in mind that the original intention of the multi-queue patches > > was to allow the queue selection algorithm to be negotiated with the > > frontend (see > > http://lists.xen.org/archives/html/xen-devel/2013-06/msg02654.html). > Particularly, > > if the frontend is Windows then netback will need to use a Toeplitz > > hash to steer traffic since this is stipulated by Microsoft's RSS > > (Receive Side Scaling) interfaces. So, IMO netback should always > > implement a select queue method, otherwise any (theoretical) > > algorithm change in __netdev_pick_tx() would be immediately imposed > > on frontends, possibly causing them to misbehave. > > I do not think you are obligated to use Toeplitz or whatever scheme > the other side wants, especially if the user on the xen-netback side > asked for XPS or similar to be used. If the frontend can only cope with one steering algorithm then how's that going to work? Flows would come in on the 'wrong' queue as far as the frontend is concerned and then who knows what would happen? If the frontend says it can only handle 'algorithm X' then the backend should either guarantee it's going to use 'algorthm X' or it should use a single queue. Paul -- 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
From: Paul Durrant <Paul.Durrant@citrix.com> Date: Mon, 23 Jun 2014 09:18:23 +0000 > Ok, for the addition of a new algorithm that may be the case but > what about the existent algorithm stability issue? Who's going to > watch the implementation of __netdev_pick_tx() and make sure there > is no semantic change? I'm still of the opinion that the > implementation should be kept within netback so that we can > guarantee stability, even if it means duplication. Either that or we > need some guarantee that the semantic of __netdev_pick_tx() will > never change. Please stop talking like this. If the default changes it's probably to fix a bug or make things better, you want no bugs to get fix if it breaks "stability"? -- 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
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: 23 June 2014 10:26 > To: Paul Durrant > Cc: Wei Liu; xen-devel@lists.xen.org; netdev@vger.kernel.org; > boris.ostrovsky@oracle.com; Ian Campbell > Subject: Re: [PATCH net] xen-netback: bookkeep number of queues in our > own module > > From: Paul Durrant <Paul.Durrant@citrix.com> > Date: Mon, 23 Jun 2014 09:18:23 +0000 > > > Ok, for the addition of a new algorithm that may be the case but > > what about the existent algorithm stability issue? Who's going to > > watch the implementation of __netdev_pick_tx() and make sure there > > is no semantic change? I'm still of the opinion that the > > implementation should be kept within netback so that we can > > guarantee stability, even if it means duplication. Either that or we > > need some guarantee that the semantic of __netdev_pick_tx() will > > never change. > > Please stop talking like this. > > If the default changes it's probably to fix a bug or make things > better, you want no bugs to get fix if it breaks "stability"? To some extent, yes. My hypothesis is that the frontend and the backend agree a steering algorithm so that they can ensure all rx and tx traffic for a flow hits a particular CPU, so that locking can be avoided. If the backend then changes its algorithm then packets may arrive on the wrong queue, hit the wrong CPU and now you have a frontend that may try to access critical data structures without locking from two different CPUs. That's going to be a pretty subtle crash to have to debug. Paul -- 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/06/14 10:20, Paul Durrant wrote: >> -----Original Message----- >> From: David Miller [mailto:davem@davemloft.net] >> Sent: 23 June 2014 10:09 >> To: Paul Durrant >> Cc: Wei Liu; xen-devel@lists.xen.org; netdev@vger.kernel.org; >> boris.ostrovsky@oracle.com; Ian Campbell >> Subject: Re: [PATCH net] xen-netback: bookkeep number of queues in our >> own module >> >> From: Paul Durrant <Paul.Durrant@citrix.com> >> Date: Mon, 23 Jun 2014 07:59:16 +0000 >> >>> Bear in mind that the original intention of the multi-queue patches >>> was to allow the queue selection algorithm to be negotiated with the >>> frontend (see >>> http://lists.xen.org/archives/html/xen-devel/2013-06/msg02654.html). >> Particularly, >>> if the frontend is Windows then netback will need to use a Toeplitz >>> hash to steer traffic since this is stipulated by Microsoft's RSS >>> (Receive Side Scaling) interfaces. So, IMO netback should always >>> implement a select queue method, otherwise any (theoretical) >>> algorithm change in __netdev_pick_tx() would be immediately imposed >>> on frontends, possibly causing them to misbehave. >> >> I do not think you are obligated to use Toeplitz or whatever scheme >> the other side wants, especially if the user on the xen-netback side >> asked for XPS or similar to be used. > > If the frontend can only cope with one steering algorithm then how's > that going to work? Flows would come in on the 'wrong' queue as far as > the frontend is concerned and then who knows what would happen? If the > frontend says it can only handle 'algorithm X' then the backend should > either guarantee it's going to use 'algorthm X' or it should use a > single queue. We specify that frontends have to handle packets from any flow arriving on any queue and and given the absence of any hashing negotiation we should just use the default queue selection. 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 4dd7c4a..93602a6 100644 --- a/drivers/net/xen-netback/common.h +++ b/drivers/net/xen-netback/common.h @@ -222,6 +222,7 @@ struct xenvif { /* Queues */ struct xenvif_queue *queues; + unsigned int num_queues; /* Miscellaneous private stuff. */ struct net_device *dev; diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c index 852da34..7daf64c 100644 --- a/drivers/net/xen-netback/interface.c +++ b/drivers/net/xen-netback/interface.c @@ -140,7 +140,8 @@ static void xenvif_wake_queue_callback(unsigned long data) static u16 xenvif_select_queue(struct net_device *dev, struct sk_buff *skb, void *accel_priv, select_queue_fallback_t fallback) { - unsigned int num_queues = dev->real_num_tx_queues; + struct xenvif *vif = netdev_priv(dev); + unsigned int num_queues = vif->num_queues; u32 hash; u16 queue_index; @@ -162,7 +163,7 @@ static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev) { struct xenvif *vif = netdev_priv(dev); struct xenvif_queue *queue = NULL; - unsigned int num_queues = dev->real_num_tx_queues; + unsigned int num_queues = vif->num_queues; u16 index; int min_slots_needed; @@ -225,7 +226,7 @@ static struct net_device_stats *xenvif_get_stats(struct net_device *dev) { struct xenvif *vif = netdev_priv(dev); struct xenvif_queue *queue = NULL; - unsigned int num_queues = dev->real_num_tx_queues; + unsigned int num_queues = vif->num_queues; unsigned long rx_bytes = 0; unsigned long rx_packets = 0; unsigned long tx_bytes = 0; @@ -256,7 +257,7 @@ out: static void xenvif_up(struct xenvif *vif) { struct xenvif_queue *queue = NULL; - unsigned int num_queues = vif->dev->real_num_tx_queues; + unsigned int num_queues = vif->num_queues; unsigned int queue_index; for (queue_index = 0; queue_index < num_queues; ++queue_index) { @@ -272,7 +273,7 @@ static void xenvif_up(struct xenvif *vif) static void xenvif_down(struct xenvif *vif) { struct xenvif_queue *queue = NULL; - unsigned int num_queues = vif->dev->real_num_tx_queues; + unsigned int num_queues = vif->num_queues; unsigned int queue_index; for (queue_index = 0; queue_index < num_queues; ++queue_index) { @@ -379,7 +380,7 @@ static void xenvif_get_ethtool_stats(struct net_device *dev, struct ethtool_stats *stats, u64 * data) { struct xenvif *vif = netdev_priv(dev); - unsigned int num_queues = dev->real_num_tx_queues; + unsigned int num_queues = vif->num_queues; int i; unsigned int queue_index; struct xenvif_stats *vif_stats; @@ -436,10 +437,7 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid, char name[IFNAMSIZ] = {}; snprintf(name, IFNAMSIZ - 1, "vif%u.%u", domid, handle); - /* Allocate a netdev with the max. supported number of queues. - * When the guest selects the desired number, it will be updated - * via netif_set_real_num_tx_queues(). - */ + /* Allocate a netdev with the max. supported number of queues. */ dev = alloc_netdev_mq(sizeof(struct xenvif), name, ether_setup, xenvif_max_queues); if (dev == NULL) { @@ -458,11 +456,9 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid, vif->dev = dev; vif->disabled = false; - /* Start out with no queues. The call below does not require - * rtnl_lock() as it happens before register_netdev(). - */ + /* Start out with no queues. */ vif->queues = NULL; - netif_set_real_num_tx_queues(dev, 0); + vif->num_queues = 0; dev->netdev_ops = &xenvif_netdev_ops; dev->hw_features = NETIF_F_SG | @@ -677,7 +673,7 @@ static void xenvif_wait_unmap_timeout(struct xenvif_queue *queue, void xenvif_disconnect(struct xenvif *vif) { struct xenvif_queue *queue = NULL; - unsigned int num_queues = vif->dev->real_num_tx_queues; + unsigned int num_queues = vif->num_queues; unsigned int queue_index; if (netif_carrier_ok(vif->dev)) @@ -724,7 +720,7 @@ void xenvif_deinit_queue(struct xenvif_queue *queue) void xenvif_free(struct xenvif *vif) { struct xenvif_queue *queue = NULL; - unsigned int num_queues = vif->dev->real_num_tx_queues; + unsigned int num_queues = vif->num_queues; unsigned int queue_index; /* Here we want to avoid timeout messages if an skb can be legitimately * stuck somewhere else. Realistically this could be an another vif's @@ -751,9 +747,9 @@ void xenvif_free(struct xenvif *vif) /* Free the array of queues. The call below does not require * rtnl_lock() because it happens after unregister_netdev(). */ - netif_set_real_num_tx_queues(vif->dev, 0); vfree(vif->queues); vif->queues = NULL; + vif->num_queues = 0; free_netdev(vif->dev); diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c index 96c63dc2..c724538 100644 --- a/drivers/net/xen-netback/xenbus.c +++ b/drivers/net/xen-netback/xenbus.c @@ -527,9 +527,7 @@ static void connect(struct backend_info *be) /* Use the number of queues requested by the frontend */ be->vif->queues = vzalloc(requested_num_queues * sizeof(struct xenvif_queue)); - rtnl_lock(); - netif_set_real_num_tx_queues(be->vif->dev, requested_num_queues); - rtnl_unlock(); + be->vif->num_queues = requested_num_queues; for (queue_index = 0; queue_index < requested_num_queues; ++queue_index) { queue = &be->vif->queues[queue_index]; @@ -546,9 +544,7 @@ static void connect(struct backend_info *be) * earlier queues can be destroyed using the regular * disconnect logic. */ - rtnl_lock(); - netif_set_real_num_tx_queues(be->vif->dev, queue_index); - rtnl_unlock(); + be->vif->num_queues = queue_index; goto err; } @@ -561,9 +557,7 @@ static void connect(struct backend_info *be) * and also clean up any previously initialised queues. */ xenvif_deinit_queue(queue); - rtnl_lock(); - netif_set_real_num_tx_queues(be->vif->dev, queue_index); - rtnl_unlock(); + be->vif->num_queues = queue_index; goto err; } } @@ -582,13 +576,11 @@ static void connect(struct backend_info *be) return; err: - if (be->vif->dev->real_num_tx_queues > 0) + if (be->vif->num_queues > 0) xenvif_disconnect(be->vif); /* Clean up existing queues */ vfree(be->vif->queues); be->vif->queues = NULL; - rtnl_lock(); - netif_set_real_num_tx_queues(be->vif->dev, 0); - rtnl_unlock(); + be->vif->num_queues = 0; return; } @@ -596,7 +588,7 @@ err: static int connect_rings(struct backend_info *be, struct xenvif_queue *queue) { struct xenbus_device *dev = be->dev; - unsigned int num_queues = queue->vif->dev->real_num_tx_queues; + unsigned int num_queues = queue->vif->num_queues; unsigned long tx_ring_ref, rx_ring_ref; unsigned int tx_evtchn, rx_evtchn; int err;
The original code uses netdev->real_num_tx_queues to bookkeep number of queues and invokes netif_set_real_num_tx_queues to set the number of queues. However, netif_set_real_num_tx_queues doesn't allow real_num_tx_queues to be smaller than 1, which means setting the number to 0 will not work and real_num_tx_queues is untouched. This is bogus when xenvif_free is invoked before any number of queues is allocated. That function needs to iterate through all queues to free resources. Using the wrong number of queues results in NULL pointer dereference. So we bookkeep the number of queues in xen-netback to solve this problem. The usage of real_num_tx_queues in core driver is to cap queue index to a valid value. In start_xmit we've already guarded against out of range queue index so we should be fine. This fixes a regression introduced by multiqueue patchset in 3.16-rc1. Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Signed-off-by: Wei Liu <wei.liu2@citrix.com> CC: Ian Campbell <ian.campbell@citrix.com> --- drivers/net/xen-netback/common.h | 1 + drivers/net/xen-netback/interface.c | 30 +++++++++++++----------------- drivers/net/xen-netback/xenbus.c | 20 ++++++-------------- 3 files changed, 20 insertions(+), 31 deletions(-)