Date: Thu, 11 May 1995 23:06:22 -0700 From: William Maddox <maddox@CS.Berkeley.EDU> To: Tatu Ylonen <ylo@fx7.cs.hut.fi> Cc: freebsd-bugs@freefall.cdrom.com, maddox@redwood.CS.Berkeley.EDU Subject: Re: i386/395: CRITICAL PROBLEM: spl functions implemented incorrectly Message-ID: <199505120606.XAA13119@redwood.CS.Berkeley.EDU> In-Reply-To: Your message of "Thu, 11 May 1995 16:40:02 PDT." <199505112340.QAA19464@freefall.cdrom.com>
index | next in thread | previous in thread | raw e-mail
You recently reported a bug in "spl.h" :
> 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.
...
>Fix:
...
> 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.
....
I think there is another potential problem lurking in spl.h, in the following:
/*
* ipending has to be volatile so that it is read every time it is accessed
* in splx() and spl0(), but we don't want it to be read nonatomically when
* it is changed. Pretending that ipending is a plain int happens to give
* suitable atomic code for "ipending |= constant;".
*/
#define setdelayed() (*(unsigned *)&ipending |= loadandclear(&idelayed))
#define setsoftast() (*(unsigned *)&ipending |= SWI_AST_PENDING)
#define setsoftclock() (*(unsigned *)&ipending |= SWI_CLOCK_PENDING)
#define setsoftnet() (*(unsigned *)&ipending |= SWI_NET_PENDING)
#define setsofttty() (*(unsigned *)&ipending |= SWI_TTY_PENDING)
I assume that the remark above regarding "suitable atomic code" means
that the bits are set in place in memory, using a single instruction.
Just because GCC happens to do this today doesn't mean it will tomorrow.
As the internal execution mechanisms of new-generation x86 processors become
more heavily parallel and RISC-like, instruction sequences involving
architecturally
unnecessary register temporaries may turn out to be faster. I wouldn't bet on
the stability
of a compiler's choice of instructions in future releases, and I doubt that
anyone is
going to check this each time a new version of GCC is integrated into FreeBSD.
I would suggest defining these functions as in-line assembly-language
functions.
In any case, this is the sort of thing that should be done in assembler, not C.
--Bill
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199505120606.XAA13119>