Re: Re: [PATCH v4] signal: trace_signal_deliver when signal_group_exit

Re: Re: [PATCH v4] signal: trace_signal_deliver when signal_group_exit

[weizhenliang]

On 04/25 Christian wrote:
>On Wed, Apr 24, 2019 at 6:16 PM Oleg Nesterov <oleg@redhat.com> wrote:
>>
>> On 04/24, weizhenliang wrote:
>> >
>> > On 04/24, Oleg wrote:
>> > >On 04/24, Christian Brauner wrote:
>> > >>
>> > >> On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
>> > >>
>> > >> > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>> > >
>> > >Yes, but ...
>> > >
>> > >> > Reported-by: kbuild test robot <lkp@intel.com>
>> > >
>> > >Hmm, really?
>> >
>> > Yes, the kbuild test robot says that if I fix the problem with the 
>> > third parameter type, I should add this tag. What is wrong or missing?
>>
>> But this patch does not fix the problem reported by robot, your patch 
>> (which introduced that problem) was dropped, the problem has gone.
>>
>> With this "Reported-by: kbuild test robot <lkp@intel.com>" tag it 
>> looks as if test-robot has found the problem you are trying to fix: 
>> the lack of trace_signal_deliver(SIGKILL).
>
>Yeah, Oleg's absolutely right. That tag should just go.
>The Fixes line is all that we want, I think.

Got it ~ 
Thank you (Oleg and Christian) for the kind guidance
And I will update the patch as soon as possible

Wei.

Re: Re: [PATCH v4] signal: trace_signal_deliver when signal_group_exit

[Christian Brauner]

On Wed, Apr 24, 2019 at 6:16 PM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 04/24, weizhenliang wrote:
> >
> > On 04/24, Oleg wrote:
> > >On 04/24, Christian Brauner wrote:
> > >>
> > >> On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
> > >>
> > >> > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> > >
> > >Yes, but ...
> > >
> > >> > Reported-by: kbuild test robot <lkp@intel.com>
> > >
> > >Hmm, really?
> >
> > Yes, the kbuild test robot says that if I fix the problem with the third parameter type,
> > I should add this tag. What is wrong or missing?
>
> But this patch does not fix the problem reported by robot, your patch
> (which introduced that problem) was dropped, the problem has gone.
>
> With this "Reported-by: kbuild test robot <lkp@intel.com>" tag it looks
> as if test-robot has found the problem you are trying to fix: the lack of
> trace_signal_deliver(SIGKILL).

Yeah, Oleg's absolutely right. That tag should just go.
The Fixes line is all that we want, I think.

Christian

Re: Re: [PATCH v4] signal: trace_signal_deliver when signal_group_exit

[Oleg Nesterov]

On 04/24, weizhenliang wrote:
>
> On 04/24, Oleg wrote:
> >On 04/24, Christian Brauner wrote:
> >>
> >> On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
> >>
> >> > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> >
> >Yes, but ...
> >
> >> > Reported-by: kbuild test robot <lkp@intel.com>
> >
> >Hmm, really?
>
> Yes, the kbuild test robot says that if I fix the problem with the third parameter type,
> I should add this tag. What is wrong or missing?

But this patch does not fix the problem reported by robot, your patch
(which introduced that problem) was dropped, the problem has gone.

With this "Reported-by: kbuild test robot <lkp@intel.com>" tag it looks
as if test-robot has found the problem you are trying to fix: the lack of
trace_signal_deliver(SIGKILL).

Oleg.

Re: Re: [PATCH v4] signal: trace_signal_deliver when signal_group_exit

[weizhenliang]

On 04/24, Oleg wrote:
>On 04/24, Christian Brauner wrote:
>>
>> On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
>>
>> > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>
>Yes, but ...
>
>> > Reported-by: kbuild test robot <lkp@intel.com>
>
>Hmm, really?

Yes, the kbuild test robot says that if I fix the problem with the third parameter type, 
I should add this tag. What is wrong or missing?

Wei.

Re: Re: [PATCH v4] signal: trace_signal_deliver when signal_group_exit

[weizhenliang]

On 04/24, Christian Brauner wrote:
>On Wed, Apr 24, 2019 at 08:52:38PM +0800, Zhenliang Wei wrote:
>> 
>> Acked-by: Christian Brauner <christian@brauner.io>
>
>I think we're supposed to use more Reviewed-bys so feel free (or Andrew) to change this to:
>
>Reviewed-by: Christian Brauner <christian@brauner.io>

Ok, I will change this in patch v5.

>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -2441,6 +2441,8 @@ bool get_signal(struct ksignal *ksig)
>>  	if (signal_group_exit(signal)) {
>>  		ksig->info.si_signo = signr = SIGKILL;
>>  		sigdelset(&current->pending.signal, SIGKILL);
>> +		trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
>> +			&sighand->action[signr - 1]);
>
>Hm, sorry for being the really nitpicky person here. Just for the sake of consistency how about we do either:
>
>+		trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
>+			&sighand->action[SIGKILL - 1]);
>
>or
>
>+		trace_signal_deliver(signr, SEND_SIG_NOINFO,
>+			&sighand->action[signr - 1]);
>
>I'm not going to argue about this though. Can just also leave it as is.

Thank you for your comments and learn from rigorous people! I will take:

+		trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
+			&sighand->action[SIGKILL - 1]);

Any other suggestions about the patch?

Wei