[gnutls-devel] [PATCH] Re: TCP Fast Open

[Tim Ruehsen]

On Friday, July 15, 2016 9:21:03 AM CEST Nikos Mavrogiannopoulos wrote:
> On Thu, Jul 14, 2016 at 12:45 PM, Tim Ruehsen <tim.ruehsen at gmx.de> wrote:
> > Here is my patch, first version.
> > 
> > Please review and comment.
> > 
> > - I have no platform without TFO to test with... so compilation might
> > break on such platforms, could anyone give it a try ?
> 
> I've committed it on a special branch and run through the CI. The
> windows and freebsd builds fail:
> https://gitlab.com/gnutls/gnutls/builds/2370926
> https://gitlab.com/gnutls/gnutls/builds/2370929

I haven't taken care of HAVE_WRITEV in _gnutls_writev(). The amended patch 
should do that. Please test again.

> > - Please check the docs... I am unsure about completeness
> 
> It would be best to mention that optimization in the manual, while
> also discussing other optimizations such as false start and session
> resumption. I could take care of that.

That would be great !

> > - To check the timings / test TFO, I added 'return 0;' in line 1245 of
> > src/
> > cli.c and run it with and without --fastopen against www.google.com 443.
> > I have ~35ms ping here and had a reproducable gain of ~32ms when using --
> > fastopen (152ms vs. 120ms).
> 
> That's quite nice. Have you noticed firewalls or middle boxes blocking
> that approach? Is there anything we can fix for them or fallback?

Not that I know of. A fallback should IMO applied at the application layer.

But there is one thing that should be mentioned. The client applications 
(currently only gnutls-cli) need a different retry strategy with TFO.

Without TFO, socket.c/socket_open() loops over the addrinfo list returned by 
getaddrinfo() until connect() returns success. 

With TFO, we return with the first successful call to socket(). The implicit 
connect happens later - and from there we currently can't easily 'continue 
looping'. Of course we could (we still have 'res' and 'ptr' in the socket_st), 
but that needs some refactoring of socket.c - not part of this patch.

> > @@ -496,8 +496,17 @@ _gnutls_writev(gnutls_session_t session, const
> > giovec_t * giovec,> 
> >         }
> >         
> >         if (no_writev == 0) {
> > 
> > -            i = session->internals.vec_push_func(fd, giovec,
> > -                                 giovec_cnt);
> > +            if (session->internals.connect_addr) {
> > +                if (session->internals.vec_push_func == system_writev)
> > +                    i = system_writev_tfo(session, giovec, giovec_cnt);
> > +#ifdef MSG_NOSIGNAL
> > +                else if (session->internals.vec_push_func ==
> > system_writev_nosignal) +                    i =
> > system_writev_nosignal_tfo(session, giovec, giovec_cnt); +#endif
> > +                else
> > +                    i = session->internals.vec_push_func(fd, giovec,
> > giovec_cnt); +            } else
> > +                i = session->internals.vec_push_func(fd, giovec,
> > giovec_cnt);
> This path is a bit worrying. Why not have
> gnutls_transport_set_fastopen() replace all the pull/push functions
> with its own version?

True, but the new writev_tfo functions need a session pointer instead of a 
transport pointer. And I didn't want to take care for user supplied transport 
pointers + user defined write functions (set before or after 
gnutls_transport_set_fastopen()).
But I see room for a internal refactoring, that uses session for all system 
write/writev funtions... and call user supplied functions from there. IMO, 
that should be another patch.

> A test program utilizing this code path should also be added to avoid
> breaking that functionality in the future.

It needs client+server connected via TCP to test that code path. Can you 
propose any test that is best to start with ?

Tim
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Support-TCP-Fast-Open.patch
Type: text/x-patch
Size: 16527 bytes
Desc: not available
URL: </pipermail/attachments/20160715/668d3338/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: </pipermail/attachments/20160715/668d3338/attachment-0001.sig>