Re: [PATCHv4 6/6] x86/signal: add SA_{X32,IA32}_ABI sa_flags

[Dmitry Safonov]

Thanks for your replies Oleg, Cyrill,

2016-09-01 15:45 GMT+03:00 Cyrill Gorcunov <gorcunov@xxxxxxxxx>:
> On Thu, Sep 01, 2016 at 02:27:44PM +0200, Oleg Nesterov wrote:
>> > Hi Oleg,
>> > can I have your acks or reviewed-by tags for 4-5-6 patches in the series,
>> > or there is something left to fix?
>>
>> Well yes... Although let me repeat, I am not sure I personally like
>> the very idea of 3/6 and 6/6. But as I already said I do not feel I
>> understand the problem space enough, so I won't argue.
>>
>> However, let me ask again. Did you consider another option? Why criu
>> can't exec a dummy 32-bit binary before anything else?
>
> I'm not really sure how this would look then. If I understand you
> correctly you propose to exec dummy 32bit during "forking" stage
> where we're recreating a process tree, before anything else. If
> true this implies that we will need two criu engines: one compiled
> with 64 bit and (same) second but compiled with 32 bits, no?

Yep, we would need then full CRIU, but compiled in 32 bits.
And it can be then even more complicated, as 64-bit parent
can have 32-bit child, which can have 64-bit child... et cetera.

And the biggest problem in this approach would be not the size of
code changes to CRIU (which are already quite large with this
patches set), but AFAICS, it will have big performance penalty:
we would need to bounce process tree, processes properties
from parent-CRIU to child-CRIU after exec() call and down on
the processes hierarchy, recreating processes while synchronizing
process's data from images.

As for now, we already have time-critical problems in ÐRIU and
we try to reduce the number of system calls, while it's still slow
at some places. But that approach will lead to:
o exec different CRIU
o initialize it (i.e, parse /proc/self/maps to know it's vmas)
o transphere process tree, for each process it's properties with IPC
   after exec()
It will all go for a large number of syscalls in total.

So, for the current patches set the performance penalty is one call
to arch_prctl() to map 32-bit vdso blob. It's even smaller, as one
specifies the address on which to map the blob and doesn't need
additional mremap()'s to move the blob on needed location.
And this arch_prctl() API is visible under CHECKPOINT_RESTORE
config option, so will not bother anyone.

-- 
             Dmitry