Re: [PATCH 2/4] [RFC rev2] virtio-net changes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Rusty,

Thanks for your feedback. I agree with all the changes, and will
make it and resubmit next.

thanks,

- KK

Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote on 04/13/2011 06:58:02 AM:

> Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> 04/13/2011 06:58 AM
>
> To
>
> Krishna Kumar2/India/IBM@IBMIN, davem@xxxxxxxxxxxxx, mst@xxxxxxxxxx
>
> cc
>
> eric.dumazet@xxxxxxxxx, arnd@xxxxxxxx, netdev@xxxxxxxxxxxxxxx,
> horms@xxxxxxxxxxxx, avi@xxxxxxxxxx, anthony@xxxxxxxxxxxxx,
> kvm@xxxxxxxxxxxxxxx, Krishna Kumar2/India/IBM@IBMIN
>
> Subject
>
> Re: [PATCH 2/4] [RFC rev2] virtio-net changes
>
> On Tue, 05 Apr 2011 20:38:52 +0530, Krishna Kumar <krkumar2@xxxxxxxxxx>
wrote:
> > Implement mq virtio-net driver.
> >
> > Though struct virtio_net_config changes, it works with the old
> > qemu since the last element is not accessed unless qemu sets
> > VIRTIO_NET_F_MULTIQUEUE.
> >
> > Signed-off-by: Krishna Kumar <krkumar2@xxxxxxxxxx>
>
> Hi Krishna!
>
>     This change looks fairly solid, but I'd prefer it split into a few
> stages for clarity.
>
> The first patch should extract out the struct send_queue and struct
> receive_queue, even though there's still only one.  The second patch
> can then introduce VIRTIO_NET_F_MULTIQUEUE.
>
> You could split into more parts if that makes sense, but I'd prefer to
> see the mechanical changes separate from the feature addition.
>
> > -struct virtnet_info {
> > -   struct virtio_device *vdev;
> > -   struct virtqueue *rvq, *svq, *cvq;
> > -   struct net_device *dev;
> > +/* Internal representation of a send virtqueue */
> > +struct send_queue {
> > +   /* Virtqueue associated with this send _queue */
> > +   struct virtqueue *svq;
>
> You can simply call this vq now it's inside 'send_queue'.
>
> > +
> > +   /* TX: fragments + linear part + virtio header */
> > +   struct scatterlist tx_sg[MAX_SKB_FRAGS + 2];
>
> Similarly, this can just be sg.
>
> > +static void free_receive_bufs(struct virtnet_info *vi)
> > +{
> > +   int i;
> > +
> > +   for (i = 0; i < vi->numtxqs; i++) {
> > +      BUG_ON(vi->rq[i] == NULL);
> > +      while (vi->rq[i]->pages)
> > +         __free_pages(get_a_page(vi->rq[i], GFP_KERNEL), 0);
> > +   }
> > +}
>
> You can skip the BUG_ON(), since the next line will have the same effect.
>
> > +/* Free memory allocated for send and receive queues */
> > +static void free_rq_sq(struct virtnet_info *vi)
> > +{
> > +   int i;
> > +
> > +   if (vi->rq) {
> > +      for (i = 0; i < vi->numtxqs; i++)
> > +         kfree(vi->rq[i]);
> > +      kfree(vi->rq);
> > +   }
> > +
> > +   if (vi->sq) {
> > +      for (i = 0; i < vi->numtxqs; i++)
> > +         kfree(vi->sq[i]);
> > +      kfree(vi->sq);
> > +   }
>
> This looks weird, even though it's correct.
>
> I think we need a better name than numtxqs and shorter than
> num_queue_pairs.  Let's just use num_queues; sure, there are both tx and
> rq queues, but I still think it's pretty clear.
>
> > +   for (i = 0; i < vi->numtxqs; i++) {
> > +      struct virtqueue *svq = vi->sq[i]->svq;
> > +
> > +      while (1) {
> > +         buf = virtqueue_detach_unused_buf(svq);
> > +         if (!buf)
> > +            break;
> > +         dev_kfree_skb(buf);
> > +      }
> > +   }
>
> I know this isn't your code, but it's ugly :)
>
>         while ((buf = virtqueue_detach_unused_buf(svq)) != NULL)
>                 dev_kfree_skb(buf);
>
> > +   for (i = 0; i < vi->numtxqs; i++) {
> > +      struct virtqueue *rvq = vi->rq[i]->rvq;
> > +
> > +      while (1) {
> > +         buf = virtqueue_detach_unused_buf(rvq);
> > +         if (!buf)
> > +            break;
>
> Here too...
>
> > +#define MAX_DEVICE_NAME      16
>
> This isn't a good idea, see below.
>
> > +static int initialize_vqs(struct virtnet_info *vi, int numtxqs)
> > +{
> > +   vq_callback_t **callbacks;
> > +   struct virtqueue **vqs;
> > +   int i, err = -ENOMEM;
> > +   int totalvqs;
> > +   char **names;
>
> This whole routine is really messy.  How about doing find_vqs first,
> then have routines like setup_rxq(), setup_txq() and setup_controlq()
> would make this neater:
>
>         static int setup_rxq(struct send_queue *sq, char *name);
>
> Also, use kasprintf() instead of kmalloc & sprintf.
>
> > +#if 1
> > +   /* Allocate/initialize parameters for recv/send virtqueues */
>
> Why is this #if 1'd?
>
> I do prefer the #else method of doing two loops, myself (but use
> kasprintf).
>
> Cheers,
> Rusty.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux