Date: Sat, 25 Feb 2012 05:40:26 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Gleb Smirnoff <glebius@freebsd.org> Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r232044 - projects/pf/head/sys/sys Message-ID: <20120225052404.M2440@besplex.bde.org> In-Reply-To: <20120223191032.GW92625@FreeBSD.org> References: <201202231019.q1NAJObb099152@svn.freebsd.org> <20120224034735.T1834@besplex.bde.org> <20120223191032.GW92625@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120225052404.M2440>