On Wed, May 04, 2016 at 03:53:29PM +0200, Peter Zijlstra wrote: > On Wed, May 04, 2016 at 02:39:40PM +0200, Petr Mladek wrote: > > > + * This barrier also ensures that if another CPU goes through the > > > + * syscall barrier, sees the TIF_PATCH_PENDING writes in > > > + * klp_start_transition(), and calls klp_patch_task(), it also sees the > > > + * above write to the target state. Otherwise it can put the task in > > > + * the wrong universe. (oops, missed a "universe" -> "patch state" rename) > > > + */ > > > > By other words, it makes sure that klp_patch_task() will assign the > > right patch_state. Where klp_patch_task() could not be called > > before we set TIF_PATCH_PENDING in klp_start_transition(). > > > > > + smp_wmb(); > > > +} > > So I've not read the patch; but ending a function with an smp_wmb() > feels wrong. > > A wmb orders two stores, and I feel both stores should be well visible > in the same function. Yeah, I would agree with that. And also, it's probably a red flag that the barrier needs *three* paragraphs to describe the various cases its needed for. However, there are some complications: 1) The stores are in separate functions (which is a generally a good thing as it greatly helps the readability of the code). 2) Which stores are being ordered depends on whether the function is called in the enable path or the disable path. 3) Either way it actually orders *two* separate pairs of stores. Anyway I'm thinking I should move that barrier out of klp_init_transition() and into its callers. The stores will still be in separate functions but at least there will be better visibility of where the stores are occurring, and the comments can be a little more focused. -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-s390" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
