From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 82127C04A6B for ; Fri, 10 May 2019 13:27:19 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5182C2175B for ; Fri, 10 May 2019 13:27:19 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="kvfeudL7" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5182C2175B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=MrPTNVEHOmyHOkRW4DQri1S94clj7+KDPgilUxJDw2I=; b=kvfeudL77fXpIS 9GJUceWg0BBw6J5Hrtq6GO+OnA6HJbPNdnMybOD99ozcupHBlrLWmr1ioSsjc5CFlivNFnhYkXkpV uaVAIn+/6+K/n//Qgm1wOcOlZadl3OnZs9ulzz0j911RIpGKl7fyrWVidpWB8RgpYUoCdv1TuvdgP FC8jF+WzMA2J+DXkL6PKggQo+fb3B1yCFPoORE/xqCHSlSLN5Gfc414gY11fdGv7Ow1Vb3nY7TrYM O/rxf03uv7iYwPaqYC8mRPLCvVcc+7eJOcWFJFSImo5PLVH/VAunGIM/Nd0kE8r6nuNmFbLlhkexf OaNmY+QfFzuNFnnlIwDg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hP5YS-0001d2-E9; Fri, 10 May 2019 13:27:16 +0000 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70] helo=foss.arm.com) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hP5YP-0001ca-7v for linux-arm-kernel@lists.infradead.org; Fri, 10 May 2019 13:27:14 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A4759374; Fri, 10 May 2019 06:27:08 -0700 (PDT) Received: from e103592.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 937253F575; Fri, 10 May 2019 06:27:07 -0700 (PDT) Date: Fri, 10 May 2019 14:26:59 +0100 From: Dave Martin To: Hillf Danton Subject: Re: [patch] arm64: assembler: Update the yield NEON comment Message-ID: <20190510132651.GA28398@e103592.cambridge.arm.com> References: <177247.865216003-sendEmail@laptop-0p1i5f25> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <177247.865216003-sendEmail@laptop-0p1i5f25> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190510_062713_295226_6B7E6494 X-CRM114-Status: GOOD ( 25.51 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Zijlstra , Will Deacon , "linux-arm-kernel@lists.infradead.org" , Ard Biesheuvel Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, May 09, 2019 at 03:01:43PM +0000, Hillf Danton wrote: Your mailer did something funny here and send a multipart MIME message. If in doubt, use git send-email. > The comment was a bit misleading when it was created in commit 24534b3511, and > deserves a tweak like, > > - * - Check whether the preempt count is exactly 1, in which case disabling > - * preemption once will make the task preemptible. If this is not the case, > + * - Check whether the preempt count is exactly 1, in which case decrementing > + * preempt count once will make the task preemptible. If this is not the case, > > then code fix was added in commit 7faa313f05 with the comment left behind untouched. > > It no longer matches the code now, so fix it. It is changed along the original > direction as much as I can, though simply deleting the relevant block looks fine. > > And finally a question remains: is it needed to decrement preempt count before > invoking kernel_neon_end() in which preempt_enable() is put at the end? Nit: please follow the recommendations in Documentation/process/submitting-patches.rst regarding formatting commit messages. (Or run scripts/checkpatch.pl.) > > Cc: Ard Biesheuvel > Cc: Dave Martin > Cc: Will Deacon > Cc: Peter Zijlstra > Signed-off-by: Hillf Danton > --- > arch/arm64/include/asm/assembler.h | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h > index c5308d0..8518a7b 100644 > --- a/arch/arm64/include/asm/assembler.h > +++ b/arch/arm64/include/asm/assembler.h > @@ -713,13 +713,9 @@ USER(\label, ic ivau, \tmp2) // invalidate I line PoU > * Note that the patchup code does not support assembler directives that change > * the output section, any use of such directives is undefined. > * > - * The yield itself consists of the following: > - * - Check whether the preempt count is exactly 1, in which case disabling > - * preemption once will make the task preemptible. If this is not the case, > - * yielding is pointless. > - * - Check whether TIF_NEED_RESCHED is set, and if so, disable and re-enable > - * kernel mode NEON (which will trigger a reschedule), and branch to the > - * yield fixup code. > + * The yield itself decrements the preempt count and if count hits zero, disable > + * and re-enable kernel mode NEON (which will trigger a reschedule), and branch > + * to the yield fixup code. Good spot -- the original comment is definitely wrong: disabling preemption certainly shouldn't make the task preemptible! So, I'm certainly in favour of fixing that. Your new text doesn't look right, though: AFAICT commit 7faa313f05 didn't intentionally change the behaviour here. It looks to me like the preempt count (i.e., current_thread_info()->preempt) is not decremented by this code: the value is read and the subtraction is done for comparison purposes, but no value is stored back. This is intentional, because the the code needs to do some cleanup before preemption can be enabled for real: the call to kernel_neon_end does both of those jobs. Or did I miss something? Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel