From owner-svn-src-projects@FreeBSD.ORG Fri Feb 24 18:40:29 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9F6BD106564A; Fri, 24 Feb 2012 18:40:29 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail12.syd.optusnet.com.au (mail12.syd.optusnet.com.au [211.29.132.193]) by mx1.freebsd.org (Postfix) with ESMTP id 3920C8FC0A; Fri, 24 Feb 2012 18:40:28 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail12.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q1OIeQnS000990 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 25 Feb 2012 05:40:27 +1100 Date: Sat, 25 Feb 2012 05:40:26 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Gleb Smirnoff In-Reply-To: <20120223191032.GW92625@FreeBSD.org> Message-ID: <20120225052404.M2440@besplex.bde.org> References: <201202231019.q1NAJObb099152@svn.freebsd.org> <20120224034735.T1834@besplex.bde.org> <20120223191032.GW92625@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org, Bruce Evans Subject: Re: svn commit: r232044 - projects/pf/head/sys/sys X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Feb 2012 18:40:29 -0000 On Thu, 23 Feb 2012, Gleb Smirnoff wrote: > On Fri, Feb 24, 2012 at 04:30:53AM +1100, Bruce Evans wrote: > B> [... should use FreeBSD kernel API for timevalsub(),..] > Thanks for important comments. Is the fix in r232062 okay? It is short enough (not many places to change), but seems to unimprove the variable names. % Modified: projects/pf/head/sys/contrib/pf/net/pf_norm.c % ============================================================================== % --- projects/pf/head/sys/contrib/pf/net/pf_norm.c Thu Feb 23 18:59:32 2012 (r232061) % +++ projects/pf/head/sys/contrib/pf/net/pf_norm.c Thu Feb 23 19:03:22 2012 (r232062) % @@ -1670,7 +1670,6 @@ pf_normalize_tcp_stateful(struct mbuf *m % * connection limit until we can come up with a better % * lowerbound to the TS echo check. % */ % - struct timeval delta_ts; % int ts_fudge; % % % @@ -1686,9 +1685,9 @@ pf_normalize_tcp_stateful(struct mbuf *m % /* Calculate max ticks since the last timestamp */ % #define TS_MAXFREQ 1100 /* RFC max TS freq of 1Khz + 10% skew */ % #define TS_MICROSECS 1000000 /* microseconds per second */ % - timersub(&uptime, &src->scrub->pfss_last, &delta_ts); % - tsval_from_last = (delta_ts.tv_sec + ts_fudge) * TS_MAXFREQ; % - tsval_from_last += delta_ts.tv_usec / (TS_MICROSECS/TS_MAXFREQ); % + timevalsub(&uptime, &src->scrub->pfss_last); % + tsval_from_last = (uptime.tv_sec + ts_fudge) * TS_MAXFREQ; % + tsval_from_last += uptime.tv_usec / (TS_MICROSECS/TS_MAXFREQ); % % if ((src->state >= TCPS_ESTABLISHED && % dst->state >= TCPS_ESTABLISHED) && The FreeBSD kernel API is unfortunately not compatible -- its takes 2 parameters instead of 3, with the target timeval being an input-output parameter. This change seems to abuse the `uptime' variable for a delta-time that is far from being an uptime. To write it clearly, I think using the 2-parameter API requires an extra statement: delta_ts = uptime; /* although it is not yet a delta */ timevalsub(&delta_ts, &src->scrub->pfss_last); % @@ -1712,8 +1711,8 @@ pf_normalize_tcp_stateful(struct mbuf *m % DPFPRINTF((" tsval: %u tsecr: %u +ticks: %u " % "idle: %jus %lums\n", % tsval, tsecr, tsval_from_last, % - (uintmax_t)delta_ts.tv_sec, % - delta_ts.tv_usec / 1000)); % + (uintmax_t)uptime.tv_sec, % + uptime.tv_usec / 1000)); At least keeping the old variable avoids the need to change this. The 3-parameter form is sometimes more convenient, but not really needed. CPU architectures often only support only 2-parameter operations because the bloat for the 3-parameter form is larger. I don't like large APIs, so I will consider only supporting the 2-parameter form a feature :-). In software, the compiler can optimize away the extra assignment much more easily than CPU hardware can do it. Actually, this optimization is probably now routine for hardware too. Bruce