On Tue, Jul 8, 2014 at 1:29 AM, Mark Kettenis <mark.kette...@xs4all.nl> wrote: >> Date: Mon, 7 Jul 2014 13:46:03 -0700 >> From: Matthew Dempsky <matt...@dempsky.org> >> >> 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.
