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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3B6B2C433F5 for ; Sun, 17 Oct 2021 07:44:41 +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 ED7DA61027 for ; Sun, 17 Oct 2021 07:44:40 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org ED7DA61027 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=rXBzwBGQmvzL+Zd1fkSoB/tuCu9s5MG1ZJThYrVdmes=; b=sw8HtbzNHEV7IP i9mJ6aU6eONvVH2ZiOH/48sEHeEOeTmEytf62hQRrNUvKFApno8ySfpIdUlMPKr7A1Q9pk+uUgoDo 3Z7OPAhi9EWKuOHpfDmMxtN5gMkvcFA/RIqpfKzQJh3QZ6FCpaIWc7HQwvf/b2YZZVV+8+hzaqBCi fzX5HS9BaPtTvRPeieIVDMCvZ+GGX41bC5UKKnGFsz7QIMeLsd0YFLBQTMV3czEAhBixFHHvHYWXI 0l6aY+nHhRX/jm69vBj/eFDe+GGRbNDxAUZLAyQdpsLWqF8mAFmzXOv4D0ljCRWnWt9KQ1tNr9FiH PhpTx1jYaXurjBp8zDfQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mc0oi-00Btpv-I8; Sun, 17 Oct 2021 07:42:48 +0000 Received: from mail-pg1-x534.google.com ([2607:f8b0:4864:20::534]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mc0oe-00Btp5-Ky for linux-arm-kernel@lists.infradead.org; Sun, 17 Oct 2021 07:42:46 +0000 Received: by mail-pg1-x534.google.com with SMTP id s136so9628143pgs.4 for ; Sun, 17 Oct 2021 00:42:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=cA31YHoAojFMXfvWIq1mbnRWY4u6SaL5BET0d5T/ktw=; b=qnF4rTeRZDNSrp9fYaIMA+NmTnQdkkydcDZnUGGvt9G7E4U6das5WLceavH1fz7W2R ROi6ZjQ0sJNwTvaHRw0zpTB2g95nWy6L9tDYzEpRFCsPAgj2eUMxpUUQHyB6prfOuvW+ mmWtdhGQjqeRUodYPOLgdzXuOuy7TwemrGhH5qIJ85P9kK3qHcwPpa13bmTAVebAEUQr DsFyJCW8EhN9c7JptPjp3NZjr9wF1vDmzCJWwqGlkcIs3DGu22Hidv/AVOpA8lnT3OLG 0i0hX525+IcXKZaKWU4EFfY1vKFGB/MipL3D3yeyErO/dJsFRQjJBT7bQM3AUBNln1Oe T1NA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=cA31YHoAojFMXfvWIq1mbnRWY4u6SaL5BET0d5T/ktw=; b=MXRN+UfZg2P7Jtm3csRPl1pVAHLUybHYFHbhTIkzKjkYZG9hAumi0TtgNykNfFDfh9 hbmKeDOlLXlzKsjVliYmuaDQbHWo/iMI41JBD9ggJjyefZU+Xrn2GErtX9cr71K9m7/T WVqWW0Ln/ZT32JTKEBpqXOq2LSd3sj7TvbIlEOLd7EV5oitkzas770i6++Bhgzw07C0v TIA2tYWhmlyP837NiYmTv89WGVsndG7yYJOOpnlaNZPjRVyys3zQbUDwP4a95Y1enH9p uSHjWrVjT7LNxriapwj4nK+wFWpVbodZBVXwe8fVsOCQ0yKttRWXnw30+PgLLXf1DGXN tRQQ== X-Gm-Message-State: AOAM5325eklUjVLt7Yk4yMShbaGzxAZDtzTJBQECKoBqf81s9Aw6kvC8 uNH67D8LSqWN7ihZIJep3A== X-Google-Smtp-Source: ABdhPJwDOxO5q67+iounQrxtmhizWndA71VCMYH4kGmfb7B5+E/poyFPWGQKM4KpUlMPudG6gKshTA== X-Received: by 2002:a63:3e84:: with SMTP id l126mr12995435pga.55.1634456561089; Sun, 17 Oct 2021 00:42:41 -0700 (PDT) Received: from piliu.users.ipa.redhat.com ([209.132.188.80]) by smtp.gmail.com with ESMTPSA id n7sm9589763pfd.37.2021.10.17.00.42.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 17 Oct 2021 00:42:40 -0700 (PDT) Date: Sun, 17 Oct 2021 15:42:34 +0800 From: Pingfan Liu To: Linus Torvalds Cc: Mark Rutland , Catalin Marinas , Thomas Gleixner , Will Deacon , Marc Zyngier , Linux Kernel Mailing List , Linux ARM , "Paul E. McKenney" , Peter Zijlstra Subject: Re: [GIT PULL] arm64 fixes for 5.15-rc5 Message-ID: References: <20211011104729.GB1421@C02TD0UTHF1T.local> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211017_004244_740471_0ECC792D X-CRM114-Status: GOOD ( 61.18 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Linus, I am not in the Cc list, so just aware of it the day before yesterday. On Mon, Oct 11, 2021 at 12:54:49PM -0700, Linus Torvalds wrote: > On Mon, Oct 11, 2021 at 3:47 AM Mark Rutland wrote: > > > > Sorry; I agree that commit messages don't explain this thoroughly. I can > > go and rework the commit messages to clarify things, if you'd like? > > So I really hate that patch, even with explanation. > > Having the Kconfig option for "do the right thing" is not how this > should be fixed. > > Either the generic code is generic, or it isn't. > > And if the generic code doesn't work for arm64, we shouldn't add a > random Kconfig option for "this architecture wants different > behavior". > > > The TL;DR is that a bunch of constraints conspire such that we can't > > defer accounting things to the irqdomain or irqchip code without making > > this even more complicated/fragile, and many architectures get this > > subtly wrong today -- it's not that arm64 is necessarily much different > > from other architectures using this code, just that we're trying to > > solve this first. > > So then I think the fix is to just say "accounting in this generic > function is wrong, and the accounting needs to be moved to the > callers". > > That's particularly true if you think arm64 is only the only actually > _tested_ case that gets this wrong, and other architectures most > likely have the exact same issue, but you only fixed it for arm64. > > So do it unconditionally - possibly even using a coccinelle script if > there are lots of callers. > > Because generic code that just isn't generic, but randomly does > different things based on subtle Kconfig options that are different > for different architectures is bad, bad, bad. > When composing the patch, I failed to realize it, but now, I learn. > And yes, I realize that we've had that kind of stuff before (and we > have odd Kconfig option testing in that irqdesc.c file elsewhere), but > the Kconfig options we currently have are mostly either > > (a) actual real honest-to-goodness config options with semantic > meaning (ie things like CONFIG_SMP and CONFIG_NUMA) > > (b) really ugly workarounds for odd special module exports (that > CONFIG_KVM_BOOK3S_64_HV_MODULE thing for irq_to_desc() that we *tried* > but failed to remove). > > And so the reason I really hate that patch is that it introduces a new > "different architectures randomly and inexplicably do different > things, and the generic behavior is very different on arm64 than it is > elsewhere". > > That's just the worst kind of hack to me. > > And in this case, it's really *horribly* hard to see what the call > chain is. It all ends up being actively obfuscated and obscured > through that 'handle_arch_irq' function pointer, that is sometimes set > through set_handle_irq(), and sometimes set directly. > > I really think that if the rule is "we can't do accounting in > handle_domain_irq(), because it's too late for arm64", then the fix > really should be to just not do that. > > Move the irq_enter()/irq_exit() to the callers - quite possibly far up > the call chain to the root of it all, and just say "architecture code > needs to do this in the low-level code before calling > handle_arch_irq". > > Or, if it turns out that 99% of callers do want it - and don't have > the issues arm64 has - maybe we can have a helper wrapper that does > the irq_enter/irq_exit, and another version that doesn't do it, and at > least it's clear to the callers which one it is. But it looks like > particularly with the odd indirection through that handle_arch_irq > function, it's best to just say "this needs to be done". > > What architectures actually care? From what I can follow, it's really > GENERIC_IRQ_MULTI_HANDLER that ends up doing this all, and then arm64 > has it's own special case for no obvious reason (why isn't it using > GENERIC_IRQ_MULTI_HANDLER that seems to do the EXACT same thing in > generic code except for a special "default != NULL" case?) > > Anyway, it _looks_ to me like the pattern is very simple: > > Step 1: > - remove irq_enter/irq_exit from handle_domain_irq(), move it to all callers. > > This clearly doesn't change anything at all, but also doesn't fix the > problem you have. But it's easy to verify that the code is the same > before-and-after. > > Step 2 is the pattern matching step: > > - if the caller of handle_domain_irq() ends up being a function that > is registered with set_handle_irq(), then we > (a) remove the irq_enter/irq_exit from it > (b) add it to the architectures that use handle_arch_irq. > (c) make sure that if there are other callers of it (not through > handle_arch_irq) we move that irq_enter/irq_exit into them too > > I _suspect_ - but didn't check - that Step 2(c) doesn't actually > exist. But who knows. > > It really looks like there is a very tight connection between "uses > handle_domain_irq()" and "uses handle_arch_irq/set_handle_irq()". No? > > Is this a bit more work? Yes. > > Would this fix arm64? Yes - it ends up effectively doing what you did. > > Would this fix _other_ architectures doing the same thing that you > suspect are broken? YES. Instead of leaving them randomly broken. > > And most importantly, it would make the rules for "generic" code > clear, and actually generic. > > Now, it may be that I'm wrong. I'm willing to be convinced, and if you > beat me over the head enough I guess I can take that pull request and > just be unhappy about it. > I had thought if any arch adapts GENERIC_ENTRY, then handle_domain_irq() can be a bug. As GENERIC_ENTRY is a not _random_ config option, so try to re-anchor the config depending on GENERIC_ENTRY. But finally I gave up, since there is no direct link between them at a glance. And what is more, as you said, "the rules for "generic" code clear, and actually generic", so it is better to go in that way. I think Mark has started the work or I will be happy to re-work on these patches. Thanks, Pingfan _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel