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

quic: use send/recvmmsg when available #14880

Closed
wants to merge 5 commits into from
Closed

Conversation

icing
Copy link
Contributor

@icing icing commented Sep 12, 2024

Enable use of send/recvmmsg on platforms that support it. Add checks for sendmmsg in configure and CmakeLists.txt. Will have effect for ngtcp2 and quiche HTTP/3.

One quirk: we need to define _GNU_SOURCE in vquic.c so that sys/socket.h offers the functions.

Performance: scorecard testing show only a little improvement in download numbers. Mostly, curl is only receiving 1-2 packets at a time. This might point to Caddy's inability to send packets fast enough or may be the side-effect of other system configurations.

@icing
Copy link
Contributor Author

icing commented Sep 12, 2024

@vszakats cmake does not detect sendmmsg because the _GNU_SOURCE is not defined when testing and sys/socket.h then does not expose it. Any idea how best to progress here?

@vszakats
Copy link
Member

Wouldn't we need to define _GNU_SOURCE globally for this? Depending on what it changes, setting it for a single file, or for the duration of an #include, may lead to issues, esp thinking of unity builds (where an earlier file might include sys/socket.h without _GNU_SOURCE being set).

If so, it would likely be better to define it globally (for platforms that support it or might need it) with add_definitions("-D_GNU_SOURCE"). This should make the feature test work as it is. (Also something along these lines with ./configure)

@bagder
Copy link
Member

bagder commented Sep 12, 2024

Wouldn't we need to define _GNU_SOURCE globally for this?

I suppose we need to, at least when building with a backend for which send/recvmmsg can be useful.

@icing
Copy link
Contributor Author

icing commented Sep 13, 2024

So, should I try for a define in curl_setup.h when we are on linux?

@vszakats
Copy link
Member

vszakats commented Sep 13, 2024

So, should I try for a define in curl_setup.h when we are on linux?

I think setting it on the build level has some advantages over curl_setup.h, e.g. it guarantees they are matching with detection results. E.g. something like:

diff --git a/CMake/Platforms/WindowsCache.cmake b/CMake/Platforms/WindowsCache.cmake
index 317f21c875..57b2b40bd4 100644
--- a/CMake/Platforms/WindowsCache.cmake
+++ b/CMake/Platforms/WindowsCache.cmake
@@ -88,6 +88,7 @@ set(HAVE_FREEADDRINFO 1)
 set(HAVE_FCHMOD 0)
 set(HAVE_SOCKETPAIR 0)
 set(HAVE_SENDMSG 0)
+set(HAVE_SENDMMSG 0)
 set(HAVE_ALARM 0)
 set(HAVE_FCNTL 0)
 set(HAVE_GETPPID 0)
diff --git a/CMakeLists.txt b/CMakeLists.txt
index e84c3af325..d8eb3d0d77 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -192,6 +192,10 @@ cmake_dependent_option(ENABLE_THREADED_RESOLVER "Enable threaded DNS lookup"
 
 include(PickyWarnings)
 
+if(CMAKE_SYSTEM_NAME MATCHES "Linux")
+  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -D_GNU_SOURCE")  # Required for sendmmsg()
+endif()
+
 option(ENABLE_DEBUG "Enable curl debug features" OFF)
 option(ENABLE_CURLDEBUG "Enable TrackMemory feature" ${ENABLE_DEBUG})
 
@@ -1477,6 +1481,7 @@ check_symbol_exists("socketpair"      "${CURL_INCLUDES}" HAVE_SOCKETPAIR)
 check_symbol_exists("recv"            "${CURL_INCLUDES}" HAVE_RECV)
 check_symbol_exists("send"            "${CURL_INCLUDES}" HAVE_SEND)
 check_symbol_exists("sendmsg"         "${CURL_INCLUDES}" HAVE_SENDMSG)
+check_symbol_exists("sendmmsg"        "sys/socket.h" HAVE_SENDMMSG)
 check_symbol_exists("select"          "${CURL_INCLUDES}" HAVE_SELECT)
 check_symbol_exists("strdup"          "${CURL_INCLUDES};string.h" HAVE_STRDUP)
 check_symbol_exists("strtok_r"        "${CURL_INCLUDES};string.h" HAVE_STRTOK_R)

(add_definitions() doesn't apply to detections, as it turns out.)

And something similar in configure, via CPPFLAGS:

case $host in
  *-*-linux*)
    CPPFLAGS="$CPPFLAGS -D_GNU_SOURCE";;
esac

# then detection as normal

@icing
Copy link
Contributor Author

icing commented Sep 13, 2024

Thanks, @vszakats, incorporated that into the latest push.

Hmm, cmake does not detect it and we have other fallouts...

@vszakats
Copy link
Member

vszakats commented Sep 13, 2024

Right, this needs to be added too to make the setting propagate:

--- a/lib/curl_config.h.cmake
+++ b/lib/curl_config.h.cmake
@@ -455,6 +455,9 @@
 /* Define to 1 if you have the sendmsg function. */
 #cmakedefine HAVE_SENDMSG 1
 
+/* Define to 1 if you have the sendmmsg function. */
+#cmakedefine HAVE_SENDMMSG 1
+
 /* Define to 1 if you have the 'fsetxattr' function. */
 #cmakedefine HAVE_FSETXATTR 1

It should fix the cmake-vs-autotools check and enable sendmmsg in CMake builds.

As for the curl-for-win fallout, I think we're seeing the effect of _GNU_SOURCE. It's triggering the declaration of calloc() in system headers, which collides with curl's PP override for that symbol. The solution is probably to copy (or move?) this:

#if defined(USE_THREADS_POSIX) && defined(HAVE_PTHREAD_H)
#  include <pthread.h>
#endif

to somewhere in curl_setup.h to always have it included before curl_memory.h and memdebug.h.

@bagder
Copy link
Member

bagder commented Sep 20, 2024

That scan-build warning needs to be fixed!

Stefan Eissing and others added 5 commits September 23, 2024 10:26
add checks for sendmmsg in configure and CmakeLists.txt for
enabling use of these functions in ngtcp2/quiche quic.
Seems unnecessary, but ubuntu scanbuild errors on this.
@icing
Copy link
Contributor Author

icing commented Sep 23, 2024

That scan-build warning needs to be fixed!

I do not understand the error in tool_operate.c:173. I clear the struct explicitly now, but it seems unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants