From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755541AbcANB3v (ORCPT ); Wed, 13 Jan 2016 20:29:51 -0500 Received: from eu-smtp-delivery-143.mimecast.com ([207.82.80.143]:59406 "EHLO eu-smtp-delivery-143.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755240AbcANB3t convert rfc822-to-8bit (ORCPT ); Wed, 13 Jan 2016 20:29:49 -0500 Date: Thu, 14 Jan 2016 09:29:19 +0800 From: Huang Shijie To: Thomas Gleixner CC: , LKML , Jiang Liu , Peter Zijlstra , Subject: Re: [PATCH 1/1] Revert "genirq: Remove the second parameter from handle_irq_event_percpu()" Message-ID: <20160114012918.GA13689@sha-win-210.asiapac.arm.com> References: <1452681116-20924-1-git-send-email-zyjzyj2000@gmail.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-Originating-IP: [217.140.104.200] X-ClientProxiedBy: TY1PR01CA0003.jpnprd01.prod.outlook.com (25.161.131.141) To AM2PR08MB0435.eurprd08.prod.outlook.com (25.163.148.152) X-Microsoft-Exchange-Diagnostics: 1;AM2PR08MB0435;2:c9ClcBYN0Zvncp+iMdhloaC1g9YErWEiWRlUlvBqoE8pzXnJi9bCiDgmT3dXLJow6bWSsLMfl1BfywIIVOHxgZaKz/5HxZ7s5RO4xhJ8EkBqrVHigND45SotsKq/7g54y3DQ9Xc8aXQwfrtyFVpV+A==;3:FMzBLi9IDJIrvWHlYQ3eE3Kc1p9XoIMPhH3f3aKJtVFOcq/ZVWDJIr8W0PWWrE2NFfEs+QJ2YDcZeEgO5RKZDUCqGa+aFxnF8n9mUQnp3AtnTyrhvkBpb0aoJeJDDOuj;25:UyBtG415aAQQQWdh4goDqQVgJZ+TZM03Wugp5BcQg4+rkoYM2EVGfl743O1FRfvKJZkWArwMs2XhEQhbuZHJR+Si6J5X5PFNCg3XmQTINKjXdlrOYi15OUP+EMTCH2saQUPUse1mLYzNybTgEsaZuIKIsPhBQde59AN7lPVFUKowlaVrtSc1w5fBjD7oT95TV/tcKmVYllvaoxUs26Zv/I4vw14v0/lIm5Y0T+0crzJlDreIijfjQrvS+BOfYIUX;20:hc9pk6F5rlB9SpAzg4lPlV1XkQesA8xc7USNR2NgVY1cEujGItMDVmAm6DNNmgOksSUMtnl/CoIDQcKcp8jySo8Gft51JO0fAr62RfZC9M5N7S6SL9G7oT+96pJopQ+S2grK1ulhHLahlGn9El1RA+apiLBqc5mPH91HAOH3iOw= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AM2PR08MB0435; X-MS-Office365-Filtering-Correlation-Id: 4b161c39-98ad-485a-0444-08d31c822e83 NoDisclaimer: True X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(520078)(8121501046)(5005006)(10201501046)(3002001);SRVR:AM2PR08MB0435;BCL:0;PCL:0;RULEID:;SRVR:AM2PR08MB0435; X-Microsoft-Exchange-Diagnostics: 1;AM2PR08MB0435;4:JPB+9VCDs7hI32e6VT2aca0MVMBniNspEDKJEfEIMInuINEY5A79FE6aBoQC55DFWojldS1paafLHouQjb2vNNPHqTLt5oyuJ+gmrVdOvF/YIZum5Yq+4KqFq0iZxvgBtpwR5ASOv0JoAbhFlrZxv4j2K9XehWm38cf4FG/0Bpvz/eUisQr+qhRiE4Wjz9qJUQLTtCUfsCn8zLFRM0hGD6x0I0IOoupo3EV42Zr0K+dh6p0LXCNSX3Q9Hbfg0cHBDOVmo2TN8D0AMfTJ97nyfo2qqJuEUcePkHr3kcF/WIlZ3+crHifCtPw1zWYhAAPWLBUWNwWc7tL+OAJndtaNr0zoWUFkSHCUT5SVijVxzTjIbuuilcn3hNLbiXdqC5Y3 X-Forefront-PRVS: 08213D42D3 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6009001)(24454002)(164054003)(189002)(199003)(3846002)(47776003)(6116002)(106356001)(42186005)(105586002)(97756001)(33656002)(2906002)(101416001)(5004730100002)(586003)(575784001)(1096002)(1076002)(23726003)(66066001)(4326007)(83506001)(46406003)(87976001)(77096005)(19580395003)(2950100001)(5008740100001)(86362001)(76176999)(97736004)(19580405001)(189998001)(54356999)(4001350100001)(40100003)(122386002)(50986999)(110136002)(50466002)(92566002);DIR:OUT;SFP:1101;SCL:1;SRVR:AM2PR08MB0435;H:sha-win-210.asiapac.arm.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;AM2PR08MB0435;23:uKYLm1tGbsIKFvRrTtXTCy8NdjBGoA/+AsUR952s8?= =?us-ascii?Q?4EAsRNLsyIUWZK9iyR87KorJ2YM/mctdYhAM5HYQZ84AHPi0haukQPJrCNb4?= =?us-ascii?Q?cjnQmbkw+UKdjhb0scm1Lwyf0Pd0cZgYSymNKgFDzerFJWFJpTKa95wpRjaa?= =?us-ascii?Q?Vp/KnR1GQVgV1i8pITey9jDpi1pWk1CTYEaKUzNEtXVx3VlG9dxOJTzWKTjx?= =?us-ascii?Q?NvpNeVCGSVqDpDMZgHmSD16imq7VegbtRAExrOtLFZ+Oy/crRK9M8xm7Zn3b?= =?us-ascii?Q?e5WXuEIRh3L/nXZSaBWyWB5iiZ/UEkfGwgAzBmTsJso/ttFAMaLX8/DXiWKI?= =?us-ascii?Q?2vZsD/X7O3Vpdc/C4D8UOB4JUbaCk2NTcMwuH6b0RrnLx3aVW9C+XfqlRzRW?= =?us-ascii?Q?t1nJB/hi3Z9tUWGToriQoxKVxfBTUJFRNnOdCvpSKCztcN1pra4jowIFo1oR?= =?us-ascii?Q?WyciXnAFx/7GP6ywvJ0olKu0RecuzM/GgEi/29dWJW+BajxVZSh93G91PVCI?= =?us-ascii?Q?K2FtB/kzeppWatDi1qPnC+ypT1m8A6eUM6eoRdLQPkmuWXsfEOKwiOmptrDq?= =?us-ascii?Q?GmeP9Z4PInKUi44zYiz0WUwXTjFvC/4pZsaPNITByVPA4avWTHDB7+29Igxd?= =?us-ascii?Q?D5NMOEzw926OXdPHXyIU6IfpDiVdlK3tnMFQfcKzB+QvETyAIao2hhmg8Mw9?= =?us-ascii?Q?qLzLdIzOwovDFEFxETYCvtQwKSoa1l0pcQUYD26fTu6X7Qyh2F+4+qRKN6Zg?= =?us-ascii?Q?jy9uXr8MT9+14R87bSLeFZuMOPz84WJtoJ6eeuQYDUtG/KIsRWq7ORterW6k?= =?us-ascii?Q?J5ygPq5gYGzEwQXm2Qc7Zefexu3D1xftKhWJzeAVRwuk+KfbE5lsHxSlmbAc?= =?us-ascii?Q?fvWVDVwb811fSr1ScynL1gsXkA66AOqTbjPiR8L3koVnWrPl34k9hFANj32d?= =?us-ascii?Q?1xAP0E13wVs86cB30Z6eqM1WendJSubiND/3JeRM5FdadrbxMaZ+rehWX22H?= =?us-ascii?Q?jo5Akx4RHfkJ6CZ/x7G435HbxQS4OZZwc8R6ZwQ2aipgOBccvx9aLClSh2P8?= =?us-ascii?Q?zpLgcsys8LTQ7UnBl+hglkRtb5b536pzKqj8dBj+Lf80OmV230eTkfCLlIEL?= =?us-ascii?Q?kLrgnWM0/k=3D?= X-Microsoft-Exchange-Diagnostics: 1;AM2PR08MB0435;5:sJj2iXm33eEbI6V+Lsj4prypro49Z56TLfIArVsz09DGQeBBFhJW81fKNCHdpwTgI878ImPPEUHdK/L4KBZEAzXDYMsNhGavRSzzTSwd/Cr4G7jywQc7UDZTqutzX7QuKfXTzhWWv7OMXucmSfgubw==;24:b6k0ocawbnXa3BITFYEOs9+Z5Qspaz3c6d3XI18io3bQqFBKa7UPrkoF2zlvi7GAse+e3jIrd8CiWaUUK77RmahzYtXiVzM+vlMhnu0CkgQ=;20:Pg8N5g8bOAXx6yvW+Ab0NMMgtdGeU89bzE2W05vjf+okfPwipdQxSV4Cw7xqQrENPALRlA0wAf9jTVDsSb6z4QJpk9GkN445XhSDb5zKTyiyRD7JxWdaeXK8KlcpoomQHRoWUJV8u9X/J67CyDUke5eCs4F8q7Nwy4DhpCOp1mE= SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jan 2016 01:29:41.8067 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM2PR08MB0435 X-MC-Unique: JKnB_An3TIqVbZ-zvZ0IWA-1 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: 8BIT Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 13, 2016 at 02:07:25PM +0100, Thomas Gleixner wrote: > On Wed, 13 Jan 2016, zyjzyj2000@gmail.com wrote: > > > After this commit 71f64340fc0e ("genirq: Remove the second parameter > > from handle_irq_event_percpu()") is applied, the variable action is > > not protected by raw_spin_lock. The following calltrace will pop up. > > Thanks, for the report. I missed that detail when merging the patch! > > Just for correctness sake: You miss to explain why this can happen. > > It's not about the variable action, it's about desc->action not being > protected anymore. So the reason why this oopses is that the action is being > removed concurrently. > > CPU 0 CPU 1 > > free_irq() lock(desc) > lock(desc) handle_edge_irq() > handle_irq_event(desc) > unlock(desc) > desc->action = NULL handle_irq_event_percpu(desc) > action = desc->action > > While the original code did: > > free_irq() lock(desc) > lock(desc) handle_edge_irq() > handle_irq_event() > action = desc->action > unlock(desc) > desc->action = NULL handle_irq_event_percpu(desc, action) > > So now the question is whether we revert that patch or simply change > handle_irq_event_percpu() to deal with that. Patch below. > > That preserves us the code size reduction of commit 71f64340fc0e. This is safe > because we either see a valid desc->action or NULL. If the action is about to > be removed it is still valid as free_irq() is blocked on synchronize_irq(). > > free_irq() lock(desc) > lock(desc) handle_edge_irq() > handle_irq_event(desc) > set(INPROGRESS) > unlock(desc) > handle_irq_event_percpu(desc) > action = desc->action > desc->action = NULL > sychronize_irq() > while(INPROGRESS); lock(desc) > clr(INPROGRESS) > free(action) > > That's basically the same mechanism as we have for shared > interrupts. action->next can become NULL while handle_irq_event_percpu() > runs. Either it sees the action or NULL. It does not matter, because action > itself cannot go away. > > Thanks, > > tglx > > 8<------------- > > --- a/kernel/irq/handle.c > +++ b/kernel/irq/handle.c > @@ -136,9 +136,15 @@ irqreturn_t handle_irq_event_percpu(stru > { > irqreturn_t retval = IRQ_NONE; > unsigned int flags = 0, irq = desc->irq_data.irq; > - struct irqaction *action = desc->action; > + struct irqaction *action; > > - do { > + /* > + * READ_ONCE is not required here. The compiler cannot reload action > + * because it'll be action->next for the second iteration of the loop. > + */ > + action = desc->action; > + > + while (action) { > irqreturn_t res; > > trace_irq_handler_entry(irq, action); > @@ -173,7 +179,7 @@ irqreturn_t handle_irq_event_percpu(stru > > retval |= res; > action = action->next; > - } while (action); > + } > > add_interrupt_randomness(irq, flags); I prefer to this patch, revert the old the patch is not a good solution. thanks Huang Shijie