Ilya Maximets wrote:
> On 8/22/26 10:55 PM, Willem de Bruijn wrote:
> > Ilya Maximets wrote:
> >> On 8/22/26 2:09 AM, Jakub Kicinski wrote:
> >>> On Fri, 21 Aug 2026 23:45:41 +0200 Ilya Maximets wrote:
> >>>> 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.
> >>
> >>> 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!
>
> >
> >> 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
