nnrpd and TCP nagle

[Julien ÉLIE]

Hi all,

>> I've been debugging a slow reader issue with a friend and we've found
>> that when his tin does a "listgroup", each and every article number is
>> going over the wire in its own packet. That, of course, is pretty
>> inefficient.
> 
>> I've found the following in nnrpd.c (line 614):
> 
>>      /* Setting TCP_NODELAY to nnrpd fixes a problem of slow downloading
>>       * of overviews and slow answers on some architectures (like BSD/OS). */
>>      setsockopt(STDIN_FILENO, IPPROTO_TCP, TCP_NODELAY, &nodelay, sizeof(nodelay));
> 
>> Which, when removed, makes for way better performance, about an order
>> of magnitude down in number of packets and time for the group we've
>> been testing with.
> 
>> The last time I ran INN on BSD/OS was 20 years ago, and BSD/OS has
>> been discontinued in 2003, which indicates this line is somewhat
>> historic. And for NNTP TCP_NODELAY makes no sense in the first place.
> 
>> So I suggest removing this.
> 
> Yup, I agree.  It's almost never a good idea to tweak stuff like that with
> a modern networking stack.  The kernel is probably going to be smarter
> about aggregation than we're going to be.

Couldn't we just keep it for BSD-like systems?
It was a suggestion in the thread that led to that addition in nnrpd:
    http://permalink.gmane.org/gmane.network.inn/8642

The Doctor is notably still using a BSD-like system and reported that the
patch made INN way faster on his system.


Suggestion of patch:


Index: configure.ac
===================================================================
--- configure.ac	(révision 9984)
+++ configure.ac	(copie de travail)
@@ -203,6 +203,12 @@
         [Define if compiling on Linux to get prototypes for some functions.])
     ;;
 
+dnl Detect BSD-based systems for later use in nnrpd code.
+*BSD*)
+    AC_DEFINE([INN_BSD], [1],
+        [Define if compiling on BSD-based systems.])
+    ;;
+
 dnl HP-UX's native compiler needs a special flag to turn on ANSI, and needs
 dnl -g on link as well as compile for debugging to work.
 *hpux*)
Index: nnrpd/nnrpd.c
===================================================================
--- nnrpd/nnrpd.c	(révision 9984)
+++ nnrpd/nnrpd.c	(copie de travail)
@@ -13,7 +13,9 @@
 #include "portable/socket.h"
 #include <netdb.h>
 #include <signal.h>
-#include <netinet/tcp.h>
+#if defined(INN_BSD)
+# include <netinet/tcp.h>
+#endif
 
 #if HAVE_GETSPNAM
 # include <shadow.h>
@@ -579,7 +581,6 @@
     struct sockaddr *sas = (struct sockaddr *) &sss;
     socklen_t length;
     size_t size;
-    int nodelay = 1;
 
     memset(&Client, 0, sizeof(Client));
     strlcpy(Client.host, "?", sizeof(Client.host));
@@ -648,9 +649,12 @@
         Client.serverport = network_sockaddr_port(sas);
     }
 
+#if defined(INN_BSD)
     /* Setting TCP_NODELAY to nnrpd fixes a problem of slow downloading
      * of overviews and slow answers on some architectures (like BSD/OS). */
+    int nodelay = 1;
     setsockopt(STDIN_FILENO, IPPROTO_TCP, TCP_NODELAY, &nodelay, sizeof(nodelay));
+#endif
 
     notice("%s (%s) connect - port %u", Client.host, Client.ip, port);
 


-- 
Julien ÉLIE

« – Depuis quelque temps, Zérozérosix n'attire plus les mouches !
  – Espérons qu'il ne nous attirera pas d'ennuis ! » (Astérix)