From 6ee57bcc1e61d39c0579438055bc84087210f9b6 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 16 Oct 2012 23:56:14 +1030 Subject: virtio-net: correct capacity math on ring full Capacity math on ring full is wrong: we are looking at num_sg but that might be optimistic because of indirect buffer use. The implementation also penalizes fast path with extra memory accesses for the benefit of ring full condition handling which is slow path. It's easy to query ring capacity so let's do just that. This change also makes it easier to move vnet header for tx around as follow-up patch does. Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell Acked-by: Michael S. Tsirkin --- drivers/net/virtio_net.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'drivers/net') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index cbf8b062535..3db65867895 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -557,10 +557,10 @@ again: return received; } -static unsigned int free_old_xmit_skbs(struct virtnet_info *vi) +static void free_old_xmit_skbs(struct virtnet_info *vi) { struct sk_buff *skb; - unsigned int len, tot_sgs = 0; + unsigned int len; struct virtnet_stats *stats = this_cpu_ptr(vi->stats); while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) { @@ -571,10 +571,8 @@ static unsigned int free_old_xmit_skbs(struct virtnet_info *vi) stats->tx_packets++; u64_stats_update_end(&stats->tx_syncp); - tot_sgs += skb_vnet_hdr(skb)->num_sg; dev_kfree_skb_any(skb); } - return tot_sgs; } static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb) @@ -664,7 +662,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) netif_stop_queue(dev); if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) { /* More just got used, free them then recheck. */ - capacity += free_old_xmit_skbs(vi); + free_old_xmit_skbs(vi); + capacity = vi->svq->num_free; if (capacity >= 2+MAX_SKB_FRAGS) { netif_start_queue(dev); virtqueue_disable_cb(vi->svq); -- cgit v1.2.3-70-g09d2 From 7bedc7dc7c05e6072dc81da770f70c683c45da10 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 16 Oct 2012 23:56:14 +1030 Subject: virtio-net: remove unused skb_vnet_hdr->num_sg field [Split from "correct capacity math on ring full" -- Rusty] Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell Acked-by: Michael S. Tsirkin --- drivers/net/virtio_net.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/net') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 3db65867895..6c094c88424 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -95,7 +95,6 @@ struct skb_vnet_hdr { struct virtio_net_hdr hdr; struct virtio_net_hdr_mrg_rxbuf mhdr; }; - unsigned int num_sg; }; struct padded_vnet_hdr { @@ -579,6 +578,7 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb) { struct skb_vnet_hdr *hdr = skb_vnet_hdr(skb); const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest; + unsigned num_sg; pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest); @@ -617,8 +617,8 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb) else sg_set_buf(vi->tx_sg, &hdr->hdr, sizeof hdr->hdr); - hdr->num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1; - return virtqueue_add_buf(vi->svq, vi->tx_sg, hdr->num_sg, + num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1; + return virtqueue_add_buf(vi->svq, vi->tx_sg, num_sg, 0, skb, GFP_ATOMIC); } -- cgit v1.2.3-70-g09d2 From 9ed4cb073438e2154778f0d693d966359afd6549 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 16 Oct 2012 23:56:14 +1030 Subject: virtio_net: don't rely on virtqueue_add_buf() returning capacity. Now we can easily use vq->num_free to determine if there are descriptors left in the queue, we're about to change virtqueue_add_buf() to return 0 on success. The virtio_net driver is the only one which actually uses the return value, so change that. Signed-off-by: Rusty Russell Acked-by: Michael S. Tsirkin --- drivers/net/virtio_net.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) (limited to 'drivers/net') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 6c094c88424..7c7f5a94ca4 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -471,10 +471,11 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp) err = add_recvbuf_small(vi, gfp); oom = err == -ENOMEM; - if (err < 0) + if (err) break; ++vi->num; - } while (err > 0); + } while (vi->rvq->num_free); + if (unlikely(vi->num > vi->max)) vi->max = vi->num; virtqueue_kick(vi->rvq); @@ -625,27 +626,20 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb) static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) { struct virtnet_info *vi = netdev_priv(dev); - int capacity; + int err; /* Free up any pending old buffers before queueing new ones. */ free_old_xmit_skbs(vi); /* Try to transmit */ - capacity = xmit_skb(vi, skb); + err = xmit_skb(vi, skb); - /* This can happen with OOM and indirect buffers. */ - if (unlikely(capacity < 0)) { - if (likely(capacity == -ENOMEM)) { - if (net_ratelimit()) - dev_warn(&dev->dev, - "TX queue failure: out of memory\n"); - } else { - dev->stats.tx_fifo_errors++; - if (net_ratelimit()) - dev_warn(&dev->dev, - "Unexpected TX queue failure: %d\n", - capacity); - } + /* This should not happen! */ + if (unlikely(err < 0)) { + dev->stats.tx_fifo_errors++; + if (net_ratelimit()) + dev_warn(&dev->dev, + "Unexpected TX queue failure: %d\n", err); dev->stats.tx_dropped++; kfree_skb(skb); return NETDEV_TX_OK; @@ -658,13 +652,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) /* Apparently nice girls don't return TX_BUSY; stop the queue * before it gets out of hand. Naturally, this wastes entries. */ - if (capacity < 2+MAX_SKB_FRAGS) { + if (vi->svq->num_free < 2+MAX_SKB_FRAGS) { netif_stop_queue(dev); if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) { /* More just got used, free them then recheck. */ free_old_xmit_skbs(vi); - capacity = vi->svq->num_free; - if (capacity >= 2+MAX_SKB_FRAGS) { + if (vi->svq->num_free >= 2+MAX_SKB_FRAGS) { netif_start_queue(dev); virtqueue_disable_cb(vi->svq); } -- cgit v1.2.3-70-g09d2 From 0e3daa6491699640b9efc321d61310aee9a809d5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 16 Oct 2012 23:56:15 +1030 Subject: virtio: net: make it clear that virtqueue_add_buf() no longer returns > 0 We simplified virtqueue_add_buf(), make it clear in the callers. Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/net') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 7c7f5a94ca4..62898910708 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -635,7 +635,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) err = xmit_skb(vi, skb); /* This should not happen! */ - if (unlikely(err < 0)) { + if (unlikely(err)) { dev->stats.tx_fifo_errors++; if (net_ratelimit()) dev_warn(&dev->dev, -- cgit v1.2.3-70-g09d2