Re: [PATCH net-next v10 03/16] net: Add a function to splice pages into an skbuff for MSG_SPLICE_PAGES

From: David Howells <dhowells@redhat.com>
To: Yunsheng Lin <linyunsheng@huawei.com>
Cc: dhowells@redhat.com, netdev@vger.kernel.org,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
	David Ahern <dsahern@kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Christoph Hellwig <hch@infradead.org>,
	Jens Axboe <axboe@kernel.dk>, Jeff Layton <jlayton@kernel.org>,
	Christian Brauner <brauner@kernel.org>,
	Chuck Lever III <chuck.lever@oracle.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org
Subject: Re: [PATCH net-next v10 03/16] net: Add a function to splice pages into an skbuff for MSG_SPLICE_PAGES
Date: Wed, 24 May 2023 14:21:34 +0100	[thread overview]
Message-ID: <3587228.1684934494@warthog.procyon.org.uk> (raw)
In-Reply-To: <82041a42-e7b0-bde3-0f70-8ad180565794@huawei.com>

Yunsheng Lin <linyunsheng@huawei.com> wrote:

> > + * Returns the amount of data spliced/copied or -EMSGSIZE if there's
> 
> I am not seeing any copying done directly in the skb_splice_from_iter(),
> maybe iov_iter_extract_pages() has done copying for it?

Ah, I took the code for that out and deferred it.  The comment needs amending.

> > +			ret = skb_append_pagefrags(skb, page, off, part,
> > +						   frag_limit);
> > +			if (ret < 0) {
> > +				iov_iter_revert(iter, len);
> 
> I am not sure I understand the error handling here, doesn't 'len'
> indicate the remaining size of the data to be appended to skb,

Yes.

> maybe we should revert the size of data that is already appended to skb
> here?  Does 'spliced' need to be adjusted accordingly?

Neither.

> I am not very familiar with the 'struct iov_iter' yet

An iov_iter struct is a cursor over a buffer.  It advances as we draw data or
space from that buffer.  Sometimes we overdraw and have to back up a bit -
hence the revert function.  It could possibly be renamed to something more
appropriate as (if/when ITER_PIPE is removed) it doesn't actually change the
buffer.

So looking at skb_splice_from_iter():

iov_iter_extract_pages() is used to get a list of pages from the buffer that
we think we're going to be able to handle.  If the buffer is of type IOVEC or
UBUF those pages would have pins inserted into them also; otherwise no pin or
ref will be taken on them.  MSG_SPLICE_PAGES should not be used with IOVEC or
UBUF types for the moment as the network layer does not yet handle pins.

iov_iter_extract_pages() will advance the iterator past the page fragments it
has returned.  If skb_append_pagefrags() indicates that it could not attach
the page, this isn't necessarily fatal - it could return -EMSGSIZE to indicate
there was no space, in which case we return to the caller to create a new
skbuff.

If a non-fatal error occurs, we may already have committed some parts of the
buffer to the skbuff and rewinding into that part of the buffer would cause a
repeat of the data which would be bad.

What the iov_iter_revert() is doing is rewinding iterator back past the part
of the extracted pages that we didn't get to use so that we will pick up where
we left off next time we're called.  It does *not* and must not revert the
data we've already transferred.

Arguably, I should revert when I return -EIO because sendpage_ok() returned
false, but that's a fatal error.

David