From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754481AbcEPR2B (ORCPT ); Mon, 16 May 2016 13:28:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37895 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754014AbcEPR17 (ORCPT ); Mon, 16 May 2016 13:27:59 -0400 Date: Mon, 16 May 2016 12:27:55 -0500 From: Josh Poimboeuf To: Miroslav Benes Cc: Jessica Yu , Jiri Kosina , Ingo Molnar , Peter Zijlstra , Michael Ellerman , Heiko Carstens , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, x86@kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, Vojtech Pavlik , Jiri Slaby , Petr Mladek , Chris J Arges , Andy Lutomirski Subject: Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Message-ID: <20160516172755.gro5enwei2gth3ko@treble> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Mon, 16 May 2016 17:27:59 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 09, 2016 at 11:41:37AM +0200, Miroslav Benes wrote: > > +void klp_init_transition(struct klp_patch *patch, int state) > > +{ > > + struct task_struct *g, *task; > > + unsigned int cpu; > > + struct klp_object *obj; > > + struct klp_func *func; > > + int initial_state = !state; > > + > > + klp_transition_patch = patch; > > + > > + /* > > + * If the patch can be applied or reverted immediately, skip the > > + * per-task transitions. > > + */ > > + if (patch->immediate) > > + return; > > + > > + /* > > + * Initialize all tasks to the initial patch state to prepare them for > > + * switching to the target state. > > + */ > > + read_lock(&tasklist_lock); > > + for_each_process_thread(g, task) > > + task->patch_state = initial_state; > > + read_unlock(&tasklist_lock); > > + > > + /* > > + * Ditto for the idle "swapper" tasks. > > + */ > > + get_online_cpus(); > > + for_each_online_cpu(cpu) > > + idle_task(cpu)->patch_state = initial_state; > > + put_online_cpus(); > > + > > + /* > > + * Ensure klp_ftrace_handler() sees the task->patch_state updates > > + * before the func->transition updates. Otherwise it could read an > > + * out-of-date task state and pick the wrong function. > > + */ > > + smp_wmb(); > > + > > + /* > > + * Set the func transition states so klp_ftrace_handler() will know to > > + * switch to the transition logic. > > + * > > + * When patching, the funcs aren't yet in the func_stack and will be > > + * made visible to the ftrace handler shortly by the calls to > > + * klp_patch_object(). > > + * > > + * When unpatching, the funcs are already in the func_stack and so are > > + * already visible to the ftrace handler. > > + */ > > + klp_for_each_object(patch, obj) > > + klp_for_each_func(obj, func) > > + func->transition = true; > > + > > + /* > > + * Set the global target patch state which tasks will switch to. This > > + * has no effect until the TIF_PATCH_PENDING flags get set later. > > + */ > > + klp_target_state = state; > > I am afraid there is a problem for (patch->immediate == true) patches. > klp_target_state is not set for those and the comment is not entirely > true, because klp_target_state has an effect in several places. Ah, you're right. I moved this assignment here for v2. It was originally done before the patch->immediate check. If I remember correctly, I moved it closer to the barrier for better readability (but I created a bug in the process). > I guess we need to set klp_target_state even for immediate patches. Should > we also initialize it with KLP_UNDEFINED and set it to KLP_UNDEFINED in > klp_complete_transition()? Yes, to both. -- Josh