From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751593AbbEFM6U (ORCPT ); Wed, 6 May 2015 08:58:20 -0400 Received: from cantor2.suse.de ([195.135.220.15]:60161 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751328AbbEFM6P (ORCPT ); Wed, 6 May 2015 08:58:15 -0400 Date: Wed, 6 May 2015 14:58:12 +0200 (CEST) From: Miroslav Benes To: Oleg Nesterov cc: Jiri Slaby , live-patching@vger.kernel.org, jpoimboe@redhat.com, sjenning@redhat.com, jkosina@suse.cz, vojtech@suse.cz, mingo@redhat.com, linux-kernel@vger.kernel.org, Peter Zijlstra Subject: Re: [RFC kgr on klp 9/9] livepatch: send a fake signal to all tasks In-Reply-To: <20150504143423.GA25809@redhat.com> Message-ID: References: <1430739625-4658-1-git-send-email-jslaby@suse.cz> <1430739625-4658-9-git-send-email-jslaby@suse.cz> <20150504143423.GA25809@redhat.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Mon, 4 May 2015, Oleg Nesterov wrote: > Well, I can't really comment this change because I didn't see other > changes, and I do not know what klp_kgraft_task_in_progress() means... > > On 05/04, Jiri Slaby wrote: > > > > Luckily we can force the task to do that by sending it a fake signal, > > But note that signal_wake_up(0) won't wake the stopped/traced tasks up. Yes, this could happen. Such process would prevent the patching to finish, but that should not be an issue for patching as such. The process's flag would be eventually cleared. > > +static void klp_kgraft_send_fake_signal(void) > > +{ > > + struct task_struct *p; > > + unsigned long flags; > > + > > + read_lock(&tasklist_lock); > > + for_each_process(p) { > > Only the group leader can be klp_kgraft_task_in_progress? > > Looks like you need for_each_process_thread()... Thanks for spotting. This is consistent with other places in the code and needs to be fixed. > > + /* > > + * send fake signal to all non-kthread processes which are still > > + * not migrated > > + */ > > + if (!(p->flags & PF_KTHREAD) && > > So this can miss the execing kernel thread, I do not know if this is > correct or not. PF_KTHREAD is cleared in flush_old_exec(). Correct, we do not deal with kthreads in this RFC yet. There is more work to do it correctly. See changelogs and comments in other patches. > > + klp_kgraft_task_in_progress(p) && > > + lock_task_sighand(p, &flags)) { > > No need for lock_task_sighand(). Just spin_lock_irq(p->sighand->siglock). > tasklist_lock + for_each_process guarantees that "p" has a valid ->sighand. Ah, thank you. > > > + signal_wake_up(p, 0); > > To remind, this won't wakeup a TASK_STOPPED/TRACED thread. > > > void recalc_sigpending(void) > > { > > - if (!recalc_sigpending_tsk(current) && !freezing(current)) > > + if (!recalc_sigpending_tsk(current) && !freezing(current) && > > + !klp_kgraft_task_in_progress(current)) > > clear_thread_flag(TIF_SIGPENDING); > > It is not clear from this patch when TIF_SIGPENDING will be cleared. > > I assume other changes add some hooks into do_notify_resume/get_signal > paths, otherwise a klp_kgraft_task_in_progress() will spin until > klp_kgraft_task_in_progress(current) becomes "false". That is correct. The flag is cleared in do_notify_resume path and also in syscall_trace_enter_phase1. See patch number 4 of this RFC. Thanks a lot for the feedback Miroslav