| Message ID | 0dc176ed796ff5ac7164b6c18cc75d64c2a2317e.1351679008.git.mst@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
From: "Michael S. Tsirkin" <mst@redhat.com> Date: Wed, 31 Oct 2012 12:31:06 +0200 > -void vhost_zerocopy_callback(struct ubuf_info *ubuf) > +void vhost_zerocopy_callback(struct ubuf_info *ubuf, int zerocopy_status) If you're only reporting true/false values, even just for now, please use 'bool' for this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 01, 2012 at 11:50:24AM -0400, David Miller wrote: > From: "Michael S. Tsirkin" <mst@redhat.com> > Date: Wed, 31 Oct 2012 12:31:06 +0200 > > > -void vhost_zerocopy_callback(struct ubuf_info *ubuf) > > +void vhost_zerocopy_callback(struct ubuf_info *ubuf, int zerocopy_status) > > If you're only reporting true/false values, even just for now, > please use 'bool' for this. In fact next patch reports -ENOMEM when tun hits OOM so callback can distinguish between copy (>0 value) and error (<0 value) and reduce zerocopy more aggressively in case of errors. The *callback* in vhost-net currently handles all non-zero values identically, but I am not sure it's the optimal behaviour so I thought it's worth it to give callbacks the info. Do you think it's over-engineering, or a good idea?
From: "Michael S. Tsirkin" <mst@redhat.com> Date: Thu, 1 Nov 2012 18:16:11 +0200 > Do you think it's over-engineering, or a good idea? Engineer what you need, not what you might need. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 99ac2cb..92308b6 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -1600,7 +1600,7 @@ void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *ubufs) kfree(ubufs); } -void vhost_zerocopy_callback(struct ubuf_info *ubuf) +void vhost_zerocopy_callback(struct ubuf_info *ubuf, int zerocopy_status) { struct vhost_ubuf_ref *ubufs = ubuf->ctx; struct vhost_virtqueue *vq = ubufs->vq; diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index 1125af3..eb7263c3 100644 --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -191,7 +191,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *); int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log, unsigned int log_num, u64 len); -void vhost_zerocopy_callback(struct ubuf_info *); +void vhost_zerocopy_callback(struct ubuf_info *, int); int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq); #define vq_err(vq, fmt, ...) do { \ diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 6a2c34e..8bac11b 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -235,11 +235,13 @@ enum { /* * The callback notifies userspace to release buffers when skb DMA is done in * lower device, the skb last reference should be 0 when calling this. + * The zerocopy_status argument is 0 if zero copy transmit occurred, + * 1 on successful data copy; < 0 on out of memory error. * The ctx field is used to track device context. * The desc field is used to track userspace buffer index. */ struct ubuf_info { - void (*callback)(struct ubuf_info *); + void (*callback)(struct ubuf_info *, int zerocopy_status); void *ctx; unsigned long desc; }; diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 6e04b1f..eb31f6e 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -519,7 +519,7 @@ static void skb_release_data(struct sk_buff *skb) uarg = skb_shinfo(skb)->destructor_arg; if (uarg->callback) - uarg->callback(uarg); + uarg->callback(uarg, 0); } if (skb_has_frag_list(skb)) @@ -797,7 +797,7 @@ int skb_copy_ubufs(struct sk_buff *skb, gfp_t gfp_mask) for (i = 0; i < num_frags; i++) skb_frag_unref(skb, i); - uarg->callback(uarg); + uarg->callback(uarg, 1); /* skb frags point to kernel buffers */ for (i = num_frags - 1; i >= 0; i--) {
Even if skb is marked for zero copy, net core might still decide to copy it later which is somewhat slower than a copy in user context: besides copying the data we need to pin/unpin the pages. Add a parameter reporting such cases through zero copy callback: if this happens a lot, device can take this into account and switch to copying in user context. This patch updates all users but ignores the passed value for now: it will be used by follow-up patches. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- drivers/vhost/vhost.c | 2 +- drivers/vhost/vhost.h | 2 +- include/linux/skbuff.h | 4 +++- net/core/skbuff.c | 4 ++-- 4 files changed, 7 insertions(+), 5 deletions(-)