On 10/17/2013 11:24 PM, Jon Medhurst (Tixy) wrote: > On Thu, 2013-10-17 at 12:38 +0100, Will Deacon wrote: >> [adding Tixy for stop_machine() question below] >> >> On Thu, Oct 17, 2013 at 07:19:35AM +0100, Jiang Liu wrote: > [...] >>> +int __kprobes aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int >>> cnt) >>> +{ >>> + struct aarch64_insn_patch patch = { >>> + .text_addrs = addrs, >>> + .new_insns = insns, >>> + .insn_cnt = cnt, >>> + }; >>> + >>> + if (cnt <= 0) >>> + return -EINVAL; >>> + >>> + /* >>> + * Execute __aarch64_insn_patch_text() on every online CPU, >>> + * which ensure serialization among all online CPUs. >>> + */ >>> + return stop_machine(aarch64_insn_patch_text_cb, &patch, NULL); >>> +} >> >> Whoa, whoa, whoa! The comment here is wrong -- we only run the patching on >> *one* CPU, which is the right thing to do. However, the arch/arm/ call to >> stop_machine in kprobes does actually run the patching code on *all* the >> online cores (including the cache flushing!). I think this is to work around >> cores without hardware cache maintenance broadcasting, but that could easily >> be called out specially (like we do in patch.c) and the flushing could be >> separated from the patching too. > [...] > > For code modifications done in 32bit ARM kprobes (and ftrace) I'm not > sure we ever actually resolved the possible cache flushing issues. If > there was specific reasons for flushing on all cores I can't remember > them, sorry. I have a suspicion that doing so was a case of sticking > with what the code was already doing, and flushing on all cores seemed > safest to guard against problems we hadn't thought about. > > Some of the issues discussed were that we couldn't have one core > potentially executing instructions being modified by another CPU, > because that's architecturally unpredictable except for a few > instructions [1], and we also have the case where a 32-bit Thumb > instruction can straddle two different cache-lines. But these may not be > reasons to flush on all cores if stop machine is synchronising all CPU's > in a kind of holding pen and the cache operations done on one core are > broadcast to others. (Are there correct barriers involved in > stop-machine so that when the other cores resume they are guaranteed to > only see the new version of the modified code, or do we only get that > guarantee because we happen to execute the cache flushing on all cores?) I think it's cache flushing instead of stop_machine() because cache flusing includes an ISB.
The idea flow should be: 1) master acquire a lock to serialize text patching 2) all CPU barrier 3) master updates memory and flush cache 4) master set a flag to let all other CPU continue 5) all other CPU executes ISB. Updating memory and flushing cache on every CPU with stop_machine() achieves the same effect with simple implementation, but the implementation really seems a little strange. If desired, I will implement the standard flow. > > [1] > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/136441.html > > Another of the issues I hit was big.LITTLE related whereby the cache > line size is different on different cores [2]. > > [2] > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/149794.html > > I don't think anything I've said above actually gives a solid reason why > we _must_ execute cache flushing on all cores for kprobes and can't just > use the relatively new patch_text function (which checks for the one > case we do need to flush on all cores using cache_ops_need_broadcast). > > Sorry, I don't think I've added much light on things here have I? > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/