Too many places poke at [rs]q->vq->vdev->priv just to get
the the vi structure. Let's just pass the pointer around: seems
cleaner, and might even be faster.
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
drivers/net/virtio_net.c | 36 +++++++++++++++++++-----------------
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 57cbc7d..36f3dfc 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -241,11 +241,11 @@ static unsigned long mergeable_buf_to_ctx(void *buf,
unsigned int truesize)
}
/* Called from bottom half context */
-static struct sk_buff *page_to_skb(struct receive_queue *rq,
+static struct sk_buff *page_to_skb(struct virtnet_info *vi,
+ struct receive_queue *rq,
struct page *page, unsigned int offset,
unsigned int len, unsigned int truesize)
{
- struct virtnet_info *vi = rq->vq->vdev->priv;
struct sk_buff *skb;
struct skb_vnet_hdr *hdr;
unsigned int copy, hdr_len, hdr_padded_len;
@@ -328,12 +328,13 @@ static struct sk_buff *receive_small(void *buf, unsigned
int len)
}
static struct sk_buff *receive_big(struct net_device *dev,
+ struct virtnet_info *vi,
struct receive_queue *rq,
void *buf,
unsigned int len)
{
struct page *page = buf;
- struct sk_buff *skb = page_to_skb(rq, page, 0, len, PAGE_SIZE);
+ struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE);
if (unlikely(!skb))
goto err;
@@ -359,7 +360,8 @@ static struct sk_buff *receive_mergeable(struct net_device
*dev,
int offset = buf - page_address(page);
unsigned int truesize = max(len, mergeable_ctx_to_buf_truesize(ctx));
- struct sk_buff *head_skb = page_to_skb(rq, page, offset, len, truesize);
+ struct sk_buff *head_skb = page_to_skb(vi, rq, page, offset, len,
+ truesize);
struct sk_buff *curr_skb = head_skb;
if (unlikely(!curr_skb))
@@ -433,9 +435,9 @@ err_buf:
return NULL;
}
-static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
+static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
+ void *buf, unsigned int len)
{
- struct virtnet_info *vi = rq->vq->vdev->priv;
struct net_device *dev = vi->dev;
struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
struct sk_buff *skb;
@@ -459,9 +461,9 @@ static void receive_buf(struct receive_queue *rq, void *buf,
unsigned int len)
if (vi->mergeable_rx_bufs)
skb = receive_mergeable(dev, vi, rq, (unsigned long)buf, len);
else if (vi->big_packets)
- skb = receive_big(dev, rq, buf, len);
+ skb = receive_big(dev, vi, rq, buf, len);
else
- skb = receive_small(buf, len);
+ skb = receive_small(vi, buf, len);
if (unlikely(!skb))
return;
@@ -530,9 +532,9 @@ frame_err:
dev_kfree_skb(skb);
}
-static int add_recvbuf_small(struct receive_queue *rq, gfp_t gfp)
+static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
+ gfp_t gfp)
{
- struct virtnet_info *vi = rq->vq->vdev->priv;
struct sk_buff *skb;
struct skb_vnet_hdr *hdr;
int err;
@@ -655,9 +657,9 @@ static int add_recvbuf_mergeable(struct receive_queue *rq,
gfp_t gfp)
* before we're receiving packets, or from refill_work which is
* careful to disable receiving (using napi_disable).
*/
-static bool try_fill_recv(struct receive_queue *rq, gfp_t gfp)
+static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
+ gfp_t gfp)
{
- struct virtnet_info *vi = rq->vq->vdev->priv;
int err;
bool oom;
@@ -668,7 +670,7 @@ static bool try_fill_recv(struct receive_queue *rq, gfp_t
gfp)
else if (vi->big_packets)
err = add_recvbuf_big(rq, gfp);
else
- err = add_recvbuf_small(rq, gfp);
+ err = add_recvbuf_small(vi, rq, gfp);
oom = err == -ENOMEM;
if (err)
@@ -717,7 +719,7 @@ static void refill_work(struct work_struct *work)
struct receive_queue *rq = &vi->rq[i];
napi_disable(&rq->napi);
- still_empty = !try_fill_recv(rq, GFP_KERNEL);
+ still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
virtnet_napi_enable(rq);
/* In theory, this can happen: if we don't get any buffers in
@@ -736,12 +738,12 @@ static int virtnet_receive(struct receive_queue *rq, int
budget)
while (received < budget &&
(buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
- receive_buf(rq, buf, len);
+ receive_buf(vi, rq, buf, len);
received++;
}
if (rq->vq->num_free > virtqueue_get_vring_size(rq->vq) / 2) {
- if (!try_fill_recv(rq, GFP_ATOMIC))
+ if (!try_fill_recv(vi, rq, GFP_ATOMIC))
schedule_delayed_work(&vi->refill, 0);
}
@@ -817,7 +819,7 @@ static int virtnet_open(struct net_device *dev)
for (i = 0; i < vi->max_queue_pairs; i++) {
if (i < vi->curr_queue_pairs)
/* Make sure we have some buffers: if oom use wq. */
- if (!try_fill_recv(&vi->rq[i], GFP_KERNEL))
+ if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
schedule_delayed_work(&vi->refill, 0);
virtnet_napi_enable(&vi->rq[i]);
}
--
MST
Michael S. Tsirkin
2014-Oct-23 21:28 UTC
[PATCH RFC 2/4] virtio_net: get rid of virtio_net_hdr/skb_vnet_hdr
virtio 1.0 doesn't use virtio_net_hdr anymore, and in fact, it's not
really useful since virtio_net_hdr_mrg_rxbuf includes that as the first
field anyway.
Let's drop it, precalculate header len and store within vi instead.
This way we can also remove struct skb_vnet_hdr.
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
drivers/net/virtio_net.c | 88 ++++++++++++++++++++++--------------------------
1 file changed, 40 insertions(+), 48 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 36f3dfc..a795a23 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -123,6 +123,9 @@ struct virtnet_info {
/* Host can handle any s/g split between our header and packet data */
bool any_header_sg;
+ /* Packet virtio header size */
+ u8 hdr_len;
+
/* Active statistics */
struct virtnet_stats __percpu *stats;
@@ -139,21 +142,14 @@ struct virtnet_info {
struct notifier_block nb;
};
-struct skb_vnet_hdr {
- union {
- struct virtio_net_hdr hdr;
- struct virtio_net_hdr_mrg_rxbuf mhdr;
- };
-};
-
struct padded_vnet_hdr {
- struct virtio_net_hdr hdr;
+ struct virtio_net_hdr_mrg_rxbuf hdr;
/*
- * virtio_net_hdr should be in a separated sg buffer because of a
- * QEMU bug, and data sg buffer shares same page with this header sg.
- * This padding makes next sg 16 byte aligned after virtio_net_hdr.
+ * hdr is in a separate sg buffer, and data sg buffer shares same page
+ * with this header sg. This padding makes next sg 16 byte aligned
+ * after the header.
*/
- char padding[6];
+ char padding[4];
};
/* Converting between virtqueue no. and kernel tx/rx queue no.
@@ -179,9 +175,9 @@ static int rxq2vq(int rxq)
return rxq * 2;
}
-static inline struct skb_vnet_hdr *skb_vnet_hdr(struct sk_buff *skb)
+static inline struct virtio_net_hdr_mrg_rxbuf *skb_vnet_hdr(struct sk_buff
*skb)
{
- return (struct skb_vnet_hdr *)skb->cb;
+ return (struct virtio_net_hdr_mrg_rxbuf *)skb->cb;
}
/*
@@ -247,7 +243,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
unsigned int len, unsigned int truesize)
{
struct sk_buff *skb;
- struct skb_vnet_hdr *hdr;
+ struct virtio_net_hdr_mrg_rxbuf *hdr;
unsigned int copy, hdr_len, hdr_padded_len;
char *p;
@@ -260,13 +256,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info
*vi,
hdr = skb_vnet_hdr(skb);
- if (vi->mergeable_rx_bufs) {
- hdr_len = sizeof hdr->mhdr;
- hdr_padded_len = sizeof hdr->mhdr;
- } else {
- hdr_len = sizeof hdr->hdr;
+ hdr_len = vi->hdr_len;
+ if (vi->mergeable_rx_bufs)
+ hdr_padded_len = sizeof *hdr;
+ else
hdr_padded_len = sizeof(struct padded_vnet_hdr);
- }
memcpy(hdr, p, hdr_len);
@@ -317,11 +311,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info
*vi,
return skb;
}
-static struct sk_buff *receive_small(void *buf, unsigned int len)
+static struct sk_buff *receive_small(struct virtnet_info *vi, void *buf,
unsigned int len)
{
struct sk_buff * skb = buf;
- len -= sizeof(struct virtio_net_hdr);
+ len -= vi->hdr_len;
skb_trim(skb, len);
return skb;
@@ -354,8 +348,8 @@ static struct sk_buff *receive_mergeable(struct net_device
*dev,
unsigned int len)
{
void *buf = mergeable_ctx_to_buf_address(ctx);
- struct skb_vnet_hdr *hdr = buf;
- u16 num_buf = virtio16_to_cpu(rq->vq->vdev, hdr->mhdr.num_buffers);
+ struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
+ u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
struct page *page = virt_to_head_page(buf);
int offset = buf - page_address(page);
unsigned int truesize = max(len, mergeable_ctx_to_buf_truesize(ctx));
@@ -373,8 +367,8 @@ static struct sk_buff *receive_mergeable(struct net_device
*dev,
if (unlikely(!ctx)) {
pr_debug("%s: rx error: %d buffers out of %d missing\n",
dev->name, num_buf,
- virtio16_to_cpu(rq->vq->vdev,
- hdr->mhdr.num_buffers));
+ virtio16_to_cpu(vi->vdev,
+ hdr->num_buffers));
dev->stats.rx_length_errors++;
goto err_buf;
}
@@ -441,7 +435,7 @@ static void receive_buf(struct virtnet_info *vi, struct
receive_queue *rq,
struct net_device *dev = vi->dev;
struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
struct sk_buff *skb;
- struct skb_vnet_hdr *hdr;
+ struct virtio_net_hdr_mrg_rxbuf *hdr;
if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
pr_debug("%s: short packet %i\n", dev->name, len);
@@ -536,7 +530,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct
receive_queue *rq,
gfp_t gfp)
{
struct sk_buff *skb;
- struct skb_vnet_hdr *hdr;
+ struct virtio_net_hdr_mrg_rxbuf *hdr;
int err;
skb = __netdev_alloc_skb_ip_align(vi->dev, GOOD_PACKET_LEN, gfp);
@@ -547,7 +541,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct
receive_queue *rq,
hdr = skb_vnet_hdr(skb);
sg_init_table(rq->sg, MAX_SKB_FRAGS + 2);
- sg_set_buf(rq->sg, &hdr->hdr, sizeof hdr->hdr);
+ sg_set_buf(rq->sg, hdr, vi->hdr_len);
skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
@@ -557,7 +551,8 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct
receive_queue *rq,
return err;
}
-static int add_recvbuf_big(struct receive_queue *rq, gfp_t gfp)
+static int add_recvbuf_big(struct virtnet_info *vi, struct receive_queue *rq,
+ gfp_t gfp)
{
struct page *first, *list = NULL;
char *p;
@@ -588,8 +583,8 @@ static int add_recvbuf_big(struct receive_queue *rq, gfp_t
gfp)
p = page_address(first);
/* rq->sg[0], rq->sg[1] share the same page */
- /* a separated rq->sg[0] for virtio_net_hdr only due to QEMU bug */
- sg_set_buf(&rq->sg[0], p, sizeof(struct virtio_net_hdr));
+ /* a separated rq->sg[0] for header - required in case !any_header_sg */
+ sg_set_buf(&rq->sg[0], p, vi->hdr_len);
/* rq->sg[1] for data packet, from offset */
offset = sizeof(struct padded_vnet_hdr);
@@ -668,7 +663,7 @@ static bool try_fill_recv(struct virtnet_info *vi, struct
receive_queue *rq,
if (vi->mergeable_rx_bufs)
err = add_recvbuf_mergeable(rq, gfp);
else if (vi->big_packets)
- err = add_recvbuf_big(rq, gfp);
+ err = add_recvbuf_big(vi, rq, gfp);
else
err = add_recvbuf_small(vi, rq, gfp);
@@ -848,18 +843,14 @@ static void free_old_xmit_skbs(struct send_queue *sq)
static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
{
- struct skb_vnet_hdr *hdr;
+ struct virtio_net_hdr_mrg_rxbuf *hdr;
const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
struct virtnet_info *vi = sq->vq->vdev->priv;
unsigned num_sg;
- unsigned hdr_len;
+ unsigned hdr_len = vi->hdr_len;
bool can_push;
pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
- if (vi->mergeable_rx_bufs)
- hdr_len = sizeof hdr->mhdr;
- else
- hdr_len = sizeof hdr->hdr;
can_push = vi->any_header_sg &&
!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
@@ -867,7 +858,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff
*skb)
/* Even if we can, don't push here yet as this would skew
* csum_start offset below. */
if (can_push)
- hdr = (struct skb_vnet_hdr *)(skb->data - hdr_len);
+ hdr = (struct virtio_net_hdr_mrg_rxbuf *)(skb->data - hdr_len);
else
hdr = skb_vnet_hdr(skb);
@@ -902,7 +893,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff
*skb)
}
if (vi->mergeable_rx_bufs)
- hdr->mhdr.num_buffers = 0;
+ hdr->num_buffers = 0;
sg_init_table(sq->sg, MAX_SKB_FRAGS + 2);
if (can_push) {
@@ -1773,18 +1764,19 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
vi->mergeable_rx_bufs = true;
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
+ vi->hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
+ else
+ vi->hdr_len = sizeof(struct virtio_net_hdr);
+
if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT))
vi->any_header_sg = true;
if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
vi->has_cvq = true;
- if (vi->any_header_sg) {
- if (vi->mergeable_rx_bufs)
- dev->needed_headroom = sizeof(struct virtio_net_hdr_mrg_rxbuf);
- else
- dev->needed_headroom = sizeof(struct virtio_net_hdr);
- }
+ if (vi->any_header_sg)
+ dev->needed_headroom = vi->hdr_len;
/* Use single tx/rx queue pair as default */
vi->curr_queue_pairs = 1;
--
MST
Michael S. Tsirkin
2014-Oct-23 21:28 UTC
[PATCH RFC 3/4] virtio_net: stricter short buffer length checks
Our buffer length check is not strict enough for mergeable
buffers: buffer can still be shorter that header + address
by 2 bytes.
Fix that up.
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
drivers/net/virtio_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a795a23..9c6d50f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -437,7 +437,7 @@ static void receive_buf(struct virtnet_info *vi, struct
receive_queue *rq,
struct sk_buff *skb;
struct virtio_net_hdr_mrg_rxbuf *hdr;
- if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
+ if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
pr_debug("%s: short packet %i\n", dev->name, len);
dev->stats.rx_length_errors++;
if (vi->mergeable_rx_bufs) {
--
MST
From: Michael S. Tsirkin> Too many places poke at [rs]q->vq->vdev->priv just to get > the the vi structure. Let's just pass the pointer around: seems > cleaner, and might even be faster. > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > drivers/net/virtio_net.c | 36 +++++++++++++++++++----------------- > 1 file changed, 19 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 57cbc7d..36f3dfc 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c...> static struct sk_buff *receive_big(struct net_device *dev, > + struct virtnet_info *vi,Do you need to pass 'dev' here? Looks like it is obtainable from vi->dev (as below). David> struct receive_queue *rq, > void *buf, > unsigned int len) > { > struct page *page = buf; > - struct sk_buff *skb = page_to_skb(rq, page, 0, len, PAGE_SIZE); > + struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE);...> -static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len) > +static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > + void *buf, unsigned int len) > { > - struct virtnet_info *vi = rq->vq->vdev->priv; > struct net_device *dev = vi->dev;...
On Fri, Oct 24, 2014 at 10:02:15AM +0000, David Laight wrote:> From: Michael S. Tsirkin > > > Too many places poke at [rs]q->vq->vdev->priv just to get > > the the vi structure. Let's just pass the pointer around: seems > > cleaner, and might even be faster. > > > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > --- > > drivers/net/virtio_net.c | 36 +++++++++++++++++++----------------- > > 1 file changed, 19 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 57cbc7d..36f3dfc 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > ... > > static struct sk_buff *receive_big(struct net_device *dev, > > + struct virtnet_info *vi, > > Do you need to pass 'dev' here? > Looks like it is obtainable from vi->dev (as below). > > David > > > struct receive_queue *rq, > > void *buf, > > unsigned int len) > > { > > struct page *page = buf; > > - struct sk_buff *skb = page_to_skb(rq, page, 0, len, PAGE_SIZE); > > + struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE); > ... > > -static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len) > > +static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > + void *buf, unsigned int len) > > { > > - struct virtnet_info *vi = rq->vq->vdev->priv; > > struct net_device *dev = vi->dev; > ...It's a matter of style, isn't it? We have dev to hand, it seems cleaner to just pass it around. -- MST
"Michael S. Tsirkin" <mst at redhat.com> writes:> Too many places poke at [rs]q->vq->vdev->priv just to get > the the vi structure. Let's just pass the pointer around: seems > cleaner, and might even be faster.Agreed, it's neater. Acked-by: Rusty Russell <rusty at rustcorp.com.au> Thanks, Rusty.> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > drivers/net/virtio_net.c | 36 +++++++++++++++++++----------------- > 1 file changed, 19 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 57cbc7d..36f3dfc 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -241,11 +241,11 @@ static unsigned long mergeable_buf_to_ctx(void *buf, unsigned int truesize) > } > > /* Called from bottom half context */ > -static struct sk_buff *page_to_skb(struct receive_queue *rq, > +static struct sk_buff *page_to_skb(struct virtnet_info *vi, > + struct receive_queue *rq, > struct page *page, unsigned int offset, > unsigned int len, unsigned int truesize) > { > - struct virtnet_info *vi = rq->vq->vdev->priv; > struct sk_buff *skb; > struct skb_vnet_hdr *hdr; > unsigned int copy, hdr_len, hdr_padded_len; > @@ -328,12 +328,13 @@ static struct sk_buff *receive_small(void *buf, unsigned int len) > } > > static struct sk_buff *receive_big(struct net_device *dev, > + struct virtnet_info *vi, > struct receive_queue *rq, > void *buf, > unsigned int len) > { > struct page *page = buf; > - struct sk_buff *skb = page_to_skb(rq, page, 0, len, PAGE_SIZE); > + struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE); > > if (unlikely(!skb)) > goto err; > @@ -359,7 +360,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, > int offset = buf - page_address(page); > unsigned int truesize = max(len, mergeable_ctx_to_buf_truesize(ctx)); > > - struct sk_buff *head_skb = page_to_skb(rq, page, offset, len, truesize); > + struct sk_buff *head_skb = page_to_skb(vi, rq, page, offset, len, > + truesize); > struct sk_buff *curr_skb = head_skb; > > if (unlikely(!curr_skb)) > @@ -433,9 +435,9 @@ err_buf: > return NULL; > } > > -static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len) > +static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > + void *buf, unsigned int len) > { > - struct virtnet_info *vi = rq->vq->vdev->priv; > struct net_device *dev = vi->dev; > struct virtnet_stats *stats = this_cpu_ptr(vi->stats); > struct sk_buff *skb; > @@ -459,9 +461,9 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len) > if (vi->mergeable_rx_bufs) > skb = receive_mergeable(dev, vi, rq, (unsigned long)buf, len); > else if (vi->big_packets) > - skb = receive_big(dev, rq, buf, len); > + skb = receive_big(dev, vi, rq, buf, len); > else > - skb = receive_small(buf, len); > + skb = receive_small(vi, buf, len); > > if (unlikely(!skb)) > return; > @@ -530,9 +532,9 @@ frame_err: > dev_kfree_skb(skb); > } > > -static int add_recvbuf_small(struct receive_queue *rq, gfp_t gfp) > +static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq, > + gfp_t gfp) > { > - struct virtnet_info *vi = rq->vq->vdev->priv; > struct sk_buff *skb; > struct skb_vnet_hdr *hdr; > int err; > @@ -655,9 +657,9 @@ static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp) > * before we're receiving packets, or from refill_work which is > * careful to disable receiving (using napi_disable). > */ > -static bool try_fill_recv(struct receive_queue *rq, gfp_t gfp) > +static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq, > + gfp_t gfp) > { > - struct virtnet_info *vi = rq->vq->vdev->priv; > int err; > bool oom; > > @@ -668,7 +670,7 @@ static bool try_fill_recv(struct receive_queue *rq, gfp_t gfp) > else if (vi->big_packets) > err = add_recvbuf_big(rq, gfp); > else > - err = add_recvbuf_small(rq, gfp); > + err = add_recvbuf_small(vi, rq, gfp); > > oom = err == -ENOMEM; > if (err) > @@ -717,7 +719,7 @@ static void refill_work(struct work_struct *work) > struct receive_queue *rq = &vi->rq[i]; > > napi_disable(&rq->napi); > - still_empty = !try_fill_recv(rq, GFP_KERNEL); > + still_empty = !try_fill_recv(vi, rq, GFP_KERNEL); > virtnet_napi_enable(rq); > > /* In theory, this can happen: if we don't get any buffers in > @@ -736,12 +738,12 @@ static int virtnet_receive(struct receive_queue *rq, int budget) > > while (received < budget && > (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) { > - receive_buf(rq, buf, len); > + receive_buf(vi, rq, buf, len); > received++; > } > > if (rq->vq->num_free > virtqueue_get_vring_size(rq->vq) / 2) { > - if (!try_fill_recv(rq, GFP_ATOMIC)) > + if (!try_fill_recv(vi, rq, GFP_ATOMIC)) > schedule_delayed_work(&vi->refill, 0); > } > > @@ -817,7 +819,7 @@ static int virtnet_open(struct net_device *dev) > for (i = 0; i < vi->max_queue_pairs; i++) { > if (i < vi->curr_queue_pairs) > /* Make sure we have some buffers: if oom use wq. */ > - if (!try_fill_recv(&vi->rq[i], GFP_KERNEL)) > + if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL)) > schedule_delayed_work(&vi->refill, 0); > virtnet_napi_enable(&vi->rq[i]); > } > -- > MST
Rusty Russell
2014-Oct-28 00:27 UTC
[PATCH RFC 2/4] virtio_net: get rid of virtio_net_hdr/skb_vnet_hdr
"Michael S. Tsirkin" <mst at redhat.com> writes:> virtio 1.0 doesn't use virtio_net_hdr anymore, and in fact, it's not > really useful since virtio_net_hdr_mrg_rxbuf includes that as the first > field anyway. > > Let's drop it, precalculate header len and store within vi instead. > > This way we can also remove struct skb_vnet_hdr.Yes, this is definitely a win. Acked-by: Rusty Russell <rusty at rustcorp.com.au> Thanks, Rusty.> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > drivers/net/virtio_net.c | 88 ++++++++++++++++++++++-------------------------- > 1 file changed, 40 insertions(+), 48 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 36f3dfc..a795a23 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -123,6 +123,9 @@ struct virtnet_info { > /* Host can handle any s/g split between our header and packet data */ > bool any_header_sg; > > + /* Packet virtio header size */ > + u8 hdr_len; > + > /* Active statistics */ > struct virtnet_stats __percpu *stats; > > @@ -139,21 +142,14 @@ struct virtnet_info { > struct notifier_block nb; > }; > > -struct skb_vnet_hdr { > - union { > - struct virtio_net_hdr hdr; > - struct virtio_net_hdr_mrg_rxbuf mhdr; > - }; > -}; > - > struct padded_vnet_hdr { > - struct virtio_net_hdr hdr; > + struct virtio_net_hdr_mrg_rxbuf hdr; > /* > - * virtio_net_hdr should be in a separated sg buffer because of a > - * QEMU bug, and data sg buffer shares same page with this header sg. > - * This padding makes next sg 16 byte aligned after virtio_net_hdr. > + * hdr is in a separate sg buffer, and data sg buffer shares same page > + * with this header sg. This padding makes next sg 16 byte aligned > + * after the header. > */ > - char padding[6]; > + char padding[4]; > }; > > /* Converting between virtqueue no. and kernel tx/rx queue no. > @@ -179,9 +175,9 @@ static int rxq2vq(int rxq) > return rxq * 2; > } > > -static inline struct skb_vnet_hdr *skb_vnet_hdr(struct sk_buff *skb) > +static inline struct virtio_net_hdr_mrg_rxbuf *skb_vnet_hdr(struct sk_buff *skb) > { > - return (struct skb_vnet_hdr *)skb->cb; > + return (struct virtio_net_hdr_mrg_rxbuf *)skb->cb; > } > > /* > @@ -247,7 +243,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, > unsigned int len, unsigned int truesize) > { > struct sk_buff *skb; > - struct skb_vnet_hdr *hdr; > + struct virtio_net_hdr_mrg_rxbuf *hdr; > unsigned int copy, hdr_len, hdr_padded_len; > char *p; > > @@ -260,13 +256,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, > > hdr = skb_vnet_hdr(skb); > > - if (vi->mergeable_rx_bufs) { > - hdr_len = sizeof hdr->mhdr; > - hdr_padded_len = sizeof hdr->mhdr; > - } else { > - hdr_len = sizeof hdr->hdr; > + hdr_len = vi->hdr_len; > + if (vi->mergeable_rx_bufs) > + hdr_padded_len = sizeof *hdr; > + else > hdr_padded_len = sizeof(struct padded_vnet_hdr); > - } > > memcpy(hdr, p, hdr_len); > > @@ -317,11 +311,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, > return skb; > } > > -static struct sk_buff *receive_small(void *buf, unsigned int len) > +static struct sk_buff *receive_small(struct virtnet_info *vi, void *buf, unsigned int len) > { > struct sk_buff * skb = buf; > > - len -= sizeof(struct virtio_net_hdr); > + len -= vi->hdr_len; > skb_trim(skb, len); > > return skb; > @@ -354,8 +348,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, > unsigned int len) > { > void *buf = mergeable_ctx_to_buf_address(ctx); > - struct skb_vnet_hdr *hdr = buf; > - u16 num_buf = virtio16_to_cpu(rq->vq->vdev, hdr->mhdr.num_buffers); > + struct virtio_net_hdr_mrg_rxbuf *hdr = buf; > + u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); > struct page *page = virt_to_head_page(buf); > int offset = buf - page_address(page); > unsigned int truesize = max(len, mergeable_ctx_to_buf_truesize(ctx)); > @@ -373,8 +367,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, > if (unlikely(!ctx)) { > pr_debug("%s: rx error: %d buffers out of %d missing\n", > dev->name, num_buf, > - virtio16_to_cpu(rq->vq->vdev, > - hdr->mhdr.num_buffers)); > + virtio16_to_cpu(vi->vdev, > + hdr->num_buffers)); > dev->stats.rx_length_errors++; > goto err_buf; > } > @@ -441,7 +435,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > struct net_device *dev = vi->dev; > struct virtnet_stats *stats = this_cpu_ptr(vi->stats); > struct sk_buff *skb; > - struct skb_vnet_hdr *hdr; > + struct virtio_net_hdr_mrg_rxbuf *hdr; > > if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) { > pr_debug("%s: short packet %i\n", dev->name, len); > @@ -536,7 +530,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq, > gfp_t gfp) > { > struct sk_buff *skb; > - struct skb_vnet_hdr *hdr; > + struct virtio_net_hdr_mrg_rxbuf *hdr; > int err; > > skb = __netdev_alloc_skb_ip_align(vi->dev, GOOD_PACKET_LEN, gfp); > @@ -547,7 +541,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq, > > hdr = skb_vnet_hdr(skb); > sg_init_table(rq->sg, MAX_SKB_FRAGS + 2); > - sg_set_buf(rq->sg, &hdr->hdr, sizeof hdr->hdr); > + sg_set_buf(rq->sg, hdr, vi->hdr_len); > skb_to_sgvec(skb, rq->sg + 1, 0, skb->len); > > err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp); > @@ -557,7 +551,8 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq, > return err; > } > > -static int add_recvbuf_big(struct receive_queue *rq, gfp_t gfp) > +static int add_recvbuf_big(struct virtnet_info *vi, struct receive_queue *rq, > + gfp_t gfp) > { > struct page *first, *list = NULL; > char *p; > @@ -588,8 +583,8 @@ static int add_recvbuf_big(struct receive_queue *rq, gfp_t gfp) > p = page_address(first); > > /* rq->sg[0], rq->sg[1] share the same page */ > - /* a separated rq->sg[0] for virtio_net_hdr only due to QEMU bug */ > - sg_set_buf(&rq->sg[0], p, sizeof(struct virtio_net_hdr)); > + /* a separated rq->sg[0] for header - required in case !any_header_sg */ > + sg_set_buf(&rq->sg[0], p, vi->hdr_len); > > /* rq->sg[1] for data packet, from offset */ > offset = sizeof(struct padded_vnet_hdr); > @@ -668,7 +663,7 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq, > if (vi->mergeable_rx_bufs) > err = add_recvbuf_mergeable(rq, gfp); > else if (vi->big_packets) > - err = add_recvbuf_big(rq, gfp); > + err = add_recvbuf_big(vi, rq, gfp); > else > err = add_recvbuf_small(vi, rq, gfp); > > @@ -848,18 +843,14 @@ static void free_old_xmit_skbs(struct send_queue *sq) > > static int xmit_skb(struct send_queue *sq, struct sk_buff *skb) > { > - struct skb_vnet_hdr *hdr; > + struct virtio_net_hdr_mrg_rxbuf *hdr; > const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest; > struct virtnet_info *vi = sq->vq->vdev->priv; > unsigned num_sg; > - unsigned hdr_len; > + unsigned hdr_len = vi->hdr_len; > bool can_push; > > pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest); > - if (vi->mergeable_rx_bufs) > - hdr_len = sizeof hdr->mhdr; > - else > - hdr_len = sizeof hdr->hdr; > > can_push = vi->any_header_sg && > !((unsigned long)skb->data & (__alignof__(*hdr) - 1)) && > @@ -867,7 +858,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb) > /* Even if we can, don't push here yet as this would skew > * csum_start offset below. */ > if (can_push) > - hdr = (struct skb_vnet_hdr *)(skb->data - hdr_len); > + hdr = (struct virtio_net_hdr_mrg_rxbuf *)(skb->data - hdr_len); > else > hdr = skb_vnet_hdr(skb); > > @@ -902,7 +893,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb) > } > > if (vi->mergeable_rx_bufs) > - hdr->mhdr.num_buffers = 0; > + hdr->num_buffers = 0; > > sg_init_table(sq->sg, MAX_SKB_FRAGS + 2); > if (can_push) { > @@ -1773,18 +1764,19 @@ static int virtnet_probe(struct virtio_device *vdev) > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) > vi->mergeable_rx_bufs = true; > > + if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) > + vi->hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf); > + else > + vi->hdr_len = sizeof(struct virtio_net_hdr); > + > if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT)) > vi->any_header_sg = true; > > if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) > vi->has_cvq = true; > > - if (vi->any_header_sg) { > - if (vi->mergeable_rx_bufs) > - dev->needed_headroom = sizeof(struct virtio_net_hdr_mrg_rxbuf); > - else > - dev->needed_headroom = sizeof(struct virtio_net_hdr); > - } > + if (vi->any_header_sg) > + dev->needed_headroom = vi->hdr_len; > > /* Use single tx/rx queue pair as default */ > vi->curr_queue_pairs = 1; > -- > MST