Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable UDP GRO for QUIC #14012

Closed
wants to merge 4 commits into from
Closed

Conversation

tatsuhiro-t
Copy link
Contributor

No description provided.

@bagder bagder added the HTTP/3 h3 or quic related label Jun 25, 2024
@bagder
Copy link
Member

bagder commented Jun 25, 2024

Nice. @tatsuhiro-t, have done any measurements to see how this changes performance?

@tatsuhiro-t
Copy link
Contributor Author

I did some experiments on loopback. Both curl and server did not fully utilize CPU, and 4GiB transfer took ~11 seconds without GRO and ~10 seconds with GRO. So it does make a difference, but YMMV.

@bagder
Copy link
Member

bagder commented Jun 26, 2024

The linux-musl-llvm build gets this compiler error:

In file included from /home/runner/work/curl/curl/curl/_x64-linux-musl-bld/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:496:
/home/runner/work/curl/curl/curl/lib/vquic/vquic.c:339:47: error: cast from 'unsigned char *' to 'struct cmsghdr *' increases required alignment from 1 to 4 [-Werror,-Wcast-align]
  339 |   for(cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
      |                                               ^~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-musl/sys/socket.h:359:8: note: expanded from macro 'CMSG_NXTHDR'
  359 |         ? 0 : (struct cmsghdr *)__CMSG_NEXT(cmsg))
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/runner/work/curl/curl/curl/_x64-linux-musl-bld/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:496:
/home/runner/work/curl/curl/curl/lib/vquic/vquic.c:339:47: error: comparison of integers of different signs: 'unsigned long' and 'long' [-Werror,-Wsign-compare]
  339 |   for(cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
      |                                               ^~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-musl/sys/socket.h:358:44: note: expanded from macro 'CMSG_NXTHDR'
  358 |         __CMSG_LEN(cmsg) + sizeof(struct cmsghdr) >= __MHDR_END(mhdr) - (unsigned char *)(cmsg) \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.

@tatsuhiro-t
Copy link
Contributor Author

I think it needs (void*) casts somewhere to shut up these warnings.

@tatsuhiro-t
Copy link
Contributor Author

Well, this is famous musl issue, and the only simple workaround is disable compiler warnings for this portion of the code: see https://git.openembedded.org/meta-openembedded/tree/meta-oe/recipes-devtools/breakpad/breakpad/0001-Turn-off-sign-compare-for-musl-libc.patch

@tatsuhiro-t
Copy link
Contributor Author

GRO is only enabled for ngtcp2+nghttp3 and quiche. The others do their own I/O, and enabling GRO here causes issues unless they are prepared for it.

@bagder bagder requested a review from icing June 26, 2024 09:53
@bagder
Copy link
Member

bagder commented Jun 26, 2024

Basic speed test with ngtcp2 with and without this PR. Server: caddy. Running on localhost.

tldr: the parallel test doing 50 times 100MB downloads runs 39% faster.

without

$ python3 scorecard.py --caddy -d --download=100mb h3
  Server       Size  single(1x1) [cpu/rss]         serial(50x1) [cpu/rss]         parallel(50x50) [cpu/rss]                Errors       
  caddy       100MB     554 MB/s [67.1%/14MB]          574 MB/s [71.5%/16MB]          509 MB/s [80.0%/16MB]               -          

with

$ python3 scorecard.py --caddy -d --download=100mb h3
  Server       Size  single(1x1) [cpu/rss]         serial(50x1) [cpu/rss]         parallel(50x50) [cpu/rss]                Errors       
  caddy       100MB     680 MB/s [57.3%/14MB]          701 MB/s [61.9%/15MB]          710 MB/s [81.9%/17MB]               -          

@bagder bagder closed this in a571afc Jun 26, 2024
@bagder
Copy link
Member

bagder commented Jun 26, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HTTP/3 h3 or quic related
Development

Successfully merging this pull request may close these issues.

None yet

2 participants