Re: [libmicrohttpd] [EXTERNAL] Re: New Features: Create response from an IOVEC and Thread-safe Adding Connections to an EPoll Internal Thread MHD_Daemon

[Christian Grothoff]

Hi Daemon,
Hi Lawrence,

After more carefully looking at your code after

/* Adjust the internal tracking information for the iovec to take this
last send into account. */

I am quite confused about how this is supposed to possibly work. It
seems way more convoluted than necessary.

Why not simply one for loop:

if (ret >= 0)
{
  for (i = 0; i<response->data_iovcnt_left;i++)
  {
     if (ret < response->data_iov_left[i].iov_len)
     {
        response->data_iov_left[i].iov_base += ret;
        response->data_iov_left[i].iov_len -= ret;
        response->data_iov_left += i;
        response->data_iovcnt_left -= i;
        break;
     }
     ret -= response->data_iov_left[i].iov_len;
  }
}

I don't see hte need for the special-casing of [0], or the memset to zero.

What am I missing?

My 2 cents

Christian

On 1/15/21 9:12 PM, Earp, Damon N. (GSFC-619.0)[SCIENCE SYSTEMS AND
APPLICATIONS INC] wrote:
> Evgeny,
> 
> Lawrence went ahead and made some changes. Attached is the new patch file. 
> 
> Differences between this and the last patch file:
> 
> * Updated the test_get_iovec with the requested changes
> * Added test_https_get_iovec for testing https, included testing a 0
> length iovec.
> 
>> Does version 0.9.72 fix epoll without listen socket?
> Yes, we were able to remove all our changes regarding queuing new
> connections. Thanks!
> 
> Thanks,
> Damon Earp
> 
> ------------------------------------------------------------------------
> *From:* Christian Grothoff
> *Sent:* Friday, January 15, 2021 05:50
> *To:* Earp, Damon N. (GSFC-619.0)[SCIENCE SYSTEMS AND APPLICATIONS INC];
> Sebald, Lawrence (GSFC-619.0)[SCIENCE SYSTEMS AND APPLICATIONS INC]
> *Subject:* Fwd: [libmicrohttpd] [EXTERNAL] Re: New Features: Create
> response from an IOVEC and Thread-safe Adding Connections to an EPoll
> Internal Thread MHD_Daemon
> 
> Forwarding Evgeny's reply, in case you are not on the list...
> 
> 
> -------- Forwarded Message --------
> Subject: Re: [libmicrohttpd] [EXTERNAL] Re: New Features: Create
> response from an IOVEC and Thread-safe Adding Connections to an EPoll
> Internal Thread MHD_Daemon
> Date: Fri, 15 Jan 2021 12:08:03 +0300
> From: Evgeny Grin <k2k@yandex.ru>
> Reply-To: libmicrohttpd development and user mailinglist
> <libmicrohttpd@gnu.org>
> To: libmicrohttpd@gnu.org
> 
> Hello Damon,
> 
> Thanks for the new patch. It looks almost ready!
> 
> Besides minor things, like Yoda conditions which could be easily fixed
> by myself on merge, I'd like to ask you for two more improvements for
> test-suite.
> 
> There is the one single real buffer used as several smaller buffers to
> provide a response. This can hide some buffer overrun problems,
> especially if system sendmsd() is not available. Could you update the
> test with unordered data, like (very simplified):
> 
> static const char *str1 = "abc";
> static const char *str3 = "xyz";
> static const char *str2 = "123";
> 
> vec[0].iov_base = str1;
> vec[0].iov_len = 3;
> vec[1].iov_base = str2;
> vec[1].iov_len = 3;
> vec[2].iov_base = str3;
> vec[2].iov_len = 3;
> 
> The second thing is additional test for HTTPS. It could be very simple
> but we must sure that TLS implementation works correct as well.
> 
> Maybe Christian have some additional comments/wishes.
> 
> Does version 0.9.72 fix epoll without listen socket?
> 
> -- 
> Best Wishes,
> Evgeny
> 
> 
> On 14.01.2021 21:05, Earp, Damon N. (GSFC-619.0)[SCIENCE SYSTEMS AND
> APPLICATIONS INC] via libmicrohttpd wrote:
>> 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.
> 
> 
>

signature.asc
Description: OpenPGP digital signature