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>
index | next in thread | previous in thread | raw e-mail
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
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120225052404.M2440>