RE: [PATCH net] xen-netback: bookkeep number of queues in our own module

-----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:

quoted

From: Wei Liu <redacted>
Date: Wed, 18 Jun 2014 15:09:18 +0100

quoted

The original code uses netdev->real_num_tx_queues to bookkeep

number of

quoted

quoted

queues and invokes netif_set_real_num_tx_queues to set the number

of

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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 <redacted>

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
 

quoted

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.

quoted

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