Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1473509 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv4 6/6] x86/signal: add SA_{X32,IA32}_ABI sa_flags Dmitry Safonov <0x7f454c46@gmail.com> - 2016-08-31 16:10 +0200
    Re: [PATCHv4 6/6] x86/signal: add SA_{X32,IA32}_ABI sa_flags Oleg Nesterov <oleg@redhat.com> - 2016-09-01 14:40 +0200
      Re: [PATCHv4 6/6] x86/signal: add SA_{X32,IA32}_ABI sa_flags Cyrill Gorcunov <gorcunov@gmail.com> - 2016-09-01 14:50 +0200
        Re: [PATCHv4 6/6] x86/signal: add SA_{X32,IA32}_ABI sa_flags Dmitry Safonov <0x7f454c46@gmail.com> - 2016-09-01 15:50 +0200
          Re: [PATCHv4 6/6] x86/signal: add SA_{X32,IA32}_ABI sa_flags Cyrill Gorcunov <gorcunov@gmail.com> - 2016-09-01 16:00 +0200
          Re: [PATCHv4 6/6] x86/signal: add SA_{X32,IA32}_ABI sa_flags Oleg Nesterov <oleg@redhat.com> - 2016-09-01 23:20 +0200

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

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?

2016-08-31 16:59 GMT+03:00 Dmitry Safonov <dsafonov@virtuozzo.com>:
> Introduce new flags that defines which ABI to use on creating sigframe.
> Those flags kernel will set according to sigaction syscall ABI,
> which set handler for the signal being delivered.
>
> So that will drop the dependency on TIF_IA32/TIF_X32 flags on signal deliver.
> Those flags will be used only under CONFIG_COMPAT.
>
> Similar way ARM uses sa_flags to differ in which mode deliver signal
> for 26-bit applications (look at SA_THIRYTWO).
>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: linux-mm@kvack.org
> Cc: x86@kernel.org
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Pavel Emelyanov <xemul@virtuozzo.com>
> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
> Reviewed-by: Andy Lutomirski <luto@kernel.org>

Thanks,
             Dmitry

[toc] | [next] | [standalone]

#1474372

On 08/31, Dmitry Safonov 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?

Oleg.

[toc] | [prev] | [next] | [standalone]

#1474380

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?

[toc] | [prev] | [next] | [standalone]

#1474406

Thanks for your replies Oleg, Cyrill,

2016-09-01 15:45 GMT+03:00 Cyrill Gorcunov <gorcunov@gmail.com>:
> 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

[toc] | [prev] | [next] | [standalone]

#1474408

On Thu, Sep 01, 2016 at 04:47:23PM +0300, Dmitry Safonov wrote:
> Thanks for your replies Oleg, Cyrill,
> 
> 2016-09-01 15:45 GMT+03:00 Cyrill Gorcunov <gorcunov@gmail.com>:
> > 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.

Yup, this gonna be a mess, that's why I asked, because I suspect
Oleg meant something else maybe?

[toc] | [prev] | [next] | [standalone]

#1474607

On 09/01, Dmitry Safonov wrote:
>
> 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.

I do not really understand why it has to be so complicated, but
I can be easily wrong.

> And this arch_prctl() API is visible under CHECKPOINT_RESTORE
> config option, so will not bother anyone.

I mostly dislike 6/6. This new feauture looks a bit strange to me.

Nevermind, let me repeat once again, I am not trying to argue with
this series. No objections from me.

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web