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
