Date: Tue, 16 Dec 2008 20:12:46 -0800 From: "Peter Wemm" <peter@wemm.org> To: "Bjoern A. Zeeb" <bz@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r186057 - head/sys/netinet Message-ID: <e7db6d980812162012w1f98f3f9kc4383823a5b62482@mail.gmail.com> In-Reply-To: <200812132159.mBDLxIQv040799@svn.freebsd.org> References: <200812132159.mBDLxIQv040799@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Dec 13, 2008 at 1:59 PM, Bjoern A. Zeeb <bz@freebsd.org> wrote:
> De-virtualize the MD5 context for TCP initial seq number generation
> and make it a function local variable like we do almost everywhere
> inside the kernel.
[..]
> --- head/sys/netinet/vinet.h Sat Dec 13 21:17:46 2008 (r186056)
> +++ head/sys/netinet/vinet.h Sat Dec 13 21:59:18 2008 (r186057)
> @@ -142,7 +142,6 @@ struct vnet_inet {
> int _isn_last_reseed;
> u_int32_t _isn_offset;
> u_int32_t _isn_offset_old;
> - MD5_CTX _isn_ctx;
>
> struct inpcbhead _udb;
> struct inpcbinfo _udbinfo;
I'm bitterly unhappy with this. Every time these structs are touched,
either directly or indirectly, there is a guaranteed ABI breakage with
kernel modules.
There needs to be a __FreeBSD_version bump (or something similar)
every time any of these structures change, and any kernel modules
*must* be prevented from loading. It can't be a >= some version, it
has to be an exact match.
With the global variable method the linker calculates the offsets at
load time. With this abomination, the knowledge of the structure
layout is compiled into the generated code with no chance of a fixup.
There are no sanity checks. If a module that referenced _isn_ctx is
loaded the old way, there would be a link error. The new way will
have it silently trash _udb instead.
There is a whole world of hurt being unleashed here. I suspect that
we might even be possible to run out of digits in our
__FreeBSD_version numbering scheme.
I know we've talked about ABI-stable alternatives after the
infrastructure is done, but it needs to be absolutely clear that
touching this structure in the current form is a guaranteed ABI break,
and is currently undetected. If you boot kernel.old, you're hosed.
(Again, with -current, this might be ok for a while, but this scheme
won't wash with ports or other 3rd party modules)
BTW; the reason why _isn_ctx was global is because it is a non-trivial
size. It might only be 88 bytes, but when you've only got a few KB of
stack and lots of nesting, this stuff adds up pretty quickly. We were
right on the edge of stack overflows for quite some time and had to go
and hunt down this sort of thing. In this case it is probably
harmless, but we must not get into the habit of casually discarding
previous hard-won space savings.
In the mean time, I'd like to see some compile-time asserts in there
to make sure there are no accidental size changes of this structure.
--
Peter Wemm - peter@wemm.org; peter@FreeBSD.org; peter@yahoo-inc.com; KI6FJV
"All of this is for nothing if we don't go to the stars" - JMS/B5
"If Java had true garbage collection, most programs would delete
themselves upon execution." -- Robert Sewell
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?e7db6d980812162012w1f98f3f9kc4383823a5b62482>