[8u] RFR: nativeInstruction atomicity and correctness of patching

[Anton Kozlov]

Hi,

On Wed, 2016-03-30 at 16:25 +0100, Andrew Haley wrote:
> On 03/30/2016 04:15 PM, Anton Kozlov wrote:
> > ICache::invalidate_word used here not to provide coherency between
> > Icache and Dcache, but to maintain in-order visibillity of code
> > modification.
> 
> What exactly are you trying to synchronize?
ICache::invalidate ensures that all CPUs see modifications of code. Data modification in trampoline should be visible in the same way, same time as code modification would. For this ICache::invalidate used, as it by implementation should synchornize dcaches among CPUs.

> Any code modifications
> must have their own icache invalidation, which does a full DSB.  But
> this call does not modify any code.
DSB is not enough for this as it only forces memory write to complete, but it not affects other CPUs caches. ICache::invalidate (implemented as __clear_cache), flushes and invalidates CPUs dcaches and icaches, roughly. This should allow trampoline to load proper call destination from dcache.

> > I mean, if NativeTrampolineCall::set_destination_mt_safe
> > called before another code modification, effect of it will be
> > observed
> > by another cpu before effect of 2nd code modification.
>
> Only if that other CPU executes a read fence.
Since there is no read fence on executing site, cache maintance used here to do same job.

> 
> > This is achieved by implementation of ICache::invalidate_word,
> > which
> > is just call to __clear_cache syscall wrapper.
>
> I don't think there's any point.  It's not instructions but data, so
> ICache::invalidate_word is inappropriate.
> Andrew.

Agree, ICache is bad name for that. How about direct __clear_cache call?
Also, I will try to study implementation of __clear_cache to know, how exactly it's done.
But for me it looks like if we drop cache maintance in NativeTrampolineCall::set_destination_mt_safe, we'll get race condition, and some thread could call old destination. 

And sorry for being late, thank you for review!

Thanks,
Anton