libmicrohttpd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [libmicrohttpd] [EXTERNAL] Re: New Features: Create response from an


From: Earp, Damon N. (GSFC-619.0)[SCIENCE SYSTEMS AND APPLICATIONS INC]
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: Tue, 2 Feb 2021 18:34:38 +0000

Afternoon,

Just checking in on the thread to make sure you all have everything you need from us. 

Thanks again,
Damon Earp

From: Sebald, Lawrence (GSFC-619.0)[SCIENCE SYSTEMS AND APPLICATIONS INC] <lawrence.sebald@nasa.gov>
Sent: Tuesday, January 19, 2021 12:29
To: libmicrohttpd development and user mailinglist <libmicrohttpd@gnu.org>
Cc: Christian Grothoff <grothoff@gnunet.org>; Earp, Damon N. (GSFC-619.0)[SCIENCE SYSTEMS AND APPLICATIONS INC] <damon.n.earp@nasa.gov>; Evgeny Grin <k2k@yandex.ru>
Subject: RE: [libmicrohttpd] [EXTERNAL] Re: New Features: Create response from an IOVEC and Thread-safe Adding Connections to an EPoll Internal Thread MHD_Daemon
 

Christian and Evgeny,

 

Thank you again for your prompt review of the patch file.

 

> 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.

 

The original reason that that code is as convoluted as it is is that our original patch had the internal copy of the iovec passed into the callback that is called when the response is freed. The idea was to ensure that the internal copy of the iovec would have the same exact elements in it as what the originally passed in iovec contained. You are probably correct that this could be simplified a bit now that the response free callback no longer passes the iovec and instead just uses a single user data pointer as does the callback-based content reader response free, as long as there’s no want for reuse of the response. Basically, the [0] element is the only one that should ever differ from what was passed in by the user with the code as it stands right now, which is what the data_iov_tr element is used for (keeping the original copy of the element at data_iov_left[0]). Technically speaking, the only part of the data_iov_tr that needs to be zeroed is the iov_base, so the memset could be replaced just with setting that value, but I figured the memset was more clear for intent.

 

> While working on merging your patch, I've discovered that you designed new kind of responses as non-reusable and they cannot be used simultaneously for replying for two requests as response contains several members (like data_iov_left, data_iovcnt_left) that related to some specific response process. Was it your initial design idea?

 

Our system only uses these responses once, so reusability was not something that we had in mind initially, no. It is necessary to keep some state since sendmsg/writev do not guarantee that the data written is fully sent in one call, just as send/write do not -- therefore we do have to keep track of the position within the iovec where the last write ended to ensure that we can take this into account.

 

> Also test_get_iovec is still using one continuous buffer. Maybe you used some wrong version of this file for the patch?

 

The test_get_iovec test actually tests both cases. The testInternalGet function is called twice in main, once using  a contiguous buffer (created with the ahc_echo callback) and once using a non-contiguous one (created with the ncont_echo callback).

 

Regards,

Lawrence

 

From: Evgeny Grin <k2k@yandex.ru>
Sent: Sunday, January 17, 2021 11:40 AM
To: 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>
Cc: Christian Grothoff <grothoff@gnunet.org>; libmicrohttpd development and user mailinglist <libmicrohttpd@gnu.org>
Subject: Re: [libmicrohttpd] [EXTERNAL] Re: New Features: Create response from an IOVEC and Thread-safe Adding Connections to an EPoll Internal Thread MHD_Daemon

 

Hi Daemon and Lawrence,

While working on merging your patch, I've discovered that you designed new kind of responses as non-reusable and they cannot be used simultaneously for replying for two requests as response contains several members (like data_iov_left, data_iovcnt_left) that related to some specific response process. Was it your initial design idea?

Also test_get_iovec is still using one continuous buffer. Maybe you used some wrong version of this file for the patch?

--
Wishes,
Evgeny

 


From: Christian Grothoff <grothoff@gnunet.org>

Sent: Saturday, January 16, 2021, 13:53 UTC+3

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

 

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.
 
 
 
 

 


reply via email to

[Prev in Thread] Current Thread [Next in Thread]