Greetings,
We've tried to address each of the concerns outlined in the earlier
emails. Attached is the new patch file with all the changes applied to
the 0.9.72 tarball. Let us know if there is anything missing, or if
there are other issues with the patch.
* All changes regarding creating a connection queue have been removed.
0.9.72's implementation is working well using the NO_LISTEN_SOCKET and
EPOLL_INTERNAL_THREAD options for us.
* We added a struct MHD_Iovec, to remove sys/uio.h and ensure
MHD_create_response_from_iovec is available on all platforms.
* We've updated doc/libmicrohttpd.texi to include the new
MHD_create_response_from_iovec function and the new MHD_Iovec struct.
* To address the testing concern, we copied the test_get_sendfile.c unit
test and changed the response type from fd to an iovec. And updated the
Makefile for it to run on `make check`.
* We've updated MHD_send_iovec_ to use MHD_send_data_ to replace
sendmsg/writev where it is not available.
* To test an unsupported platform we compiled and tested the iovec
response on windows 10 using mingw-w64.
Thanks,
Damon Earp
------------------------------------------------------------------------
*From:* Earp, Damon N. (GSFC-619.0)[SCIENCE SYSTEMS AND APPLICATIONS
INC] <damon.n.earp@nasa.gov>
*Sent:* Friday, January 8, 2021 15:12
*To:* Christian Grothoff <grothoff@gnunet.org>; libmicrohttpd@gnu.org
<libmicrohttpd@gnu.org>
*Cc:* Sebald, Lawrence (GSFC-619.0)[SCIENCE SYSTEMS AND APPLICATIONS
INC] <lawrence.sebald@nasa.gov>
*Subject:* Re: [EXTERNAL] Re: [libmicrohttpd] New Features: Create
response from an IOVEC and Thread-safe Adding Connections to an EPoll
Internal Thread MHD_Daemon
Thanks for the feedback,
I'm not surprised by any of the gripes, most of this stuff has been done
for our use case and not in a portable manner. As neither of us are
adept at this code we might need some guidance. We missed the addition
of the new connection list that was added in 0.9.72 and will look to use
that instead of our now duplicate implementation. Anything to minimize
the number of changes we make will be great.
> Could you explain the new functionality added by
0.9.72-internal_queue_connection.patch?
Hopefully most of these changes are moot with 0.9.72's
`new_connections_head`. Now that we are aware of our duplicate effort we
might start from scratch on this with a fresh 0.9.72 and see if any of
these changes are needed still.
* We implemented an `internal_queue_connection` which appends the new
connection struct to our (duplicated) new connection linked list,
then uses the itc interface to inform the internal thread. All while
using a dedicated mutex. We followed the `internal_add_connection`
signature and saved all the parameters so we could easily call
`internal_add_connection` in our event thread.
* Instead of exposing a new `MHD_queue_connection` we updated
`MHD_add_connection` to detect a daemon that
was MHD_USE_INTERNAL_POLLING_THREAD and MHD_USE_EPOLL. In this case
we use the `internal_queue_connection` which didn't race when called
from our external thread.
* The changes at `@@ -4992,6 +5054,32 @@` is for `MHD_epoll`. This
change is where we drain our connection queue and call
`internal_add_connection` on from the internal thread.
* The changes around `daemon->listen_socket_in_epoll = true;` and
`epoll_ctl` are in `setup_epoll_to_listen`. We had to reorder this
logic because the itc eventfd wasn't being registered with epoll
when MHD_USE_NO_LISTEN_SOCKET was paired with
MHD_USE_INTERNAL_POLLING_THREAD and MHD_USE_EPOLL. My guess is we
will run into this again and this might be the only change that
remains in this patch.
* All other changes are just setting up our linked list struct, mutex,
and header definitions
> The header sys/uio.h is not portable.
Noted, we will work on making the iovec code portable and emulating
sendmsg on incompatible platforms.
> Did you run any tests with/without your patch? Did it significantly
increase throughput?
We have run correctness tests but not benchmarking, patched 0.9.62 has
been in production for over a month using these changes. We used perf
early on and noticed memcpys dominating our cpu time which is why we
started down this path. We've had times where end users have requested >
5 TB of data in a sort timespan and there were 4 copies made going from
kernel through our program and back to kernel. With the addition of a
unit test we can compare MHD_create_response_from_iovec to
MHD_create_response_from_callback.
> Please, use "diff -p" next time as it simplify reviewing the changes.
On it, first time doing this.
> It would be great if you could update the libmicrohttpd manual
Will do.
> It would be good to have a simple unit test
We will look into adding one.
Thanks,
Damon Earp
------------------------------------------------------------------------
*From:* Christian Grothoff <grothoff@gnunet.org>
*Sent:* Friday, January 8, 2021 13:47
*To:* libmicrohttpd development and user mailinglist <libmicrohttpd@gnu.org>
*Cc:* Earp, Damon N. (GSFC-619.0)[SCIENCE SYSTEMS AND APPLICATIONS INC]
<damon.n.earp@nasa.gov>; Sebald, Lawrence (GSFC-619.0)[SCIENCE SYSTEMS
AND APPLICATIONS INC] <lawrence.sebald@nasa.gov>
*Subject:* [EXTERNAL] Re: [libmicrohttpd] New Features: Create response
from an IOVEC and Thread-safe Adding Connections to an EPoll Internal
Thread MHD_Daemon
Dear Damon,
Dear Lawrence,
This is a good place for such patches.
Like Evgeny, I'm in principle in favor, but have also some "major" gripes:
1) It would be great if you could update the libmicrohttpd manual (in
docs/) to document the new external API function(s). Just a few
sentences explaining the arguments and what the function does.
2) It would be good to have a simple unit test (see src/testcurl/ for
examples), especially so that we find out if basic things break on
certain platforms (given the non-portable nature of the patch).
Happy hacking!
Christian
On 1/8/21 6:14 PM, Earp, Damon N. (GSFC-619.0)[SCIENCE SYSTEMS AND
APPLICATIONS INC] via libmicrohttpd wrote:
We've made changes to libmicrohttpd that we'd like to see integrated into the
project. If this isn't the appropriate forum for submitting patches please let
us know.
OpenPGP_signature