RFC copy aside frags with destructors (was [PATCH 7/9] net: add skb_orphan_frags to copy aside frags with destructors)
Get series with b4
b4 am [email protected]Versions newest first
- v1 RFC Latest6 patches 23 repliesIn review
Patches in v1 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/6 | skbuff: support per-page destructors in copy_ubufsnet/ | +14 | 9 replies | |
| 2/6 | skbuff: add an api to orphan fragsinclude/ | +24 | 1 reply | |
| 3/6 | skbuff: convert to skb_orphan_fragsnet/ | +8 | 0 replies | |
| 4/6 | tun: orphan frags on xmitdrivers/ | +2 | 1 reply | |
| 5/6 | net: orphan frags on receivenet/ | +2 | 3 replies | |
| 6/6 | skbuff: set zerocopy flag on frag destructorinclude/ net/ | +21 | 1 reply |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.
Cover letter [PATCH RFC 0/6] copy aside frags with destructors (was [PATCH 7/9] net: add skb_orphan_frags to copy aside frags with destructors)
Quoted 9 lines · Ian Campbell
Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · David Miller
From: "Michael S. Tsirkin" <[email protected]>
Date: Fri, 4 May 2012 00:10:24 +0300Quoted 2 lines
Hmm we orphan skbs when we loop them back so how about reusing the
skb->destructorfor this?That's one possibility.
So originally I thought it would work: destructor would wake the original owner which would do data copy and modify the fragments. But then I unfortunately realized that would be racy: the new owner could be using the old frags and there appears no way for us to
make sure it doesn't so that we can put the original page. And the same logic applies to modifying the frags at any other time if the skb is cloned. So it seems we must copy if we want to clone the skb.
Further, destructor itself can't do the copy because this needs to allocate memory in atomic context, and destructor
itself can't fail.
For this second problem I see two solutions: either pre-allocate the copy buffer just in case and track it together with the destructor, or use an skb flag to make the check for destructors quick (if not completely free).
Second option is what macvtap zero copy uses and it already does copy on clone too. So I hacked that to make it support tcp/udp used by sunrpc.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
But I fear we're about to toss Ian into yet another rabbit hole. :-)
Let's try to converge on something quickly as I think integration of his work has been delayed enough as-is.
...
Quoted 7 lines
Quoted 2 lines
It's weekend here, I'll work on a patch like this
Sunday.Thanks, I was starting to feel my nose twitching and my ears beginning to elongate ;-)
Ian.
Here's a first stub at a fix. Basically to be able to modify frags on the fly we must make sure the skb isn't cloned, so the moment someone clones the skb we need to trigger the frag copy logic. And this is exactly what happens with SKBTX_DEV_ZEROCOPY so it seems to make sense to reuse that logic.
The below patchset replaces patch 7
([PATCH 7/9] net: add skb_orphan_frags to copy aside frags with destructors) in Ian's patchset and needs to be applied there.
Compiled only but I'd like to hear what people think all the same because it does add a couple of branches on fast path. On the other hand this makes it generic so the same logic will be reusable for packet sockets (which IIRC are currently buggy in the same way as sunrpc) and for adding zero copy support to tun.
Please comment,
Thanks!
Signature · 19 lines
MST Michael S. Tsirkin (6): skbuff: support per-page destructors in copy_ubufs skbuff: add an api to orphan frags skbuff: convert to skb_orphan_frags tun: orphan frags on xmit net: orphan frags on receive skbuff: set zerocopy flag on frag destructor drivers/net/tun.c | 2 ++ include/linux/skbuff.h | 41 +++++++++++++++++++++++++++++++++++++++++ net/core/dev.c | 2 ++ net/core/skbuff.c | 43 +++++++++++++++++++++++++------------------ 4 files changed, 70 insertions(+), 18 deletions(-) -- MST