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

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]

Date:	Mon, 23 Jun 2014 09:18:23 +0000
From:	Paul Durrant <Paul.Durrant@...rix.com>
To:	Wei Liu <wei.liu2@...rix.com>
CC:	Wei Liu <wei.liu2@...rix.com>, David Miller <davem@...emloft.net>,
	"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"boris.ostrovsky@...cle.com" <boris.ostrovsky@...cle.com>,
	Ian Campbell <Ian.Campbell@...rix.com>
Subject: RE: [PATCH net] xen-netback: bookkeep number of queues in our own
 module

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@...rix.com]
> Sent: 23 June 2014 09:49
> To: Paul Durrant
> Cc: Wei Liu; David Miller; xen-devel@...ts.xen.org; netdev@...r.kernel.org;
> boris.ostrovsky@...cle.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@...cle.com>
> > > > > Signed-off-by: Wei Liu <wei.liu2@...rix.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@...r.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html