Re: [PATCH V6 net-next 2/5] xen-netback: Add support for multiple queues

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2014-03-18 at 10:48 +0000, Andrew Bennieston wrote:
[snip... queeues...]

All sounds fine, thanks for the explanations.

> >>   	xen_net_read_rate(dev, &credit_bytes, &credit_usec);
> >>   	read_xenbus_vif_flags(be);
> >>
> >> -	be->vif->num_queues = 1;
> >> +	/* Use the number of queues requested by the frontend */
> >> +	be->vif->num_queues = requested_num_queues;
> >>   	be->vif->queues = vzalloc(be->vif->num_queues *
> >>   			sizeof(struct xenvif_queue));
> >> +	rtnl_lock();
> >> +	netif_set_real_num_tx_queues(be->vif->dev, be->vif->num_queues);
> >> +	rtnl_unlock();
> >
> > I'm always a bit suspicious of this construct -- it makes me thing the
> > call is happening from the wrong context and that the right context
> > would naturally hold the lock already.
> 
> netif_set_real_num_tx_queues() must be called either with this lock
> held, or before the netdev is registered. The netdev is registered early
> so that it can be plugged into a bridge or whatever other network
> configuration has to happen. The point at which we know the correct
> number of tx queues happens in response to the frontend changing
> Xenstore entries, so the rtnl lock is not naturally held here.
> xenvif_carrier_on() and xenvif_carrier_off() also take this lock, but
> they are not the appropriate place to set the number of queues.

Great, just wanted to be sure it had been thought about and not just
"slap a lock around it to make it stop complaining" :-)

> >>   	for (queue_index = 0; queue_index < be->vif->num_queues; ++queue_index) {
> >>   		queue = &be->vif->queues[queue_index];
> >> @@ -547,29 +575,52 @@ static int connect_rings(struct backend_info *be, struct xenvif_queue *queue)
> >>   	unsigned long tx_ring_ref, rx_ring_ref;
> >>   	unsigned int tx_evtchn, rx_evtchn;
> >>   	int err;
> >> +	char *xspath;
> >> +	size_t xspathsize;
> >> +	const size_t xenstore_path_ext_size = 11; /* sufficient for "/queue-NNN" */
> >> +
> >> +	/* If the frontend requested 1 queue, or we have fallen back
> >> +	 * to single queue due to lack of frontend support for multi-
> >> +	 * queue, expect the remaining XenStore keys in the toplevel
> >> +	 * directory. Otherwise, expect them in a subdirectory called
> >> +	 * queue-N.
> >> +	 */
> >> +	if (queue->vif->num_queues == 1) {
> >> +		xspath = (char *)dev->otherend;
> >
> > Casting away a const is naughty. Either make xspath const or if that
> > isn't possible make it dynamic in all cases with a strcpy in this
> > degenerate case.
> >
> 
> Ok, I can change this. I was trying to avoid the strcpy.

Thanks.

Ian.