From owner-freebsd-bugs Thu May 11 23:03:47 1995 Return-Path: bugs-owner Received: (from majordom@localhost) by freefall.cdrom.com (8.6.10/8.6.6) id XAA07090 for bugs-outgoing; Thu, 11 May 1995 23:03:47 -0700 Received: from redwood.CS.Berkeley.EDU (redwood.CS.Berkeley.EDU [128.32.36.44]) by freefall.cdrom.com (8.6.10/8.6.6) with ESMTP id XAA07081 for ; Thu, 11 May 1995 23:03:46 -0700 Received: from redwood.CS.Berkeley.EDU (localhost.Berkeley.EDU [127.0.0.1]) by redwood.CS.Berkeley.EDU (8.6.11/8.6.9) with ESMTP id XAA13119; Thu, 11 May 1995 23:06:32 -0700 From: William Maddox Message-Id: <199505120606.XAA13119@redwood.CS.Berkeley.EDU> X-Mailer: exmh version 1.6 4/21/95 To: Tatu Ylonen cc: freebsd-bugs@freefall.cdrom.com, maddox@redwood.CS.Berkeley.EDU Subject: Re: i386/395: CRITICAL PROBLEM: spl functions implemented incorrectly In-reply-to: Your message of "Thu, 11 May 1995 16:40:02 PDT." <199505112340.QAA19464@freefall.cdrom.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Date: Thu, 11 May 1995 23:06:22 -0700 Sender: bugs-owner@FreeBSD.org Precedence: bulk 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