| Message ID | 20201230191244.610449-1-jonathan.lemon@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Generic zcopy_* functions | expand |
On Wed, Dec 30, 2020 at 2:12 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > From: Jonathan Lemon <bsd@fb.com> > > This is set of cleanup patches for zerocopy which are intended > to allow a introduction of a different zerocopy implementation. > > The top level API will use the skb_zcopy_*() functions, while > the current TCP specific zerocopy ends up using msg_zerocopy_*() > calls. > > There should be no functional changes from these patches. > > v2->v3: > Rename zc_flags to 'flags'. Use SKBFL_xxx naming, similar > to the SKBTX_xx naming. Leave zerocopy_success naming alone. > Reorder patches. > > v1->v2: > Break changes to skb_zcopy_put into 3 patches, in order to > make it easier to follow the changes. Add Willem's suggestion > about renaming sock_zerocopy_ Overall, this latest version looks fine to me. The big question is how this fits in with the broader rx direct placement feature. But it makes sense to me to checkpoint as is at this point. One small comment: skb_zcopy_* is a logical prefix for functions that act on sk_buffs, Such as skb_zcopy_set, which associates a uarg with an skb. Less for functions that operate directly on the uarg, and do not even take an skb as argument: skb_zcopy_get and skb_zcopy_put. Perhaps net_zcopy_get/net_zcopy_put?
On Mon, Jan 04, 2021 at 12:39:35PM -0500, Willem de Bruijn wrote: > On Wed, Dec 30, 2020 at 2:12 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > From: Jonathan Lemon <bsd@fb.com> > > > > This is set of cleanup patches for zerocopy which are intended > > to allow a introduction of a different zerocopy implementation. > > > > The top level API will use the skb_zcopy_*() functions, while > > the current TCP specific zerocopy ends up using msg_zerocopy_*() > > calls. > > > > There should be no functional changes from these patches. > > > > v2->v3: > > Rename zc_flags to 'flags'. Use SKBFL_xxx naming, similar > > to the SKBTX_xx naming. Leave zerocopy_success naming alone. > > Reorder patches. > > > > v1->v2: > > Break changes to skb_zcopy_put into 3 patches, in order to > > make it easier to follow the changes. Add Willem's suggestion > > about renaming sock_zerocopy_ > > Overall, this latest version looks fine to me. > > The big question is how this fits in with the broader rx direct > placement feature. But it makes sense to me to checkpoint as is at > this point. > > One small comment: skb_zcopy_* is a logical prefix for functions that > act on sk_buffs, Such as skb_zcopy_set, which associates a uarg with > an skb. Less for functions that operate directly on the uarg, and do > not even take an skb as argument: skb_zcopy_get and skb_zcopy_put. > Perhaps net_zcopy_get/net_zcopy_put? Or even just zcopy_get / zcopy_put ?
On Mon, Jan 4, 2021 at 11:17 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > On Mon, Jan 04, 2021 at 12:39:35PM -0500, Willem de Bruijn wrote: > > On Wed, Dec 30, 2020 at 2:12 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > > > From: Jonathan Lemon <bsd@fb.com> > > > > > > This is set of cleanup patches for zerocopy which are intended > > > to allow a introduction of a different zerocopy implementation. > > > > > > The top level API will use the skb_zcopy_*() functions, while > > > the current TCP specific zerocopy ends up using msg_zerocopy_*() > > > calls. > > > > > > There should be no functional changes from these patches. > > > > > > v2->v3: > > > Rename zc_flags to 'flags'. Use SKBFL_xxx naming, similar > > > to the SKBTX_xx naming. Leave zerocopy_success naming alone. > > > Reorder patches. > > > > > > v1->v2: > > > Break changes to skb_zcopy_put into 3 patches, in order to > > > make it easier to follow the changes. Add Willem's suggestion > > > about renaming sock_zerocopy_ > > > > Overall, this latest version looks fine to me. > > > > The big question is how this fits in with the broader rx direct > > placement feature. But it makes sense to me to checkpoint as is at > > this point. > > > > One small comment: skb_zcopy_* is a logical prefix for functions that > > act on sk_buffs, Such as skb_zcopy_set, which associates a uarg with > > an skb. Less for functions that operate directly on the uarg, and do > > not even take an skb as argument: skb_zcopy_get and skb_zcopy_put. > > Perhaps net_zcopy_get/net_zcopy_put? > > Or even just zcopy_get / zcopy_put ? Zerocopy is such an overloaded term, that I'd keep some prefix, at least.
On Mon, Jan 04, 2021 at 11:22:35PM -0500, Willem de Bruijn wrote: > On Mon, Jan 4, 2021 at 11:17 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > On Mon, Jan 04, 2021 at 12:39:35PM -0500, Willem de Bruijn wrote: > > > On Wed, Dec 30, 2020 at 2:12 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > > > > > From: Jonathan Lemon <bsd@fb.com> > > > > > > > > This is set of cleanup patches for zerocopy which are intended > > > > to allow a introduction of a different zerocopy implementation. > > > > > > > > The top level API will use the skb_zcopy_*() functions, while > > > > the current TCP specific zerocopy ends up using msg_zerocopy_*() > > > > calls. > > > > > > > > There should be no functional changes from these patches. > > > > > > > > v2->v3: > > > > Rename zc_flags to 'flags'. Use SKBFL_xxx naming, similar > > > > to the SKBTX_xx naming. Leave zerocopy_success naming alone. > > > > Reorder patches. > > > > > > > > v1->v2: > > > > Break changes to skb_zcopy_put into 3 patches, in order to > > > > make it easier to follow the changes. Add Willem's suggestion > > > > about renaming sock_zerocopy_ > > > > > > Overall, this latest version looks fine to me. > > > > > > The big question is how this fits in with the broader rx direct > > > placement feature. But it makes sense to me to checkpoint as is at > > > this point. > > > > > > One small comment: skb_zcopy_* is a logical prefix for functions that > > > act on sk_buffs, Such as skb_zcopy_set, which associates a uarg with > > > an skb. Less for functions that operate directly on the uarg, and do > > > not even take an skb as argument: skb_zcopy_get and skb_zcopy_put. > > > Perhaps net_zcopy_get/net_zcopy_put? > > > > Or even just zcopy_get / zcopy_put ? > > Zerocopy is such an overloaded term, that I'd keep some prefix, at least. I'll make that change and repost the series when net-next opens. -- Jonathan
From: Jonathan Lemon <bsd@fb.com> This is set of cleanup patches for zerocopy which are intended to allow a introduction of a different zerocopy implementation. The top level API will use the skb_zcopy_*() functions, while the current TCP specific zerocopy ends up using msg_zerocopy_*() calls. There should be no functional changes from these patches. v2->v3: Rename zc_flags to 'flags'. Use SKBFL_xxx naming, similar to the SKBTX_xx naming. Leave zerocopy_success naming alone. Reorder patches. v1->v2: Break changes to skb_zcopy_put into 3 patches, in order to make it easier to follow the changes. Add Willem's suggestion about renaming sock_zerocopy_ Patch 1: remove dead function Patch 2: simplify sock_zerocopy_put Patch 3: push status/refcounts into sock_zerocopy_callback Patch 4: replace sock_zerocopy_put with skb_zcopy_put Patch 5: rename sock_zerocopy_get Patch 6: Add an optional skb parameter to callback, allowing access to the attached skb from the callback. Patch 7: Add skb_zcopy_put_abort, and move zerocopy logic into the callback function. There unfortunately is still a check against the callback type here. Patch 8: Relocate skb_zcopy_clear() in skb_release_data() Patch 9: rename sock_zerocopy_ to msg_zerocopy_ Patch 10: Move zerocopy bits from tx_flags into flags for clarity. These bits will be used in the RX path in the future. Patch 11: Set the skb flags from the ubuf being attached, instead of a fixed value, allowing different initialization types. Patch 12: Replace open-coded assignments with skb_zcopy_init() Jonathan Lemon (12): skbuff: remove unused skb_zcopy_abort function skbuff: simplify sock_zerocopy_put skbuff: Push status and refcounts into sock_zerocopy_callback skbuff: replace sock_zerocopy_put() with skb_zcopy_put() skbuff: replace sock_zerocopy_get with skb_zcopy_get skbuff: Add skb parameter to the ubuf zerocopy callback skbuff: Call sock_zerocopy_put_abort from skb_zcopy_put_abort skbuff: Call skb_zcopy_clear() before unref'ing fragments skbuff: rename sock_zerocopy_* to msg_zerocopy_* net: group skb_shinfo zerocopy related bits together. skbuff: add flags to ubuf_info for ubuf setup tap/tun: add skb_zcopy_init() helper for initialization. drivers/net/tap.c | 6 +- drivers/net/tun.c | 6 +- drivers/net/xen-netback/common.h | 3 +- drivers/net/xen-netback/interface.c | 4 +- drivers/net/xen-netback/netback.c | 5 +- drivers/vhost/net.c | 4 +- include/linux/skbuff.h | 106 +++++++++++++++------------- net/core/skbuff.c | 65 ++++++++--------- net/ipv4/ip_output.c | 5 +- net/ipv4/tcp.c | 8 +-- net/ipv6/ip6_output.c | 5 +- net/kcm/kcmsock.c | 4 +- 12 files changed, 113 insertions(+), 108 deletions(-)