> Date: Tue, 8 Jul 2014 09:05:38 -0700 > From: Matthew Dempsky <[email protected]> > > On Tue, Jul 8, 2014 at 1:29 AM, Mark Kettenis <[email protected]> wrote: > >> Date: Mon, 7 Jul 2014 13:46:03 -0700 > >> From: Matthew Dempsky <[email protected]> > >> > >> p_sigmask is only modified by the owning thread from process context > >> (e.g., sys_sigprocmask(), sys_sigreturn(), userret(), or postsig()), > >> but it can be accessed anywhere (e.g., interrupts or threads on other > >> CPUs). Currently sys_sigprocmask() protects p_sigmask with splhigh(), > >> but that's not MP-safe. > >> > >> The simpler solution is to take advantage of our atomics APIs. > >> Unfortunately for implementing SIG_SETMASK, we don't have an > >> atomic_store_int() function, and I can't bring myself to abuse > >> "volatile" for this purpose, so I've resorted to atomic_swap_uint(). > > > > Sory, but I think that's wrong. You need volatile to make sure the > > mask is read from memory when you're checking bits. Or you need to > > insert the proper memory barriers I think. > > To be clear: I meant I don't want to abuse "volatile" as though it's a > magic "make-these-operations-**atomic**" flag, as that's not what it > really means (even if in practice it often has that effect). > > Also, as long as the (always current thread) mutators and cross-thread > accessors are still serialized by the kernel lock, marking p_sigmask > as volatile shouldn't be necessary. However, I do agree that once we > start unlocking any of them (which is a future goal of this work), we > need some sort of atomic guarantee on the read side too, and marking > p_sigmask as volatile seems like a reasonable first step. (I'd > probably go further and also decorate the accesses with C11-style > atomic_load()s.) > > So I'm happy to mark p_sigmask as volatile with this diff if you'd > prefer. I just don't think it's strictly necessary yet, but I'm not > opposed to it either.
Even if the kernel lock protects us now, I think not adding volatile would cause nasty surprises in the future.
