Patch: Improve HTTP time by sending Connection:close

Patch: Improve HTTP time by sending Connection:close

[Walter Huf]

[-- Attachment #1: Type: text/plain, Size: 2225 bytes --]

GRUB's HTTP module declares support for HTTP/1.1, which defaults to
Connection:keepalive. At the end of the content, the server holds the TCP
connection open waiting for the next request.
It seems that grub_net_poll_cards() is watching for the HTTP module to set
net->stall, and otherwise waits the full 400ms GRUB_NET_INTERVAL to return
to processing. However, HTTP module only sets that flag in specific
conditions:

   - parse_line detects that we are at the end of downloading a chunked
   Transfer-Encoding
   - http_err detects a problem with the underlying TCP connection
   - http_receive has queued 20 netbuffer packets for processing

If the file is small and takes less than 20 packets, and the server is not
using chunked encoding, grub_net_poll_cards() will wait the full 400ms
before continuing to process and finish the file download.

This patch sets Connection:close, which will tell the server to close the
connection as soon as it has finished sending the file. GRUB closes any
connections that are left open (in http_seek), so it does not change
performance. When the server disconnects, I think it triggers http_err and
then quits out of grub_net_poll_cards early.

---
diff --git a/grub-core/net/http.c b/grub-core/net/http.c
index 5aa4ad3..a5d64f6 100644
--- a/grub-core/net/http.c
+++ b/grub-core/net/http.c
@@ -318,6 +318,7 @@ http_establish (struct grub_file *file, grub_off_t
offset, int initial)
    + grub_strlen (data->filename)
    + sizeof (" HTTP/1.1\r\nHost: ") - 1
    + grub_strlen (file->device->net->server)
+   + sizeof ("\r\nConnection: close") - 1
    + sizeof ("\r\nUser-Agent: " PACKAGE_STRING
      "\r\n") - 1
    + sizeof ("Range: bytes=XXXXXXXXXXXXXXXXXXXX"
@@ -366,6 +367,17 @@ http_establish (struct grub_file *file, grub_off_t
offset, int initial)
        grub_strlen (file->device->net->server));

   ptr = nb->tail;
+  err = grub_netbuff_put (nb,
+  sizeof ("\r\nConnection: close")
+  - 1);
+  if (err)
+    {
+      grub_netbuff_free (nb);
+      return err;
+    }
+  grub_memcpy (ptr, "\r\nConnection: close",
+       sizeof ("\r\nConnection: close") - 1);
+  ptr = nb->tail;
   err = grub_netbuff_put (nb,
   sizeof ("\r\nUser-Agent: " PACKAGE_STRING "\r\n")
   - 1);

[-- Attachment #2: Type: text/html, Size: 3747 bytes --]

Re: Patch: Improve HTTP time by sending Connection:close

[Daniel Kiper]

On Sat, Nov 12, 2016 at 09:26:17AM -0800, Walter Huf wrote:
> GRUB's HTTP module declares support for HTTP/1.1, which defaults to
> Connection:keepalive. At the end of the content, the server holds the TCP
> connection open waiting for the next request.
> It seems that grub_net_poll_cards() is watching for the HTTP module to set

Do you have a feeling or are you sure? I think that we have to be sure here.

> net->stall, and otherwise waits the full 400ms GRUB_NET_INTERVAL to return
> to processing. However, HTTP module only sets that flag in specific
> conditions:
>
>    - parse_line detects that we are at the end of downloading a chunked
>    Transfer-Encoding
>    - http_err detects a problem with the underlying TCP connection
>    - http_receive has queued 20 netbuffer packets for processing
>
> If the file is small and takes less than 20 packets, and the server is not
> using chunked encoding, grub_net_poll_cards() will wait the full 400ms
> before continuing to process and finish the file download.
>
> This patch sets Connection:close, which will tell the server to close the
> connection as soon as it has finished sending the file. GRUB closes any
> connections that are left open (in http_seek), so it does not change

Why in http_seek? Is it correct or not?

> performance. When the server disconnects, I think it triggers http_err and

"I think" is too soft. You have to be sure here.

> then quits out of grub_net_poll_cards early.

Lack of SOB.

Have you tested this patch with large/huge number of files to transfer? I have
a feeling that it can slowdown whole transfer in such cases due to number of
connects/disconnects. Maybe this feature should be conditional thing.

Daniel

Re: Patch: Improve HTTP time by sending Connection:close

[Andrei Borzenkov]

On Tue, Nov 15, 2016 at 4:21 PM, Daniel Kiper <dkiper@net-space.pl> wrote:
> On Sat, Nov 12, 2016 at 09:26:17AM -0800, Walter Huf wrote:
>> GRUB's HTTP module declares support for HTTP/1.1, which defaults to
>> Connection:keepalive. At the end of the content, the server holds the TCP
>> connection open waiting for the next request.
>> It seems that grub_net_poll_cards() is watching for the HTTP module to set
>
> Do you have a feeling or are you sure? I think that we have to be sure here.
>

See http://savannah.gnu.org/bugs/?49531 which has more details.

>> net->stall, and otherwise waits the full 400ms GRUB_NET_INTERVAL to return
>> to processing. However, HTTP module only sets that flag in specific
>> conditions:
>>
>>    - parse_line detects that we are at the end of downloading a chunked
>>    Transfer-Encoding
>>    - http_err detects a problem with the underlying TCP connection
>>    - http_receive has queued 20 netbuffer packets for processing
>>
>> If the file is small and takes less than 20 packets, and the server is not
>> using chunked encoding, grub_net_poll_cards() will wait the full 400ms
>> before continuing to process and finish the file download.
>>
>> This patch sets Connection:close, which will tell the server to close the
>> connection as soon as it has finished sending the file. GRUB closes any
>> connections that are left open (in http_seek), so it does not change
>
> Why in http_seek? Is it correct or not?
>
>> performance. When the server disconnects, I think it triggers http_err and
>
> "I think" is too soft. You have to be sure here.
>
>> then quits out of grub_net_poll_cards early.
>
> Lack of SOB.
>

I must have missed it. Is it now required?

> Have you tested this patch with large/huge number of files to transfer? I have
> a feeling that it can slowdown whole transfer in such cases due to number of
> connects/disconnects. Maybe this feature should be conditional thing.
>

Currently grub will always establish new connection in http_open ->
http_establish, so it should not change anything from grub PoV, but
reduce amount of lingering connections on server. But I would really
like to explore another patch in above bug report - gracefully close
connection when we finished receive data. That said, this one does not
hurt as long as we do not reuse existing connection.

Re: Patch: Improve HTTP time by sending Connection:close

[Daniel Kiper]

On Tue, Nov 15, 2016 at 05:30:28PM +0300, Andrei Borzenkov wrote:
> On Tue, Nov 15, 2016 at 4:21 PM, Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Sat, Nov 12, 2016 at 09:26:17AM -0800, Walter Huf wrote:
> >> GRUB's HTTP module declares support for HTTP/1.1, which defaults to
> >> Connection:keepalive. At the end of the content, the server holds the TCP
> >> connection open waiting for the next request.
> >> It seems that grub_net_poll_cards() is watching for the HTTP module to set
> >
> > Do you have a feeling or are you sure? I think that we have to be sure here.
> >
>
> See http://savannah.gnu.org/bugs/?49531 which has more details.

OK but I think that this should be a part of commit meesage.

> >> net->stall, and otherwise waits the full 400ms GRUB_NET_INTERVAL to return
> >> to processing. However, HTTP module only sets that flag in specific
> >> conditions:
> >>
> >>    - parse_line detects that we are at the end of downloading a chunked
> >>    Transfer-Encoding
> >>    - http_err detects a problem with the underlying TCP connection
> >>    - http_receive has queued 20 netbuffer packets for processing
> >>
> >> If the file is small and takes less than 20 packets, and the server is not
> >> using chunked encoding, grub_net_poll_cards() will wait the full 400ms
> >> before continuing to process and finish the file download.
> >>
> >> This patch sets Connection:close, which will tell the server to close the
> >> connection as soon as it has finished sending the file. GRUB closes any
> >> connections that are left open (in http_seek), so it does not change
> >
> > Why in http_seek? Is it correct or not?
> >
> >> performance. When the server disconnects, I think it triggers http_err and
> >
> > "I think" is too soft. You have to be sure here.
> >
> >> then quits out of grub_net_poll_cards early.
> >
> > Lack of SOB.
> >
>
> I must have missed it. Is it now required?

No (probably it should be sooner or later but we have to discuss this
issue separetly), however, nice to have.

> > Have you tested this patch with large/huge number of files to transfer? I have
> > a feeling that it can slowdown whole transfer in such cases due to number of
> > connects/disconnects. Maybe this feature should be conditional thing.
> >
>
> Currently grub will always establish new connection in http_open ->
> http_establish, so it should not change anything from grub PoV, but
> reduce amount of lingering connections on server. But I would really
> like to explore another patch in above bug report - gracefully close
> connection when we finished receive data. That said, this one does not

Looking (shortly) at problem description I am not sure that it will fix
the issue. Though I am not convinced that we should play with keep alive
too. Hence, better/nicer solution is appreciated.

Daniel

Re: Patch: Improve HTTP time by sending Connection:close

[Walter Huf]

[-- Attachment #1: Type: text/plain, Size: 5059 bytes --]

GRUB has a bug where it waits a minimum of 400ms for every file it fetches
over HTTP, unless the server serves it with Transfer-Encoding:chunked or
the file just happens to be split into 20 TCP packets. When using
pxeboot.img built with just pxe and http module (following instructions
from https://www.gnu.org/software/grub/manual/html_node/Network.html), this
causes an initial text menu to take about 7 seconds to load with all the
modules being dynamically fetched.
The SOB (statement of benefit?) of this patch is to fix this bug with the
smallest change to existing data structures and logic.
GRUB specifically checks for existing connections that are attached to file
objects when seeking and closes them. New file requests don't have any
sockets attached, and so it always opens a new connection to fetch new
files. Adding the Connection:close header does not reduce performance.

The alternate patch on that bug ticket is a larger change, involving
changing a (module-internal) data structure. I was also less sure of the
flow of logic in http_receive(), so I did not immediately suggest it. It
seems that the GRUB project is understandably conservative about changes,
so I first provided the change that would have the smallest side-effect.

I couched my phrasing with "I believe" and "I think" because I am not sure
if I've fully understood the 3000+ lines of undocumented C code in net.c,
netbuff.c, http.c, and tcp.c in only one week. I submitted this bug to
bring a performance problem to your attention, and included a possible
patch that fixes it for me, in the hope that someone on the project who is
familiar with this code can review it and offer feedback.

If the suggestion is to instead implement a more complete implementation of
Connection:keepalive, I can try that out and offer a rough patch to improve
that. As a newcomer to this project, I would feel more comfortable
contributing a small change to fix the problem immediately instead of a
large possibly-breaking change.

Thank you!

On Tue, Nov 15, 2016 at 12:44 PM, Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Nov 15, 2016 at 05:30:28PM +0300, Andrei Borzenkov wrote:
> > On Tue, Nov 15, 2016 at 4:21 PM, Daniel Kiper <dkiper@net-space.pl>
> wrote:
> > > On Sat, Nov 12, 2016 at 09:26:17AM -0800, Walter Huf wrote:
> > >> GRUB's HTTP module declares support for HTTP/1.1, which defaults to
> > >> Connection:keepalive. At the end of the content, the server holds the
> TCP
> > >> connection open waiting for the next request.
> > >> It seems that grub_net_poll_cards() is watching for the HTTP module
> to set
> > >
> > > Do you have a feeling or are you sure? I think that we have to be sure
> here.
> > >
> >
> > See http://savannah.gnu.org/bugs/?49531 which has more details.
>
> OK but I think that this should be a part of commit meesage.
>
> > >> net->stall, and otherwise waits the full 400ms GRUB_NET_INTERVAL to
> return
> > >> to processing. However, HTTP module only sets that flag in specific
> > >> conditions:
> > >>
> > >>    - parse_line detects that we are at the end of downloading a
> chunked
> > >>    Transfer-Encoding
> > >>    - http_err detects a problem with the underlying TCP connection
> > >>    - http_receive has queued 20 netbuffer packets for processing
> > >>
> > >> If the file is small and takes less than 20 packets, and the server
> is not
> > >> using chunked encoding, grub_net_poll_cards() will wait the full 400ms
> > >> before continuing to process and finish the file download.
> > >>
> > >> This patch sets Connection:close, which will tell the server to close
> the
> > >> connection as soon as it has finished sending the file. GRUB closes
> any
> > >> connections that are left open (in http_seek), so it does not change
> > >
> > > Why in http_seek? Is it correct or not?
> > >
> > >> performance. When the server disconnects, I think it triggers
> http_err and
> > >
> > > "I think" is too soft. You have to be sure here.
> > >
> > >> then quits out of grub_net_poll_cards early.
> > >
> > > Lack of SOB.
> > >
> >
> > I must have missed it. Is it now required?
>
> No (probably it should be sooner or later but we have to discuss this
> issue separetly), however, nice to have.
>
> > > Have you tested this patch with large/huge number of files to
> transfer? I have
> > > a feeling that it can slowdown whole transfer in such cases due to
> number of
> > > connects/disconnects. Maybe this feature should be conditional thing.
> > >
> >
> > Currently grub will always establish new connection in http_open ->
> > http_establish, so it should not change anything from grub PoV, but
> > reduce amount of lingering connections on server. But I would really
> > like to explore another patch in above bug report - gracefully close
> > connection when we finished receive data. That said, this one does not
>
> Looking (shortly) at problem description I am not sure that it will fix
> the issue. Though I am not convinced that we should play with keep alive
> too. Hence, better/nicer solution is appreciated.
>
> Daniel
>

[-- Attachment #2: Type: text/html, Size: 6366 bytes --]

Re: Patch: Improve HTTP time by sending Connection:close

[Andrei Borzenkov]

16.11.2016 00:43, Walter Huf пишет:
> The SOB (statement of benefit?) of this patch

It is "Signed-off-by: xxx" line that are required by some projects
(first appeared in Linux kernel I think). OTOH some projects actively
reject it.

Re: Patch: Improve HTTP time by sending Connection:close

[Daniel Kiper]

On Tue, Nov 15, 2016 at 01:43:16PM -0800, Walter Huf wrote:
> GRUB has a bug where it waits a minimum of 400ms for every file it fetches
> over HTTP, unless the server serves it with Transfer-Encoding:chunked or
> the file just happens to be split into 20 TCP packets. When using
> pxeboot.img built with just pxe and http module (following instructions
> from https://www.gnu.org/software/grub/manual/html_node/Network.html), this
> causes an initial text menu to take about 7 seconds to load with all the
> modules being dynamically fetched.

I agree that this looks like a bug.

> The SOB (statement of benefit?) of this patch is to fix this bug with the
> smallest change to existing data structures and logic.

I meant Signed-off-by: ...
In your case it should look like:

Signed-off-by: Walter Huf <hufman@gmail.com>

> GRUB specifically checks for existing connections that are attached to file
> objects when seeking and closes them. New file requests don't have any
> sockets attached, and so it always opens a new connection to fetch new
> files. Adding the Connection:close header does not reduce performance.

I am not sure that I correctly understand what is written above but it seems
to me that we have a problem with HTTP connection close. So, I think that
there are two ways to fix it:
  - if server accepts Connection:keepalive then we should use existing
    connection as long as possible and transfer all/(maximum number of?)
    files,
  - if it does not then we should close HTTP connection immediately
    after file transfer and open another one for a new transfer.

> The alternate patch on that bug ticket is a larger change, involving
> changing a (module-internal) data structure. I was also less sure of the
> flow of logic in http_receive(), so I did not immediately suggest it. It
> seems that the GRUB project is understandably conservative about changes,
> so I first provided the change that would have the smallest side-effect.

Patch size is important factor but not crucial one. First of all it
should properly fix a given bug and do not introduce regressions.

> I couched my phrasing with "I believe" and "I think" because I am not sure
> if I've fully understood the 3000+ lines of undocumented C code in net.c,
> netbuff.c, http.c, and tcp.c in only one week. I submitted this bug to

I prefer to spend more time on code/functionality analysis than provide fix which
is based on vague imagination. So, that is why I am asking for more details.

> bring a performance problem to your attention, and included a possible

Thanks for doing that!

> patch that fixes it for me, in the hope that someone on the project who is
> familiar with this code can review it and offer feedback.

I think that we are doing that right now.

> If the suggestion is to instead implement a more complete implementation of
> Connection:keepalive, I can try that out and offer a rough patch to improve

I am not convinced that we should play with Connection:keepalive.
Please look above.

> that. As a newcomer to this project, I would feel more comfortable
> contributing a small change to fix the problem immediately instead of a
> large possibly-breaking change.

As I said earlier, size of a given patch is not very important (at least for me).
It have to be correct. So, please dive into the code and understand how it works.
If you are not sure send questions to the list then somebody familiar with a given
chunk will try to explain what is going on. We are happy to help. Though you must
put some effort to provide correct patch too. Otherwise it does not work.

Daniel

Re: Patch: Improve HTTP time by sending Connection:close

[Konrad Rzeszutek Wilk]

On Thu, Nov 17, 2016 at 07:32:16PM +0100, Daniel Kiper wrote:
> On Tue, Nov 15, 2016 at 01:43:16PM -0800, Walter Huf wrote:
> > GRUB has a bug where it waits a minimum of 400ms for every file it fetches
> > over HTTP, unless the server serves it with Transfer-Encoding:chunked or
> > the file just happens to be split into 20 TCP packets. When using
> > pxeboot.img built with just pxe and http module (following instructions
> > from https://www.gnu.org/software/grub/manual/html_node/Network.html), this
> > causes an initial text menu to take about 7 seconds to load with all the
> > modules being dynamically fetched.
> 
> I agree that this looks like a bug.
> 
> > The SOB (statement of benefit?) of this patch is to fix this bug with the
> > smallest change to existing data structures and logic.
> 
> I meant Signed-off-by: ...
> In your case it should look like:
> 
> Signed-off-by: Walter Huf <hufman@gmail.com>

It is not as simple as that. Putting your SoB means that you:

        Developer's Certificate of Origin 1.1

        By making a contribution to this project, I certify that:

        (a) The contribution was created in whole or in part by me and I
            have the right to submit it under the open source license
            indicated in the file; or

        (b) The contribution is based upon previous work that, to the best
            of my knowledge, is covered under an appropriate open source
            license and I have the right under that license to submit that
            work with modifications, whether created in whole or in part
            by me, under the same open source license (unless I am
            permitted to submit under a different license), as indicated
            in the file; or

        (c) The contribution was provided directly to me by some other
            person who certified (a), (b) or (c) and I have not modified
            it.

        (d) I understand and agree that this project and the contribution
            are public and that a record of the contribution (including all
            personal information I submit with it, including my sign-off) is
            maintained indefinitely and may be redistributed consistent with
            this project or the open source license(s) involved.

then you just add a line saying

	Signed-off-by: Random J Developer <random@developer.example.org>