| Message ID | E1XlZWY-0003HZ-Ef@gondolin.me.apana.org.au |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Nov 04, 2014 at 04:31:34PM +0800, Herbert Xu wrote: > This patch adds skb_copy_datagram_iter, which is identical to > skb_copy_datagram_iovec except that it operates on iov_iter > instead of iovec. > > Eventually all users of skb_copy_datagram_iovec should switch > over to iov_iter and then we can remove skb_copy_datagram_iovec. Too noisy, IMO. How about skb_copy_datagram_msg() first? The fewer places have to even think of iovec or iov_iter, the better... -- 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 Tue, Nov 04, 2014 at 02:32:00PM +0000, Al Viro wrote: > On Tue, Nov 04, 2014 at 04:31:34PM +0800, Herbert Xu wrote: > > This patch adds skb_copy_datagram_iter, which is identical to > > skb_copy_datagram_iovec except that it operates on iov_iter > > instead of iovec. > > > > Eventually all users of skb_copy_datagram_iovec should switch > > over to iov_iter and then we can remove skb_copy_datagram_iovec. > > Too noisy, IMO. How about skb_copy_datagram_msg() first? The fewer > places have to even think of iovec or iov_iter, the better... PS: "too noisy" is about turning every callsite of skb_copy_datagram_iovec into that of skb_copy_datagram_iter; the helper itself is just fine. -- 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 Tue, Nov 04, 2014 at 02:32:00PM +0000, Al Viro wrote: > > Too noisy, IMO. How about skb_copy_datagram_msg() first? The fewer > places have to even think of iovec or iov_iter, the better... We have places like tcp ucopy and tun that do not have msghdr. So doing skb_copy_datagram_msg means that we'd have to create a fake msghdr wrapper around them. The point is not everything comes in via sendmsg/recvmsg. What is your motivation for hiding iov/iov_iter? Do you plan to change their API at some future point? Cheers,
On Tue, Nov 04, 2014 at 02:35:36PM +0000, Al Viro wrote: > On Tue, Nov 04, 2014 at 02:32:00PM +0000, Al Viro wrote: > > > Too noisy, IMO. How about skb_copy_datagram_msg() first? The fewer > > places have to even think of iovec or iov_iter, the better... > > PS: "too noisy" is about turning every callsite of skb_copy_datagram_iovec > into that of skb_copy_datagram_iter; the helper itself is just fine. Hmm if that is your concern then I don't see how skb_copy_datagram_msg changes things as you'd still have to convert every existing caller of skb_copy_datagram_iovec. Colour me confused. Cheers,
On Tue, Nov 04, 2014 at 10:44:16PM +0800, Herbert Xu wrote: > On Tue, Nov 04, 2014 at 02:35:36PM +0000, Al Viro wrote: > > On Tue, Nov 04, 2014 at 02:32:00PM +0000, Al Viro wrote: > > > > > Too noisy, IMO. How about skb_copy_datagram_msg() first? The fewer > > > places have to even think of iovec or iov_iter, the better... > > > > PS: "too noisy" is about turning every callsite of skb_copy_datagram_iovec > > into that of skb_copy_datagram_iter; the helper itself is just fine. > > Hmm if that is your concern then I don't see how skb_copy_datagram_msg > changes things as you'd still have to convert every existing caller > of skb_copy_datagram_iovec. Colour me confused. Fewer places having to even think of iovec/iov_iter... -- 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 Tue, Nov 04, 2014 at 02:52:22PM +0000, Al Viro wrote: > > > Hmm if that is your concern then I don't see how skb_copy_datagram_msg > > changes things as you'd still have to convert every existing caller > > of skb_copy_datagram_iovec. Colour me confused. > > Fewer places having to even think of iovec/iov_iter... Well it's the difference between skb_copy_datagram_iter(..., &kmsghdr->iov_iter, ...) and skb_copy_datagram_msg(..., kmsghdr, ...) Heck we could even make skb_copy_datagram_msg an inline wrapper around skb_copy_datagram_iter if you like. Anyway, the point is that not everything comes with a kmsghdr. Cheers,
Herbert, please provide a cover letter for this series, and the most recent version of patch #2 gets various rejects when I try to apply it to net-next. Thanks. -- 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 Wed, Nov 05, 2014 at 03:24:10PM -0500, David Miller wrote: > > Herbert, please provide a cover letter for this series, and the most recent > version of patch #2 gets various rejects when I try to apply it to net-next. Sure, I'll regenerate them. However, while doing so I noticed that a number of my patches on tun/macvtap that you have previously set as accepted are missing from net-next. Could this be why you got the rejects? For example, this patch wasn't in net-next when I just did a pull. https://patchwork.ozlabs.org/patch/405966/ Cheers,
Hi Dave: This patch series adds the helper skb_copy_datagram_iter, which is meant to replace both skb_copy_datagram_iovec and its evil twin skb_copy_datagram_const_iovec. It then converts tun and macvtap over to the new helper and finally removes skb_copy_datagram_const_iovec which is only used by tun and macvtap. Cheers,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Thu, 6 Nov 2014 16:23:38 +0800 > On Wed, Nov 05, 2014 at 03:24:10PM -0500, David Miller wrote: >> >> Herbert, please provide a cover letter for this series, and the most recent >> version of patch #2 gets various rejects when I try to apply it to net-next. > > Sure, I'll regenerate them. However, while doing so I noticed that > a number of my patches on tun/macvtap that you have previously set > as accepted are missing from net-next. Could this be why you got > the rejects? Those were bug fixes so went into plain 'net', they will show up next time I do a merge and I will deal with the conflicts, if any. -- 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 06, 2014 at 12:25:00PM -0500, David Miller wrote: > From: Herbert Xu <herbert@gondor.apana.org.au> > Date: Thu, 6 Nov 2014 16:23:38 +0800 > > > On Wed, Nov 05, 2014 at 03:24:10PM -0500, David Miller wrote: > >> > >> Herbert, please provide a cover letter for this series, and the most recent > >> version of patch #2 gets various rejects when I try to apply it to net-next. > > > > Sure, I'll regenerate them. However, while doing so I noticed that > > a number of my patches on tun/macvtap that you have previously set > > as accepted are missing from net-next. Could this be why you got > > the rejects? > > Those were bug fixes so went into plain 'net', they will show up next > time I do a merge and I will deal with the conflicts, if any. I see. In that case it might be best to wait until those fixes hit net-next first before applying these patches as otherwise Stephen will get hit with some nasty merge conflicts. I'll repost them for RFC with the problems that Al pointed out fixed in the mean time. Cheers,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 7 Nov 2014 09:59:44 +0800 > In that case it might be best to wait until those fixes hit net-next > first before applying these patches as otherwise Stephen will get > hit with some nasty merge conflicts. I just merged net into net-next, so this barrier should no longer be present. Thanks. -- 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
Hi Dave: This patch series adds the helper skb_copy_datagram_iter, which is meant to replace both skb_copy_datagram_iovec and its evil twin skb_copy_datagram_const_iovec. It then converts tun and macvtap over to the new helper and finally removes skb_copy_datagram_const_iovec which is only used by tun and macvtap. The copy_to_iter return value issue pointed out by Al has now been fixed. Cheers,
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 6c8b6f6..5ff7054 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -148,6 +148,7 @@ struct net_device; struct scatterlist; struct pipe_inode_info; +struct iov_iter; #if defined(CONFIG_NF_CONNTRACK) || defined(CONFIG_NF_CONNTRACK_MODULE) struct nf_conntrack { @@ -2641,6 +2642,8 @@ int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *frm, int skb_copy_datagram_const_iovec(const struct sk_buff *from, int offset, const struct iovec *to, int to_offset, int size); +int skb_copy_datagram_iter(const struct sk_buff *from, int offset, + struct iov_iter *to, int size); void skb_free_datagram(struct sock *sk, struct sk_buff *skb); void skb_free_datagram_locked(struct sock *sk, struct sk_buff *skb); int skb_kill_datagram(struct sock *sk, struct sk_buff *skb, unsigned int flags); diff --git a/net/core/datagram.c b/net/core/datagram.c index fdbc9a8..45a9d4d 100644 --- a/net/core/datagram.c +++ b/net/core/datagram.c @@ -49,6 +49,7 @@ #include <linux/spinlock.h> #include <linux/slab.h> #include <linux/pagemap.h> +#include <linux/uio.h> #include <net/protocol.h> #include <linux/skbuff.h> @@ -482,6 +483,87 @@ fault: EXPORT_SYMBOL(skb_copy_datagram_const_iovec); /** + * skb_copy_datagram_iter - Copy a datagram to an iovec iterator. + * @skb: buffer to copy + * @offset: offset in the buffer to start copying from + * @to: iovec iterator to copy to + * @len: amount of data to copy from buffer to iovec + */ +int skb_copy_datagram_iter(const struct sk_buff *skb, int offset, + struct iov_iter *to, int len) +{ + int start = skb_headlen(skb); + int i, copy = start - offset; + struct sk_buff *frag_iter; + + trace_skb_copy_datagram_iovec(skb, len); + + /* Copy header. */ + if (copy > 0) { + if (copy > len) + copy = len; + if (copy_to_iter(skb->data + offset, copy, to)) + goto fault; + if ((len -= copy) == 0) + return 0; + offset += copy; + } + + /* Copy paged appendix. Hmm... why does this look so complicated? */ + for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { + int end; + const skb_frag_t *frag = &skb_shinfo(skb)->frags[i]; + + WARN_ON(start > offset + len); + + end = start + skb_frag_size(frag); + if ((copy = end - offset) > 0) { + int err; + u8 *vaddr; + struct page *page = skb_frag_page(frag); + + if (copy > len) + copy = len; + vaddr = kmap(page); + err = copy_to_iter(vaddr + frag->page_offset + + offset - start, copy, to); + kunmap(page); + if (err) + goto fault; + if (!(len -= copy)) + return 0; + offset += copy; + } + start = end; + } + + skb_walk_frags(skb, frag_iter) { + int end; + + WARN_ON(start > offset + len); + + end = start + frag_iter->len; + if ((copy = end - offset) > 0) { + if (copy > len) + copy = len; + if (skb_copy_datagram_iter(frag_iter, offset - start, + to, copy)) + goto fault; + if ((len -= copy) == 0) + return 0; + offset += copy; + } + start = end; + } + if (!len) + return 0; + +fault: + return -EFAULT; +} +EXPORT_SYMBOL(skb_copy_datagram_iter); + +/** * skb_copy_datagram_from_iovec - Copy a datagram from an iovec. * @skb: buffer to copy * @offset: offset in the buffer to start copying to
This patch adds skb_copy_datagram_iter, which is identical to skb_copy_datagram_iovec except that it operates on iov_iter instead of iovec. Eventually all users of skb_copy_datagram_iovec should switch over to iov_iter and then we can remove skb_copy_datagram_iovec. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- include/linux/skbuff.h | 3 + net/core/datagram.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) -- 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