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

[Anton Kozlov]

Hi, All!

Andrew, let me clarify, the concern is in cores cache coherence after executing
NativeTrampolineCall::set_destination_mt_safe?

From my point of view, since

> > > looking at linux/arch/arm/include/asm/cacheflush.h and I can't see
> > > anything like that.  All I see is that do_cache_op calls
> > > __flush_cache_user_range, and all that does is ensure that the I and
> > > D
> > > caches are coherent.
>

after any case from
1) instruction modification and ICache::flush
2) NativeTrampolineCall::set_destination_mt_safe and no data flush
we came to the same state, when new data is in the dcache of single core.

NativeTrampolineCall::set_destination_mt_safe is not different from any other
instruction modification methods.

And we both agree that it's OK to call old destination even after modification.

And eventually data change will land to other core dchache, because there is
fence on executing site, which you've mentioned.  
set_destination_mt_safe is called when PatchingLock is taken, so it's followed
by release.
Executing thread could see only 2 options, since patch is word-aligned single
word data chage:
1) new data. Then everything is OK, just call new destination
2) old data. Then execution stream should go to same patching code, c1_Runtime,
which takes same PatchingLock, 
and therefore executes acquire. Then it should see new data, perform no
patching, leave patching routine and call new destination.

Thanks,
Anton

On Tue, 2016-04-05 at 10:25 +0100, Edward Nevill wrote:
> Hi,
> 
> Was there a conclusion on this?
> 
> Are you happy with Anton's rev2 patch?
> 
> All the best,
> Ed.
> 
> On Fri, 2016-04-01 at 10:41 +0000, Anton Kozlov wrote:
> > 
> > Hi, Andrew,
> > 
> > On Thu, 2016-03-31 at 09:49 +0100, Andrew Haley wrote:
> > > 
> > > On 30/03/16 20:53, Anton Kozlov wrote:
> > > 
> > > > 
> > > > 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.
> > > I don't think so.  In order for this to work, the *reader* would have
> > > to execute some kind of fence too.  I don't think that's possible
> > > without an inter-processor interrupt across all cores, and I think
> > > that __clear_cache doesn't do that.  I certainly hope not.
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > 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.
> > > > 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.
> > > I think you will find that it does not do what you think it does. 
> > >  I'm
> > > looking at linux/arch/arm/include/asm/cacheflush.h and I can't see
> > > anything like that.  All I see is that do_cache_op calls
> > > __flush_cache_user_range, and all that does is ensure that the I and
> > > D
> > > caches are coherent.
>