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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel