From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751135AbdKYIQT (ORCPT ); Sat, 25 Nov 2017 03:16:19 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:40161 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750880AbdKYIQR (ORCPT ); Sat, 25 Nov 2017 03:16:17 -0500 X-Google-Smtp-Source: AGs4zMasN6n5NDXq6/jILrISF4O4ksIWD0TUc2d48OItiZzQfsR4VU9vdqS634317lDDqIMHAClhCw== Date: Sat, 25 Nov 2017 09:16:13 +0100 From: Ingo Molnar To: Mikulas Patocka Cc: Ingo Molnar , Peter Zijlstra , linux-kernel@vger.kernel.org Subject: Re: [PATCH] schedule: use unlikely() Message-ID: <20171125081613.damtbce2pelywx2k@gmail.com> References: <20171124073819.ejig4akxinmugbnr@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Mikulas Patocka wrote: > > > On Fri, 24 Nov 2017, Ingo Molnar wrote: > > > * Mikulas Patocka wrote: > > > > > A small patch for schedule(), so that the code goes straght in the common > > > case. > > > > > > Signed-off-by: Mikulas Patocka > > > > > > --- > > > include/linux/blkdev.h | 2 +- > > > kernel/sched/core.c | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > Index: linux-2.6/include/linux/blkdev.h > > > =================================================================== > > > --- linux-2.6.orig/include/linux/blkdev.h > > > +++ linux-2.6/include/linux/blkdev.h > > > @@ -1308,7 +1308,7 @@ static inline bool blk_needs_flush_plug( > > > { > > > struct blk_plug *plug = tsk->plug; > > > > > > - return plug && > > > + return unlikely(plug != NULL) && > > > (!list_empty(&plug->list) || > > > !list_empty(&plug->mq_list) || > > > !list_empty(&plug->cb_list)); > > > > That's an unrelated change. > > It is related, because this function gets inlined into schedule(). That needs to be in the changelog and the patch needs to be Cc:-ed to the affected maintainer as well. > > > Index: linux-2.6/kernel/sched/core.c > > > =================================================================== > > > --- linux-2.6.orig/kernel/sched/core.c > > > +++ linux-2.6/kernel/sched/core.c > > > @@ -3405,7 +3405,7 @@ void __noreturn do_task_dead(void) > > > > > > static inline void sched_submit_work(struct task_struct *tsk) > > > { > > > - if (!tsk->state || tsk_is_pi_blocked(tsk)) > > > + if (!tsk->state || unlikely(tsk_is_pi_blocked(tsk))) > > > return; > > > /* > > > * If we are going to sleep and we have plugged IO queued, > > > > Do these changes actually change the generated assembly code? > > > > Thanks, > > > > Ingo > > Yes. It saves two jumps because the compiler falsely assumes that > tsk_is_pi_blocked would return true. Likewise, the compiler falsely > assumes that tsk->plug would be true. > > The static branch prediction in gcc assumes that pointers are usually not > NULL, but in this case tsk->pi_blocked_on and tsk->plug are usually NULL. That too should be in the changelog. Thanks, Ingo