Bug 395 - All spl functions implemented incorrectly in both FreeBSD and NetBSD
Summary: All spl functions implemented incorrectly in both FreeBSD and NetBSD
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: i386 (show other bugs)
Version: Unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-bugs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 1995-05-11 16:40 UTC by Tatu Ylonen
Modified: 1995-05-11 16:40 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tatu Ylonen 1995-05-11 16:40:01 UTC
	Spl functions (splbio, splclock, splhigh, splimp, splnet,
	splsoftclock, splsofttty, splstatclock, spltty, spl0, splx)
	are defined in /usr/src/sys/i386/include/spl.h as inline
	functions that modify the ordinary variable cpl (extern
	unsigned cpl in the same header).

	This means that the compiler has full knowledge about the
	semantics, side-effects, and dependencies of these functions.
	The compiler inlines these functions, and can mix (reorder) the
	resulting code with other code in the function that calls the
	spl function.
	  - The value may not get actually written to cpl until the
	    end of the function calling the spl function, or when a
	    function with unknown dependencies is called.  This may be
	    *after* the protected code is executed, or the compiler
	    might even entirely optimize the code out because splx
	    assigns the value later in the function.
	  - It would not help to simply make cpl volatile, because the
	    compiler would still be allowed to cache intermediate
	    values or results of common subexpressions in registers
	    across the lock/unlock requests.

	A simple solution is to make the spl functions real functions
	stored in a separate file, and only have prototypes in spl.h.
	This way, the compiler does not know anything about the
	function, and will have to assume the worst: it might access
	or modify any memory, which means the compiler will have to
	flush any modifications to memory before the call, and not
	make any assumptions about common subexpressions being
	preserved across the call.  There is a slight performance
	penalty due to the real call, but it is not significant and
	not easy to avoid reliably.

	Also, be aware that if the OS is ever to be ported to a
	multiprocessor, the functions will need to be rewritten.  The
	instructions they now use are not atomic in a multiprocessor
	environment.

Fix: 

Move all spl functions (splbio, splclock, splhigh, splimp,
	splnet, splsoftclock, splsofttty, splstatclock, spltty, spl0,
	splx) away from spl.h, make them real (global) functions,
	and leave only prototypes in spl.h.  It might not be a bad
	idea to make cpl volatile to be sure; I am not sure if it
	is necessary.

	My quick fix is as follows; someone else can do it more
	cleanly (I think the spl code could be in a separate .c file,
	but I simply stored it in a random .c file).

	i386/include/spl.h:
	  - Move the GENSPL macro and its uses, and spl0 and splx to
	    i386/386/sys_machdep.c.  Edit the macro and functions to
	    make them normal non-static, non-inline functions.
	  - Add the following prototypes:
		int splbio(void);
		int splclock(void);
		int splhigh(void);
		int splimp(void);
		int splnet(void);
		int splsoftclock(void);
		int splsofttty(void);
		int splstatclock(void);
		int spltty(void);
		void spl0(void);
		void splx(int old);

	i386/i386/sys_machdep.c:
	  - I moved the spl code here.  I think it would be cleaner to
	    have a separate .c file for it.
How-To-Repeat: 
	The real effects of this problem are not predictable or
	deterministic.  They may depend on compiler version or
	optimization levels.  General unreliability, mysterious
	problems, and random panics are all likely.  I started to
	wonder about this because sysv message queues were losing
	messages (that's another story - I'll write about it
	later).  This could also explain inode lockups problems I
	had with an earlier release.

	Actually, I am surprised the kernel works at all, even fairly
	well.
Comment 1 davidg freebsd_committer freebsd_triage 1995-05-22 10:54:14 UTC
State Changed
From-To: open->closed

Bruce says that gcc's behavior for inlined functions isn't any 
different than it is for real functions WRT caching of variables 
or intermediates...and an analysis of the generated code seems 
to confirm this. It appears to be a non-problem with the current 
gcc.