| Message ID | Z0pMLtmaGPPSR3Ea@xiberoa (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] splice: do not checksum AF_UNIX sockets | expand |
On Fri, Nov 29, 2024 at 03:20:14PM -0800, Frederik Deweerdt wrote: > When `skb_splice_from_iter` was introduced, it inadvertently added > checksumming for AF_UNIX sockets. This resulted in significant > slowdowns, as when using sendfile over unix sockets. > > Using the test code [1] in my test setup (2G, single core x86_64 qemu), > the client receives a 1000M file in: > - without the patch: 1577ms (+/- 36.1ms) > - with the patch: 725ms (+/- 28.3ms) > > This commit skips addresses the issue by skipping checksumming when > splice occurs a AF_UNIX socket. > > [1] https://gist.github.com/deweerdt/a3ee2477d1d87524cf08618d3c179f06 > > Signed-off-by: Frederik Deweerdt <deweerdt.lkml@gmail.com> > Fixes: 2e910b95329c ("net: Add a function to splice pages into an skbuff for MSG_SPLICE_PAGES") > --- > net/core/skbuff.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 6841e61a6bd0..49e4f9ab625f 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -7233,7 +7233,7 @@ ssize_t skb_splice_from_iter(struct sk_buff *skb, struct iov_iter *iter, > goto out; > } > > - if (skb->ip_summed == CHECKSUM_NONE) > + if (skb->ip_summed == CHECKSUM_NONE && skb->sk->sk_family != AF_UNIX) Are you sure it is always safe to dereferene skb->sk here? I am not sure about the KCM socket case. Instead of checking skb->sk->sk_family, why not just pass an additional boolean parameter to this function? Thanks.
On Fri, Nov 29, 2024 at 05:43:04PM -0800, Cong Wang wrote: > On Fri, Nov 29, 2024 at 03:20:14PM -0800, Frederik Deweerdt wrote: > > When `skb_splice_from_iter` was introduced, it inadvertently added > > checksumming for AF_UNIX sockets. This resulted in significant > > slowdowns, as when using sendfile over unix sockets. > > > > Using the test code [1] in my test setup (2G, single core x86_64 qemu), > > the client receives a 1000M file in: > > - without the patch: 1577ms (+/- 36.1ms) > > - with the patch: 725ms (+/- 28.3ms) > > > > This commit skips addresses the issue by skipping checksumming when > > splice occurs a AF_UNIX socket. > > > > [1] https://gist.github.com/deweerdt/a3ee2477d1d87524cf08618d3c179f06 > > > > Signed-off-by: Frederik Deweerdt <deweerdt.lkml@gmail.com> > > Fixes: 2e910b95329c ("net: Add a function to splice pages into an skbuff for MSG_SPLICE_PAGES") > > --- > > net/core/skbuff.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index 6841e61a6bd0..49e4f9ab625f 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -7233,7 +7233,7 @@ ssize_t skb_splice_from_iter(struct sk_buff *skb, struct iov_iter *iter, > > goto out; > > } > > > > - if (skb->ip_summed == CHECKSUM_NONE) > > + if (skb->ip_summed == CHECKSUM_NONE && skb->sk->sk_family != AF_UNIX) > > Are you sure it is always safe to dereferene skb->sk here? I am not sure > about the KCM socket case. > > Instead of checking skb->sk->sk_family, why not just pass an additional > boolean parameter to this function? > I saw that every caller was dereferrencing an `sk`, but you are right that that a boolean would be safer. I'll resend an updated version. Thanks, Frederik
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 6841e61a6bd0..49e4f9ab625f 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -7233,7 +7233,7 @@ ssize_t skb_splice_from_iter(struct sk_buff *skb, struct iov_iter *iter, goto out; } - if (skb->ip_summed == CHECKSUM_NONE) + if (skb->ip_summed == CHECKSUM_NONE && skb->sk->sk_family != AF_UNIX) skb_splice_csum_page(skb, page, off, part); off = 0;
When `skb_splice_from_iter` was introduced, it inadvertently added checksumming for AF_UNIX sockets. This resulted in significant slowdowns, as when using sendfile over unix sockets. Using the test code [1] in my test setup (2G, single core x86_64 qemu), the client receives a 1000M file in: - without the patch: 1577ms (+/- 36.1ms) - with the patch: 725ms (+/- 28.3ms) This commit skips addresses the issue by skipping checksumming when splice occurs a AF_UNIX socket. [1] https://gist.github.com/deweerdt/a3ee2477d1d87524cf08618d3c179f06 Signed-off-by: Frederik Deweerdt <deweerdt.lkml@gmail.com> Fixes: 2e910b95329c ("net: Add a function to splice pages into an skbuff for MSG_SPLICE_PAGES") --- net/core/skbuff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)