Ilya Maximets wrote:
On 8/22/26 10:55 PM, Willem de Bruijn wrote:
quoted
Ilya Maximets wrote:
quoted
On 8/22/26 2:09 AM, Jakub Kicinski wrote:
quoted
On Fri, 21 Aug 2026 23:45:41 +0200 Ilya Maximets wrote:
quoted
Unfortunately, this needs a rebase now that a conflicting change
for skb_zerocopy() was merged:
Ugh, I was supposed to merge this first, wasn't I? Sorry.
Not a huge deal, I guess, the conflict is mechanical and the patches
are simple. I can take care of manual backports once we get the
'failed to apply' emails. Just a bit of busy work.
quoted
I was hoping for Willem to TAL since skb_tx_error() is a tx ZC
thing, now I realized that he wasn't CCed :S (please do so on v4)
FWIW, I CCed a few people on v1 to have a conversation about a proper
fix, but that wasn't fruitful. So, if I were Norbert, I wouldn't
include them for the new versions either as doing so always feels like
me being annoying. :)
Having a look now.
Thanks!
quoted
quoted
For now, the plan is to get v4 of these targeted fixes into net and
stable and then remove skb_tx_error() entirely once net-next is open,
as it seems to have lost all of its prior meaning.
The original use case in tun_net_xmit introduced in commit
149d36f7187c ("tun: report orphan frags errors to zero copy callback")
still exists. Not sure you can remove the function entirely.
The skb_tx_error() prescribes to call kfree_skb() right after it and
all the callers more or less do that (with the fixes applied).
skb_tx_error() does two things:
1. skb_zcopy_downgrade_managed() that takes extra references on frags.
2. Calls skb_zcopy_clear(skb, true);
The kfree_skb() called right after does:
__kfree_skb
skb_release_all
skb_release_data
if (skb_zcopy)
bool skip_unref = shinfo->flags & SKBFL_MANAGED_FRAG_REFS;
skb_zcopy_clear(skb, true);
if (skip_unref)
<skip unreferencing the frags, which is the same as taking
the extra reference>
So, unless I'm missing something, the kfree_skb() already does everything
that skb_tx_error() does.
Good point. I agree.
The fact that skb_tx_error() calls skb_zcopy_clear() with 'true' though
feels weird. I would understand the need for the function, if it was
actually signalling the error and not success. But you switched false
to true in commit 1f8b977ab32d ("sock: enable MSG_ZEROCOPY") nine years
ago and it seems like nobody complained so far...
I don't immediately recall the rationale. Probably not intentional
and I should have left the original call-sites, notably tun_net_xmit,
as is.
With MSG_ZEROCOPY, goal is to only set zerocopy_success to false if
a transmission could not be fully completed in zerocopy mode. Falling
back to copying deep in the stack is usually more expensive than doing
it from the start. And if the sendmsg otherwise succeeds, the caller
receives no other signal that MSG_ZEROCOPY is counterproductive.
skb_copy_ubufs will indeed call skb_zcopy_clear(.., false).
General transmit failures are signaled through the normal error path.
IMHO this includes allocation failure with GFP_ATOMIC.
That said, while I don't fully agree with these skb_tx_error()'s with
!zerocopy_success in the skb_orphan_frags() error paths, they did
precede my code. If we want to preserve them we would need to keep
skb_tx_error, with an extra zerocopy_success argument.