From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757597AbcCUSQc (ORCPT ); Mon, 21 Mar 2016 14:16:32 -0400 Received: from mx2.suse.de ([195.135.220.15]:60796 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757211AbcCUSQa convert rfc822-to-8bit (ORCPT ); Mon, 21 Mar 2016 14:16:30 -0400 Date: Mon, 21 Mar 2016 11:16:22 -0700 From: Davidlohr Bueso To: Peter Zijlstra Cc: tglx@linutronix.de, mingo@kernel.org, bigeasy@linutronix.de, umgwanakikbuti@gmail.com, paulmck@linux.vnet.ibm.com, linux-kernel@vger.kernel.org, dave@stgolabs.net Subject: Re: [PATCH 4/3] rtmutex: Avoid barrier in rt_mutex_handle_deadlock Message-ID: <20160321181622.GB32012@linux-uzut.site> References: <1457461223-4301-1-git-send-email-dave@stgolabs.net> <20160308220539.GB4404@linux-uzut.site> <20160314134038.GZ6356@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8BIT In-Reply-To: <20160314134038.GZ6356@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 14 Mar 2016, Peter Zijlstra wrote: >So you're right that it doesn't matter here, however for that very >reason I would suggest not using __set_current_state() before schedule() >unless there is a _really_ good reason, and then with an extensive >comment to go with. No problem. > >Otherwise people will manage to pick this as an example to copy and who >all knows what kind of borkage will result from that. Although I would expect 'people' to at least read the comments around the code... and not blindly use rt-deadlock-related things :) But yeah, lets drop this, I have no objection. While going through this, I did find that we could do a little better documenting the actual helpers. What do you think of the following? Thanks, Davidlohr ----------8<---------------------------------------------------------- From: Davidlohr Bueso Subject: [PATCH -tip] sched: Cleanup comments for tsk->state helpers While there is nothing wrong about the current comments, we could easily improve them by the changes proposed in this patch: - Remove duplicate text for CONFIG_DEBUG_ATOMIC_SLEEP. - Update blocking example to consider spurious wakeups (for-loop). - Point the reader to the infamous memory-barriers.txt doc, which goes into plenty of detail in the 'SLEEP AND WAKE-UP FUNCTIONS' section (the above also taken from there). Signed-off-by: Davidlohr Bueso --- include/linux/sched.h | 51 ++++++++++++++++++++++++++------------------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/include/linux/sched.h b/include/linux/sched.h index c617ea12c6b7..3a3ec2503897 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -249,6 +249,31 @@ extern char ___assert_task_state[1 - 2*!!( (task->flags & PF_FROZEN) == 0 && \ (task->state & TASK_NOLOAD) == 0) +/* + * Helpers for modifying the state of either the current task, or a foreign + * task. Each of these calls come in both full barrier and weak flavors: + * + * Weak + * set_task_state() __set_task_state() + * set_current_state() __set_current_state() + * + * Where set_current_state() and set_task_state() includes a full smp barrier + * -after- the write of ->state is correctly serialized with the later test + * of whether to actually sleep: + * + * for (;;) { + * set_current_state(TASK_UNINTERRUPTIBLE); + * if (event_indicated) + * break; + * schedule(); + * } + * + * This is commonly necessary for processes sleeping and waking through flag + * based events. If the caller does not need such serialization, then use + * weaker counterparts, which simply writes the state. + * + * Refer to Documentation/memory-barriers.txt + */ #ifdef CONFIG_DEBUG_ATOMIC_SLEEP #define __set_task_state(tsk, state_value) \ @@ -261,18 +286,6 @@ extern char ___assert_task_state[1 - 2*!!( (tsk)->task_state_change = _THIS_IP_; \ smp_store_mb((tsk)->state, (state_value)); \ } while (0) - -/* - * set_current_state() includes a barrier so that the write of current->state - * is correctly serialised wrt the caller's subsequent test of whether to - * actually sleep: - * - * set_current_state(TASK_UNINTERRUPTIBLE); - * if (do_i_need_to_sleep()) - * schedule(); - * - * If the caller does not need such serialisation then use __set_current_state() - */ #define __set_current_state(state_value) \ do { \ current->task_state_change = _THIS_IP_; \ @@ -290,24 +303,12 @@ extern char ___assert_task_state[1 - 2*!!( do { (tsk)->state = (state_value); } while (0) #define set_task_state(tsk, state_value) \ smp_store_mb((tsk)->state, (state_value)) - -/* - * set_current_state() includes a barrier so that the write of current->state - * is correctly serialised wrt the caller's subsequent test of whether to - * actually sleep: - * - * set_current_state(TASK_UNINTERRUPTIBLE); - * if (do_i_need_to_sleep()) - * schedule(); - * - * If the caller does not need such serialisation then use __set_current_state() - */ #define __set_current_state(state_value) \ do { current->state = (state_value); } while (0) #define set_current_state(state_value) \ smp_store_mb(current->state, (state_value)) -#endif +#endif /* CONFIG_DEBUG_ATOMIC_SLEEP */ /* Task command name length */ #define TASK_COMM_LEN 16 -- 2.1.4