Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 10 Sep 2021 10:08:56 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     Vasily Gorbik <gor@linux.ibm.com>
Cc:     Josh Poimboeuf <jpoimboe@redhat.com>,
        Jiri Kosina <jikos@kernel.org>,
        Miroslav Benes <mbenes@suse.cz>,
        Joe Lawrence <joe.lawrence@redhat.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        Sumanth Korikkar <sumanthk@linux.ibm.com>,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] livepatch: Kick idle cpu's tasks to perform
 transition
Message-ID: <YTsSmC3vGWa+kf5l@alley>
References: <patch.git-b76842ceb035.your-ad-here.call-01625661932-ext-1304@work.hours>
 <YSjgj+ZzOutFxevl@alley>
 <your-ad-here.call-01631177645-ext-9742@work.hours>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <your-ad-here.call-01631177645-ext-9742@work.hours>
Precedence: bulk

On Thu 2021-09-09 10:54:05, Vasily Gorbik wrote:
> On Fri, Aug 27, 2021 at 02:54:39PM +0200, Petr Mladek wrote:
> > On Wed 2021-07-07 14:49:38, Vasily Gorbik wrote:
> > > --- a/kernel/livepatch/transition.c
> > > +++ b/kernel/livepatch/transition.c
> > > @@ -415,8 +415,11 @@ void klp_try_complete_transition(void)
> > >  	for_each_possible_cpu(cpu) {
> > >  		task = idle_task(cpu);
> > >  		if (cpu_online(cpu)) {
> > > -			if (!klp_try_switch_task(task))
> > > +			if (!klp_try_switch_task(task)) {
> > >  				complete = false;
> > > +				set_tsk_need_resched(task);
> > 
> > Is this really needed?
> 
> Yes, otherwise the inner idle loop is not left and
> klp_update_patch_state() is not reached. Only waking up idle
> cpus is not enough.

I see.

> > Also, please do this in klp_send_signals(). We kick there all other
> > tasks that block the transition for too long.
> 
> #define SIGNALS_TIMEOUT 15
> 
> Hm, kicking the idle threads in klp_send_signals() means extra 15 seconds
> delay for every transition in our case and failing kselftests:
>
> I understand this 15 seconds delay for loaded system and tasks doing real
> work is good,

Yup. Also normal processes should not stay in the running state
for this long. They are typically migrated quickly. But the idle task is
special.

> but those lazy idle "running" tasks could be kicked right
> away with no harm done, right?

Fair enough.

Best Regards,
Petr