From: Dave Hart Date: Fri, 19 Mar 2010 04:09:05 +0000 (+0000) Subject: Fine-tune "ntpq -c mrulimit" row limit backoff and growth after X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=26e1f6223052fe63708c71b5cc09dec0818ab234;p=thirdparty%2Fntp.git Fine-tune "ntpq -c mrulimit" row limit backoff and growth after testing over WiFi and a lossy go6.net UDP tunnel. bk: 4ba2f8e1FmMPWvksME6yKwGxWZ7sNw --- diff --git a/include/ntp.h b/include/ntp.h index 9f2824f185..8abd5a9cb4 100644 --- a/include/ntp.h +++ b/include/ntp.h @@ -880,4 +880,6 @@ struct endpoint { #define NETINFO_CONFIG_DIR "/config/ntp" #endif +/* ntpq -c mrulist rows per request limit in ntpd */ +#define MRU_ROW_LIMIT 256 #endif /* NTP_H */ diff --git a/libntp/authusekey.c b/libntp/authusekey.c index ba7a0a18d9..c1d08132c3 100644 --- a/libntp/authusekey.c +++ b/libntp/authusekey.c @@ -23,14 +23,12 @@ authusekey( const u_char *str ) { - const u_char *cp; int len; - cp = str; - len = strlen((const char *)cp); - if (len == 0) + len = strlen((const char *)str); + if (0 == len) return 0; - MD5auth_setkey(keyno, keytype, str, (int)strlen((const char *)str)); + MD5auth_setkey(keyno, keytype, str, len); return 1; } diff --git a/ntpd/ntp_control.c b/ntpd/ntp_control.c index aca88d4e51..34042d7c2f 100644 --- a/ntpd/ntp_control.c +++ b/ntpd/ntp_control.c @@ -2995,8 +2995,8 @@ static void read_mru_list( } free_varlist(in_parms); in_parms = NULL; - if (!(0 < limit && limit <= 256)) { - ctl_error(CERR_BADFMT); + if (!(0 < limit && limit <= MRU_ROW_LIMIT)) { + ctl_error(CERR_BADVALUE); return; } diff --git a/ntpq/ntpq-subs.c b/ntpq/ntpq-subs.c index 6206ba63f8..3e7ae60c00 100644 --- a/ntpq/ntpq-subs.c +++ b/ntpq/ntpq-subs.c @@ -2252,6 +2252,7 @@ collect_mru_list( l_fp * pnow ) { + static int ntpd_row_limit = MRU_ROW_LIMIT; int c_mru_l_rc; /* this function's return code */ u_char got; /* MRU_GOT_* bits */ const char ts_fmt[] = "0x%08x.%08x"; @@ -2280,7 +2281,7 @@ collect_mru_list( l_fp last_older; sockaddr_u addr_older; int have_now; - int have_addr_older; + int have_addr_older; int have_last_older; u_short hash; mru *unlinked; @@ -2302,7 +2303,7 @@ collect_mru_list( memset(pnow, 0, sizeof(*pnow)); memset(&last_older, 0, sizeof(last_older)); - limit = 32; + limit = min(3 * MAXFRAGS, ntpd_row_limit); snprintf(req_buf, sizeof(req_buf), "limit=%d%s", limit, parms); while (TRUE) { @@ -2335,6 +2336,10 @@ collect_mru_list( NTP_INSIST(unlinked == recent); free(recent); } + } else if (CERR_BADVALUE == qres /* temp -> */ || CERR_BADFMT == qres /* <- temp */) { + /* ntpd has lower cap on row limit */ + ntpd_row_limit--; + limit = min(limit, ntpd_row_limit); } else if (ERR_INCOMPLETE == qres) { /* * Reduce the number of rows to minimize effect @@ -2536,10 +2541,12 @@ collect_mru_list( * If there were no errors, increase the number of rows * to a maximum of 3 * MAXFRAGS (the most packets ntpq * can handle in one response), on the assumption that - * no less than 3 rows fit in each packet. + * no less than 3 rows fit in each packet, capped at + * our best guess at the server's row limit. */ if (!qres) - limit = min(3 * MAXFRAGS, limit * 3 / 2); + limit = min3(3 * MAXFRAGS, ntpd_row_limit, + max(limit + 1, limit * 33 / 32)); /* * prepare next query with as many address and last-seen * timestamps as will fit in a single packet. diff --git a/ntpq/ntpq.c b/ntpq/ntpq.c index c20eff7639..42d5301523 100644 --- a/ntpq/ntpq.c +++ b/ntpq/ntpq.c @@ -370,8 +370,18 @@ struct xcmd builtins[] = { * Default values we use. */ #define DEFHOST "localhost" /* default host name */ -#define DEFTIMEOUT (5) /* 5 second time out */ -#define DEFSTIMEOUT (2) /* 2 second time out after first */ +#define DEFTIMEOUT 5 /* wait 5 seconds for 1st pkt */ +#define DEFSTIMEOUT 3 /* and 3 more for each additional */ +/* + * Requests are automatically retried once, so total timeout with no + * response is a bit over 2 * DEFTIMEOUT, or 10 seconds. At the other + * extreme, a request eliciting 32 packets of responses each for some + * reason nearly DEFSTIMEOUT seconds after the prior in that series, + * with a single packet dropped, would take around 32 * DEFSTIMEOUT, or + * 93 seconds to fail each of two times, or 186 seconds. + * Some commands involve a series of requests, such as "peers" and + * "mrulist", so the cumulative timeouts are even longer for those. + */ #define DEFDELAY 0x51EB852 /* 20 milliseconds, l_fp fraction */ #define LENHOSTNAME 256 /* host name is 256 characters long */ #define MAXCMDS 100 /* maximum commands on cmd line */ @@ -851,17 +861,17 @@ getresponse( /* * Loop until we have an error or a complete response. Nearly all - * aths to loop again use continue. + * code paths to loop again use continue. */ for (;;) { if (numfrags == 0) - tvo = tvout; + tvo = tvout; else - tvo = tvsout; + tvo = tvsout; FD_SET(sockfd, &fds); - n = select(sockfd+1, &fds, (fd_set *)0, (fd_set *)0, &tvo); + n = select(sockfd + 1, &fds, NULL, NULL, &tvo); if (n == -1) { warning("select fails", "", ""); @@ -873,24 +883,30 @@ getresponse( */ if (numfrags == 0) { if (timeo) - (void) fprintf(stderr, - "%s: timed out, nothing received\n", - currenthost); + fprintf(stderr, + "%s: timed out, nothing received\n", + currenthost); return ERR_TIMEOUT; } else { if (timeo) - (void) fprintf(stderr, + fprintf(stderr, "%s: timed out with incomplete data\n", currenthost); if (debug) { - printf("Received fragments:\n"); + fprintf(stderr, + "ERR_INCOMPLETE: Received fragments:\n"); for (n = 0; n < numfrags; n++) - printf("%4d %d\n", offsets[n], - counts[n]); - if (seenlastfrag) - printf("last fragment received\n"); - else - printf("last fragment not received\n"); + fprintf(stderr, + "%2d: %5d %5d\t%3d octets\n", + n, offsets[n], + offsets[n] + + counts[n], + counts[n]); + fprintf(stderr, + "last fragment %sreceived\n", + (seenlastfrag) + ? "" + : "not "); } return ERR_INCOMPLETE; } diff --git a/ntpq/ntpq.h b/ntpq/ntpq.h index da597e19e2..4910ed6e43 100644 --- a/ntpq/ntpq.h +++ b/ntpq/ntpq.h @@ -18,9 +18,25 @@ #define MAXARGS 4 /* - * Limit on packets in a single response + * Limit on packets in a single response. Increasing this value to + * 96 will marginally speed "mrulist" operation on lossless networks + * but it has been observed to cause loss on WiFi networks and with + * an IPv6 go6.net tunnel over UDP. That loss causes the request + * row limit to be cut in half, and it grows back very slowly to + * ensure forward progress is made and loss isn't triggered too quickly + * afterward. While the lossless case gains only marginally with + * MAXFRAGS == 96, the lossy case is a lot slower due to the repeated + * timeouts. Empirally, MAXFRAGS == 32 avoids most of the routine loss + * on both the WiFi and UDP v6 tunnel tests and seems a good compromise. + * This suggests some device in the path has a limit of 32 ~512 byte UDP + * packets in queue. + * Lowering MAXFRAGS may help with particularly lossy networks, but some + * ntpq commands may rely on the longtime value of 24 implicitly, + * assuming a single multipacket response will be large enough for any + * needs. In contrast, the "mrulist" command is implemented as a series + * of requests and multipacket responses to each. */ -#define MAXFRAGS 64 +#define MAXFRAGS 32 /* * Error codes for internal use diff --git a/ports/winnt/include/config.h b/ports/winnt/include/config.h index fc0d55a72a..f521b511fa 100644 --- a/ports/winnt/include/config.h +++ b/ports/winnt/include/config.h @@ -373,7 +373,7 @@ typedef __int32 int32_t; /* define a typedef for int32_t */ #define HAVE_LIMITS_H 1 #define HAVE_STRDUP -#define HAVE_STRCHR +#define HAVE_STRCHR /* for libopts */ #define HAVE_FCNTL_H 1 #define HAVE_SYS_RESOURCE_H #define HAVE_BSD_NICE /* emulate BSD setpriority() */