From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Christopherson Date: Mon, 1 Nov 2021 17:41:29 +0000 Subject: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration In-Reply-To: <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> Message-ID: List-Id: To: kvm-riscv@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Mon, Nov 01, 2021, Maxim Levitsky wrote: > On Thu, 2021-10-28 at 15:55 +0000, Sean Christopherson wrote: > > On Thu, Oct 28, 2021, Maxim Levitsky wrote: > > > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > > I wish there was a way to mark fields in a struct, as requiring 'READ_ONCE' on them > > > so that compiler would complain if this isn't done, or automatically use 'READ_ONCE' > > > logic. > > > > Hmm, I think you could make an argument that ON and thus the whole "control" > > word should be volatile. AFAICT, tagging just "on" as volatile actually works. > > There's even in a clause in Documentation/process/volatile-considered-harmful.rst > > that calls this out as a (potentially) legitimate use case. > > > > - Pointers to data structures in coherent memory which might be modified > > by I/O devices can, sometimes, legitimately be volatile. > > > > That said, I think I actually prefer forcing the use of READ_ONCE. The descriptor > > requires more protections than what volatile provides, namely that all writes need > > to be atomic. So given that volatile alone isn't sufficient, I'd prefer to have > > the code itself be more self-documenting. > > I took a look at how READ_ONCE/WRITE_ONCE is implemented and indeed they use volatile > (the comment above __READ_ONCE is worth gold...), so there is a bit of contradiction: > > volatile-considered-harmful.rst states not to mark struct members volatile since > you usually need more that than (very true often) and yet, I also heard that > READ_ONCE/WRITE_ONCE is very encouraged to be used to fields that are used in lockless > algorithms, even when not strictly needed, > so why not to just mark the field and then use it normally? I guess that > explicit READ_ONCE/WRITE_ONCE is much more readable/visible that a volatile > in some header file. Are you asking about this PI field in particular, or for any field in general? In this particular case, visibility and documentation is really the only difference, functionally the result is the same. But that's also very much related to why this case gets the exception listed above. The "use it normally" part is also why I don't want to tag the field volatile since writing the field absolutely cannot be done "normally", it must be done atomically, and volatile doesn't capture that detail. If you're asking about fields in general, the "volatile is harmful" guideline is to deter usage of volatile for cases where the field/variable in question is not intrinsically volatile. As the docs call out, using volatile in those cases often leads to worse code generation because the compiler is disallowed from optimizing accesses that are protected through other mechanisms. A good example in x86 KVM is the READ_ONCE(sp->unsync) in mmu_try_to_unsync_pages() to force the compiler to emit a load of sp->unsync after acquiring mmu_unsync_pages_lock. Tagging "unsync" as volatile is unnecessary since the vast majority of its usage is protected by holding a spinlock for write, and would prevent optimizing references in kvm_mmu_get_page() and other flows that are protected by mmu_lock in the legacy MMU. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Christopherson Date: Mon, 01 Nov 2021 17:41:29 +0000 Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration Message-Id: List-Id: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> In-Reply-To: <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Maxim Levitsky Cc: Marc Zyngier , Huacai Chen , Aleksandar Markovic , Paul Mackerras , Anup Patel , Paul Walmsley , Palmer Dabbelt , Albert Ou , Christian Borntraeger , Janosch Frank , Paolo Bonzini , James Morse , Alexandru Elisei , Suzuki K Poulose , Atish Patra , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, David Matlack , Oliver Upton , Jing Zhang On Mon, Nov 01, 2021, Maxim Levitsky wrote: > On Thu, 2021-10-28 at 15:55 +0000, Sean Christopherson wrote: > > On Thu, Oct 28, 2021, Maxim Levitsky wrote: > > > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > > I wish there was a way to mark fields in a struct, as requiring 'READ_ONCE' on them > > > so that compiler would complain if this isn't done, or automatically use 'READ_ONCE' > > > logic. > > > > Hmm, I think you could make an argument that ON and thus the whole "control" > > word should be volatile. AFAICT, tagging just "on" as volatile actually works. > > There's even in a clause in Documentation/process/volatile-considered-harmful.rst > > that calls this out as a (potentially) legitimate use case. > > > > - Pointers to data structures in coherent memory which might be modified > > by I/O devices can, sometimes, legitimately be volatile. > > > > That said, I think I actually prefer forcing the use of READ_ONCE. The descriptor > > requires more protections than what volatile provides, namely that all writes need > > to be atomic. So given that volatile alone isn't sufficient, I'd prefer to have > > the code itself be more self-documenting. > > I took a look at how READ_ONCE/WRITE_ONCE is implemented and indeed they use volatile > (the comment above __READ_ONCE is worth gold...), so there is a bit of contradiction: > > volatile-considered-harmful.rst states not to mark struct members volatile since > you usually need more that than (very true often) and yet, I also heard that > READ_ONCE/WRITE_ONCE is very encouraged to be used to fields that are used in lockless > algorithms, even when not strictly needed, > so why not to just mark the field and then use it normally? I guess that > explicit READ_ONCE/WRITE_ONCE is much more readable/visible that a volatile > in some header file. Are you asking about this PI field in particular, or for any field in general? In this particular case, visibility and documentation is really the only difference, functionally the result is the same. But that's also very much related to why this case gets the exception listed above. The "use it normally" part is also why I don't want to tag the field volatile since writing the field absolutely cannot be done "normally", it must be done atomically, and volatile doesn't capture that detail. If you're asking about fields in general, the "volatile is harmful" guideline is to deter usage of volatile for cases where the field/variable in question is not intrinsically volatile. As the docs call out, using volatile in those cases often leads to worse code generation because the compiler is disallowed from optimizing accesses that are protected through other mechanisms. A good example in x86 KVM is the READ_ONCE(sp->unsync) in mmu_try_to_unsync_pages() to force the compiler to emit a load of sp->unsync after acquiring mmu_unsync_pages_lock. Tagging "unsync" as volatile is unnecessary since the vast majority of its usage is protected by holding a spinlock for write, and would prevent optimizing references in kvm_mmu_get_page() and other flows that are protected by mmu_lock in the legacy MMU. 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 47F81C433F5 for ; Mon, 1 Nov 2021 17:41:40 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id B977B60EB8 for ; Mon, 1 Nov 2021 17:41:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B977B60EB8 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 0E6184B1CF; Mon, 1 Nov 2021 13:41:39 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@google.com Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jOqBn1tBabbx; Mon, 1 Nov 2021 13:41:37 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 919944B1A2; Mon, 1 Nov 2021 13:41:37 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 492B94B162 for ; Mon, 1 Nov 2021 13:41:36 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id oMDa2TGIcD8g for ; Mon, 1 Nov 2021 13:41:34 -0400 (EDT) Received: from mail-pj1-f47.google.com (mail-pj1-f47.google.com [209.85.216.47]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id D06DB4B15C for ; Mon, 1 Nov 2021 13:41:34 -0400 (EDT) Received: by mail-pj1-f47.google.com with SMTP id np3so2347947pjb.4 for ; Mon, 01 Nov 2021 10:41:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=rtTqRA7823rLhhDHR/FeABtCXlD+vQwUZXFhSKbl7nw=; b=otcBvN/3IcuDrr15J0K5CvKDj9ic0jccTokbwB/sVPFst2f0LLD/XC7PPmPO9eJXRu P2OBNUe1RlpO0zOo26l/3DPAdKxfcfvQY3tyTT+VRt2u50F02VgF8OTA49ToznF2ILGt Yy6m85iKMGx1XCrOjugojlIHRrq3kXSGg8wFybO1VALqYGLus50IE17TDpBh8OqfqKK1 AAKVEsJcdEBK8202Jfb1yydMhJbNy9mJ8dChJ1pRBEEAKk0vwWevUCT/VxYAjLKOhhmx ip94OeR4ZQJbCq2qfgBZUaSsgLQoBT6PDF+7fUOqfu8QIWy5qh2RFzJV0Hmhvl6+AMgp M0Sw== 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=rtTqRA7823rLhhDHR/FeABtCXlD+vQwUZXFhSKbl7nw=; b=Oq0Gv0RWraAeMBr9ccQsGyHeBX4Qmj9u6ADVkZnu7+sG9YULtmp1SNPDDd4s/VQEy2 LfqNXigHp0whTaXaRftk5iROBkjnYPbRKdv9kSOjKFjz0KA9kgiQvPBUkbiCCfDHWDlL pYdtTXLBLdMoK677LHZ187G2BaVsr+oybAefWt8bNMMsjGU2kdVUacFkE4k/RBHblkso cdodnYrnTjnOUKMXQRgwwqJdDkfoGKU554IHZfXBpSj5pPNcrcZ24gjkB9m5gEK49QbI DDODl27WKjU/fRN0a5quMalBa76WpvZpIkKdqDTjMK88C2DR+jA+BAu+nxY6oN4cQOIU ALyQ== X-Gm-Message-State: AOAM531MAR5i55xMxt6uGKvXeswasbEYABsL6uchesi+EKz8zjzUbfOs tIsfFu/SJovq1o7FE58dlMNjIg== X-Google-Smtp-Source: ABdhPJyQ2rd8AtzfLKSk7pcRIWMkAF0QH30KhY5NHLgIkgccLRDTnxh33KsvfNWVKvhaR3vGZrvloQ== X-Received: by 2002:a17:90b:3ec6:: with SMTP id rm6mr365778pjb.27.1635788493615; Mon, 01 Nov 2021 10:41:33 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id cv1sm86275pjb.48.2021.11.01.10.41.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 01 Nov 2021 10:41:33 -0700 (PDT) Date: Mon, 1 Nov 2021 17:41:29 +0000 From: Sean Christopherson To: Maxim Levitsky Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration Message-ID: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> Cc: Cornelia Huck , Wanpeng Li , kvm@vger.kernel.org, David Hildenbrand , linux-kernel@vger.kernel.org, Paul Mackerras , Atish Patra , linux-riscv@lists.infradead.org, Claudio Imbrenda , kvmarm@lists.cs.columbia.edu, Janosch Frank , Marc Zyngier , Joerg Roedel , Huacai Chen , Christian Borntraeger , Aleksandar Markovic , Albert Ou , kvm-ppc@vger.kernel.org, Paul Walmsley , David Matlack , linux-arm-kernel@lists.infradead.org, Jim Mattson , Anup Patel , linux-mips@vger.kernel.org, Palmer Dabbelt , kvm-riscv@lists.infradead.org, Paolo Bonzini , Vitaly Kuznetsov X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On Mon, Nov 01, 2021, Maxim Levitsky wrote: > On Thu, 2021-10-28 at 15:55 +0000, Sean Christopherson wrote: > > On Thu, Oct 28, 2021, Maxim Levitsky wrote: > > > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > > I wish there was a way to mark fields in a struct, as requiring 'READ_ONCE' on them > > > so that compiler would complain if this isn't done, or automatically use 'READ_ONCE' > > > logic. > > > > Hmm, I think you could make an argument that ON and thus the whole "control" > > word should be volatile. AFAICT, tagging just "on" as volatile actually works. > > There's even in a clause in Documentation/process/volatile-considered-harmful.rst > > that calls this out as a (potentially) legitimate use case. > > > > - Pointers to data structures in coherent memory which might be modified > > by I/O devices can, sometimes, legitimately be volatile. > > > > That said, I think I actually prefer forcing the use of READ_ONCE. The descriptor > > requires more protections than what volatile provides, namely that all writes need > > to be atomic. So given that volatile alone isn't sufficient, I'd prefer to have > > the code itself be more self-documenting. > > I took a look at how READ_ONCE/WRITE_ONCE is implemented and indeed they use volatile > (the comment above __READ_ONCE is worth gold...), so there is a bit of contradiction: > > volatile-considered-harmful.rst states not to mark struct members volatile since > you usually need more that than (very true often) and yet, I also heard that > READ_ONCE/WRITE_ONCE is very encouraged to be used to fields that are used in lockless > algorithms, even when not strictly needed, > so why not to just mark the field and then use it normally? I guess that > explicit READ_ONCE/WRITE_ONCE is much more readable/visible that a volatile > in some header file. Are you asking about this PI field in particular, or for any field in general? In this particular case, visibility and documentation is really the only difference, functionally the result is the same. But that's also very much related to why this case gets the exception listed above. The "use it normally" part is also why I don't want to tag the field volatile since writing the field absolutely cannot be done "normally", it must be done atomically, and volatile doesn't capture that detail. If you're asking about fields in general, the "volatile is harmful" guideline is to deter usage of volatile for cases where the field/variable in question is not intrinsically volatile. As the docs call out, using volatile in those cases often leads to worse code generation because the compiler is disallowed from optimizing accesses that are protected through other mechanisms. A good example in x86 KVM is the READ_ONCE(sp->unsync) in mmu_try_to_unsync_pages() to force the compiler to emit a load of sp->unsync after acquiring mmu_unsync_pages_lock. Tagging "unsync" as volatile is unnecessary since the vast majority of its usage is protected by holding a spinlock for write, and would prevent optimizing references in kvm_mmu_get_page() and other flows that are protected by mmu_lock in the legacy MMU. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm 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 336BFC433EF for ; Mon, 1 Nov 2021 17:41:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1891D60E54 for ; Mon, 1 Nov 2021 17:41:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230438AbhKARoJ (ORCPT ); Mon, 1 Nov 2021 13:44:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43742 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229947AbhKARoH (ORCPT ); Mon, 1 Nov 2021 13:44:07 -0400 Received: from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com [IPv6:2607:f8b0:4864:20::1036]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6B8A0C061714 for ; Mon, 1 Nov 2021 10:41:34 -0700 (PDT) Received: by mail-pj1-x1036.google.com with SMTP id x16-20020a17090a789000b001a69735b339so508634pjk.5 for ; Mon, 01 Nov 2021 10:41:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=rtTqRA7823rLhhDHR/FeABtCXlD+vQwUZXFhSKbl7nw=; b=otcBvN/3IcuDrr15J0K5CvKDj9ic0jccTokbwB/sVPFst2f0LLD/XC7PPmPO9eJXRu P2OBNUe1RlpO0zOo26l/3DPAdKxfcfvQY3tyTT+VRt2u50F02VgF8OTA49ToznF2ILGt Yy6m85iKMGx1XCrOjugojlIHRrq3kXSGg8wFybO1VALqYGLus50IE17TDpBh8OqfqKK1 AAKVEsJcdEBK8202Jfb1yydMhJbNy9mJ8dChJ1pRBEEAKk0vwWevUCT/VxYAjLKOhhmx ip94OeR4ZQJbCq2qfgBZUaSsgLQoBT6PDF+7fUOqfu8QIWy5qh2RFzJV0Hmhvl6+AMgp M0Sw== 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=rtTqRA7823rLhhDHR/FeABtCXlD+vQwUZXFhSKbl7nw=; b=bOJRui//zeZy+qftajNegRIjeQ5WtKqvHXN+q4F3r6P9c6oluQxy8+ayjWcpw4jtw5 TfOYXjqvHj7fBMHvJIkpqakHYW1iM1uAuD30fauP/mFvQ5i4MffVVEfFboyUmyoEqv0W Vbpl81L7xOHfT5jT8Cqat8PXfjHY2mwUZZqusj9N92frHnKb6Wuavr2K1yUaVSGK6GTW Ljyy2568v6bRMPsEGmAw0e/uc7UoTSrvRlx3sBD5AU9AN0o8aDQCpSJeu1dqZsfNIB50 dkWzTIfF3I95C0t5ziT88b4QTgUDlQEygelEA0orfU4OBzPs4yTbc4P3H3J6ToKWdnZj cwYQ== X-Gm-Message-State: AOAM533aBi0Y9aZJAreFX9gN5FfqxzVXPhEg40IT3YI/NINEKr5+X5eh uqJCwZMKa4ek+I5+V4mky0kNcA== X-Google-Smtp-Source: ABdhPJyQ2rd8AtzfLKSk7pcRIWMkAF0QH30KhY5NHLgIkgccLRDTnxh33KsvfNWVKvhaR3vGZrvloQ== X-Received: by 2002:a17:90b:3ec6:: with SMTP id rm6mr365778pjb.27.1635788493615; Mon, 01 Nov 2021 10:41:33 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id cv1sm86275pjb.48.2021.11.01.10.41.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 01 Nov 2021 10:41:33 -0700 (PDT) Date: Mon, 1 Nov 2021 17:41:29 +0000 From: Sean Christopherson To: Maxim Levitsky Cc: Marc Zyngier , Huacai Chen , Aleksandar Markovic , Paul Mackerras , Anup Patel , Paul Walmsley , Palmer Dabbelt , Albert Ou , Christian Borntraeger , Janosch Frank , Paolo Bonzini , James Morse , Alexandru Elisei , Suzuki K Poulose , Atish Patra , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, David Matlack , Oliver Upton , Jing Zhang Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration Message-ID: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> Precedence: bulk List-ID: X-Mailing-List: linux-mips@vger.kernel.org On Mon, Nov 01, 2021, Maxim Levitsky wrote: > On Thu, 2021-10-28 at 15:55 +0000, Sean Christopherson wrote: > > On Thu, Oct 28, 2021, Maxim Levitsky wrote: > > > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > > I wish there was a way to mark fields in a struct, as requiring 'READ_ONCE' on them > > > so that compiler would complain if this isn't done, or automatically use 'READ_ONCE' > > > logic. > > > > Hmm, I think you could make an argument that ON and thus the whole "control" > > word should be volatile. AFAICT, tagging just "on" as volatile actually works. > > There's even in a clause in Documentation/process/volatile-considered-harmful.rst > > that calls this out as a (potentially) legitimate use case. > > > > - Pointers to data structures in coherent memory which might be modified > > by I/O devices can, sometimes, legitimately be volatile. > > > > That said, I think I actually prefer forcing the use of READ_ONCE. The descriptor > > requires more protections than what volatile provides, namely that all writes need > > to be atomic. So given that volatile alone isn't sufficient, I'd prefer to have > > the code itself be more self-documenting. > > I took a look at how READ_ONCE/WRITE_ONCE is implemented and indeed they use volatile > (the comment above __READ_ONCE is worth gold...), so there is a bit of contradiction: > > volatile-considered-harmful.rst states not to mark struct members volatile since > you usually need more that than (very true often) and yet, I also heard that > READ_ONCE/WRITE_ONCE is very encouraged to be used to fields that are used in lockless > algorithms, even when not strictly needed, > so why not to just mark the field and then use it normally? I guess that > explicit READ_ONCE/WRITE_ONCE is much more readable/visible that a volatile > in some header file. Are you asking about this PI field in particular, or for any field in general? In this particular case, visibility and documentation is really the only difference, functionally the result is the same. But that's also very much related to why this case gets the exception listed above. The "use it normally" part is also why I don't want to tag the field volatile since writing the field absolutely cannot be done "normally", it must be done atomically, and volatile doesn't capture that detail. If you're asking about fields in general, the "volatile is harmful" guideline is to deter usage of volatile for cases where the field/variable in question is not intrinsically volatile. As the docs call out, using volatile in those cases often leads to worse code generation because the compiler is disallowed from optimizing accesses that are protected through other mechanisms. A good example in x86 KVM is the READ_ONCE(sp->unsync) in mmu_try_to_unsync_pages() to force the compiler to emit a load of sp->unsync after acquiring mmu_unsync_pages_lock. Tagging "unsync" as volatile is unnecessary since the vast majority of its usage is protected by holding a spinlock for write, and would prevent optimizing references in kvm_mmu_get_page() and other flows that are protected by mmu_lock in the legacy MMU. 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 236DCC433F5 for ; Mon, 1 Nov 2021 17:42:03 +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 D26AF60EB8 for ; Mon, 1 Nov 2021 17:42:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D26AF60EB8 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.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=I4iKMz8LMkuVeUn9hzEY+3Lyg3mWvdnjhIjXA5pkaPY=; b=EJKSEjOHMz/Xa+ KG4SS8D17ZRwUJhYkGvQp/R+qpJndFFjAtnk3YDAGnJf37hAfSYU9df1pWOZbIN+GHknPB+ZXulSm D9zSadZs9YkKpCtIK87ixTx6GpIXUpDEZHa8QbvCSrimXNcB4fuBNQW7goEIl8/oIIftlcwZFlEMV p9Typ8aWKEYvlVFTs/k6V67qpfWbckLrlbs0L4Uz0cUF37YAJWRK/f/MN6pqZ6qYCAFXyUDDQYjk2 ImVWAkahCKKLYxl/CBHxmp5t2nuqabf1BqSqUWRLVX5iEAt+PKzjEQVDpgEG8cdpNcfNJjD3vJAGO Jo3w6ODzUwq4e4GVeq9A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mhbJe-00Gvi4-SM; Mon, 01 Nov 2021 17:41:50 +0000 Received: from mail-pj1-x1034.google.com ([2607:f8b0:4864:20::1034]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mhbJR-00GveA-S3 for linux-riscv@lists.infradead.org; Mon, 01 Nov 2021 17:41:39 +0000 Received: by mail-pj1-x1034.google.com with SMTP id o10-20020a17090a3d4a00b001a6555878a8so5057908pjf.1 for ; Mon, 01 Nov 2021 10:41:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=rtTqRA7823rLhhDHR/FeABtCXlD+vQwUZXFhSKbl7nw=; b=otcBvN/3IcuDrr15J0K5CvKDj9ic0jccTokbwB/sVPFst2f0LLD/XC7PPmPO9eJXRu P2OBNUe1RlpO0zOo26l/3DPAdKxfcfvQY3tyTT+VRt2u50F02VgF8OTA49ToznF2ILGt Yy6m85iKMGx1XCrOjugojlIHRrq3kXSGg8wFybO1VALqYGLus50IE17TDpBh8OqfqKK1 AAKVEsJcdEBK8202Jfb1yydMhJbNy9mJ8dChJ1pRBEEAKk0vwWevUCT/VxYAjLKOhhmx ip94OeR4ZQJbCq2qfgBZUaSsgLQoBT6PDF+7fUOqfu8QIWy5qh2RFzJV0Hmhvl6+AMgp M0Sw== 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=rtTqRA7823rLhhDHR/FeABtCXlD+vQwUZXFhSKbl7nw=; b=BDkDYeHyFOsyPy/hUQbyUQDBIKeOyfTMxK2pNxl1JiQeltoDrp//8ApTdpUagTEDUI xPGwI/8PHfHO9+3OBBq0u5X94SwRtvByPua0tttD/PHURmDdyg3qYQIxsDFhiKR27Ov/ WaIBnvWuzHuIv7vuDuEYDtZOrsFjp7CoNcd9VFZhkQcJokB3jKgyihqZmClTTIyQxyK1 1aPoiN+3CRX65UV2boKdYw52gcgqwI+Zly8chJxdE8ReyqOBnFRP3zWKRdQ1X7lyaxGe uWVAc4sp+VncS3OjKLjVF5wxvsa8q9GCjzJWFozkoeExBEMynDOQg1YTpoBDZJoJ2bf0 D/eg== X-Gm-Message-State: AOAM530gssJgc2HQfUCJpoCyYTpF8Uc1qCLQ3QBfKLZBHJt54IRTASuH KUZOte3d3opLYWiuXuJNMZgQwQ== X-Google-Smtp-Source: ABdhPJyQ2rd8AtzfLKSk7pcRIWMkAF0QH30KhY5NHLgIkgccLRDTnxh33KsvfNWVKvhaR3vGZrvloQ== X-Received: by 2002:a17:90b:3ec6:: with SMTP id rm6mr365778pjb.27.1635788493615; Mon, 01 Nov 2021 10:41:33 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id cv1sm86275pjb.48.2021.11.01.10.41.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 01 Nov 2021 10:41:33 -0700 (PDT) Date: Mon, 1 Nov 2021 17:41:29 +0000 From: Sean Christopherson To: Maxim Levitsky Cc: Marc Zyngier , Huacai Chen , Aleksandar Markovic , Paul Mackerras , Anup Patel , Paul Walmsley , Palmer Dabbelt , Albert Ou , Christian Borntraeger , Janosch Frank , Paolo Bonzini , James Morse , Alexandru Elisei , Suzuki K Poulose , Atish Patra , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, David Matlack , Oliver Upton , Jing Zhang Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration Message-ID: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211101_104137_924314_50279C03 X-CRM114-Status: GOOD ( 27.83 ) X-BeenThere: linux-riscv@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-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Mon, Nov 01, 2021, Maxim Levitsky wrote: > On Thu, 2021-10-28 at 15:55 +0000, Sean Christopherson wrote: > > On Thu, Oct 28, 2021, Maxim Levitsky wrote: > > > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > > I wish there was a way to mark fields in a struct, as requiring 'READ_ONCE' on them > > > so that compiler would complain if this isn't done, or automatically use 'READ_ONCE' > > > logic. > > > > Hmm, I think you could make an argument that ON and thus the whole "control" > > word should be volatile. AFAICT, tagging just "on" as volatile actually works. > > There's even in a clause in Documentation/process/volatile-considered-harmful.rst > > that calls this out as a (potentially) legitimate use case. > > > > - Pointers to data structures in coherent memory which might be modified > > by I/O devices can, sometimes, legitimately be volatile. > > > > That said, I think I actually prefer forcing the use of READ_ONCE. The descriptor > > requires more protections than what volatile provides, namely that all writes need > > to be atomic. So given that volatile alone isn't sufficient, I'd prefer to have > > the code itself be more self-documenting. > > I took a look at how READ_ONCE/WRITE_ONCE is implemented and indeed they use volatile > (the comment above __READ_ONCE is worth gold...), so there is a bit of contradiction: > > volatile-considered-harmful.rst states not to mark struct members volatile since > you usually need more that than (very true often) and yet, I also heard that > READ_ONCE/WRITE_ONCE is very encouraged to be used to fields that are used in lockless > algorithms, even when not strictly needed, > so why not to just mark the field and then use it normally? I guess that > explicit READ_ONCE/WRITE_ONCE is much more readable/visible that a volatile > in some header file. Are you asking about this PI field in particular, or for any field in general? In this particular case, visibility and documentation is really the only difference, functionally the result is the same. But that's also very much related to why this case gets the exception listed above. The "use it normally" part is also why I don't want to tag the field volatile since writing the field absolutely cannot be done "normally", it must be done atomically, and volatile doesn't capture that detail. If you're asking about fields in general, the "volatile is harmful" guideline is to deter usage of volatile for cases where the field/variable in question is not intrinsically volatile. As the docs call out, using volatile in those cases often leads to worse code generation because the compiler is disallowed from optimizing accesses that are protected through other mechanisms. A good example in x86 KVM is the READ_ONCE(sp->unsync) in mmu_try_to_unsync_pages() to force the compiler to emit a load of sp->unsync after acquiring mmu_unsync_pages_lock. Tagging "unsync" as volatile is unnecessary since the vast majority of its usage is protected by holding a spinlock for write, and would prevent optimizing references in kvm_mmu_get_page() and other flows that are protected by mmu_lock in the legacy MMU. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv 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 A85D7C433F5 for ; Mon, 1 Nov 2021 17:43:10 +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 6B14B60EB8 for ; Mon, 1 Nov 2021 17:43:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 6B14B60EB8 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.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=IBswgOYjydNdKisU1+EnLyCOIHkkvDmYGOdT7/NHgUo=; b=S/5KK/5j0r2e+U G72CUl40a7awrATT51HZDCu6ZrXTuR7lIiV0COKJDVrgb9qdYZhSRbDg1tK3UK9PbEfU8RgNyFNaP zCnumMKlMmzKZfb4w3fZO2dLKCm7Be+NK+tAnOWgThGTmH3qNS3ww1t7/WzwnO9mfXq0p6EfENixu B3RIyzpV/ZN3kOI7LizyWivJ5ozgLpvHL/yWjbqOhceYeP3ym2hwVHpEM7GMswhy61xN2UJQfTkLz C6oPb+jcMX8KIqsOcngWd+BfBqzDYPTddqneRuOEGt0tXHxcLeIj5jPHxX+YLUKDVU1sJzsL2q09o 2qP/tOU/XUlane7QVvSg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mhbJV-00Gvgh-OQ; Mon, 01 Nov 2021 17:41:41 +0000 Received: from mail-pj1-x102e.google.com ([2607:f8b0:4864:20::102e]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mhbJR-00Gve8-K4 for linux-arm-kernel@lists.infradead.org; Mon, 01 Nov 2021 17:41:39 +0000 Received: by mail-pj1-x102e.google.com with SMTP id gn3so12623541pjb.0 for ; Mon, 01 Nov 2021 10:41:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=rtTqRA7823rLhhDHR/FeABtCXlD+vQwUZXFhSKbl7nw=; b=otcBvN/3IcuDrr15J0K5CvKDj9ic0jccTokbwB/sVPFst2f0LLD/XC7PPmPO9eJXRu P2OBNUe1RlpO0zOo26l/3DPAdKxfcfvQY3tyTT+VRt2u50F02VgF8OTA49ToznF2ILGt Yy6m85iKMGx1XCrOjugojlIHRrq3kXSGg8wFybO1VALqYGLus50IE17TDpBh8OqfqKK1 AAKVEsJcdEBK8202Jfb1yydMhJbNy9mJ8dChJ1pRBEEAKk0vwWevUCT/VxYAjLKOhhmx ip94OeR4ZQJbCq2qfgBZUaSsgLQoBT6PDF+7fUOqfu8QIWy5qh2RFzJV0Hmhvl6+AMgp M0Sw== 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=rtTqRA7823rLhhDHR/FeABtCXlD+vQwUZXFhSKbl7nw=; b=P4mB/IYN5jBwlhs/iiGEPKQgWxAl9gwC2nQzVTlke0eQaqotayAGo9qHiKOYA4vGWR qNEfzvJMWKgilRBaPmNUeXEpi9zeRX2LgF9QEUQBS8EECp9obnS8mWQriz2PvR8Y7LmN trdfDIvmAHRFV0gNduHRawa7S8ZIpF3/5/uCEzioRXjVh+6JAeBHj7tnLS+HwW6NmNmZ D+h3YoEHJm3ajGfv4rwX/LqE6OCswEiOLNtcLmX2mXsCgZYnPdf7GOBdbJ97VFM215CM uU6rgI1XrLdmO3RoIhScp+dn3W1/QZaxg3uBryxh/R2xuarSZ+2KjT+aykqlr7J27y94 0gJg== X-Gm-Message-State: AOAM533t+95QxuZTFOXyGvMrMUZpKaZ6EHTQ2g2zBiGje9JvLbb1wMv4 wphuISnun3fIFpMkoil8kzoCIg== X-Google-Smtp-Source: ABdhPJyQ2rd8AtzfLKSk7pcRIWMkAF0QH30KhY5NHLgIkgccLRDTnxh33KsvfNWVKvhaR3vGZrvloQ== X-Received: by 2002:a17:90b:3ec6:: with SMTP id rm6mr365778pjb.27.1635788493615; Mon, 01 Nov 2021 10:41:33 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id cv1sm86275pjb.48.2021.11.01.10.41.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 01 Nov 2021 10:41:33 -0700 (PDT) Date: Mon, 1 Nov 2021 17:41:29 +0000 From: Sean Christopherson To: Maxim Levitsky Cc: Marc Zyngier , Huacai Chen , Aleksandar Markovic , Paul Mackerras , Anup Patel , Paul Walmsley , Palmer Dabbelt , Albert Ou , Christian Borntraeger , Janosch Frank , Paolo Bonzini , James Morse , Alexandru Elisei , Suzuki K Poulose , Atish Patra , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, David Matlack , Oliver Upton , Jing Zhang Subject: Re: [PATCH v2 26/43] KVM: VMX: Read Posted Interrupt "control" exactly once per loop iteration Message-ID: References: <20211009021236.4122790-1-seanjc@google.com> <20211009021236.4122790-27-seanjc@google.com> <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20a17d75855dfb9bd496466fcd9f14baab0b2bda.camel@redhat.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211101_104137_691774_5D1B6716 X-CRM114-Status: GOOD ( 29.46 ) 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 On Mon, Nov 01, 2021, Maxim Levitsky wrote: > On Thu, 2021-10-28 at 15:55 +0000, Sean Christopherson wrote: > > On Thu, Oct 28, 2021, Maxim Levitsky wrote: > > > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > > I wish there was a way to mark fields in a struct, as requiring 'READ_ONCE' on them > > > so that compiler would complain if this isn't done, or automatically use 'READ_ONCE' > > > logic. > > > > Hmm, I think you could make an argument that ON and thus the whole "control" > > word should be volatile. AFAICT, tagging just "on" as volatile actually works. > > There's even in a clause in Documentation/process/volatile-considered-harmful.rst > > that calls this out as a (potentially) legitimate use case. > > > > - Pointers to data structures in coherent memory which might be modified > > by I/O devices can, sometimes, legitimately be volatile. > > > > That said, I think I actually prefer forcing the use of READ_ONCE. The descriptor > > requires more protections than what volatile provides, namely that all writes need > > to be atomic. So given that volatile alone isn't sufficient, I'd prefer to have > > the code itself be more self-documenting. > > I took a look at how READ_ONCE/WRITE_ONCE is implemented and indeed they use volatile > (the comment above __READ_ONCE is worth gold...), so there is a bit of contradiction: > > volatile-considered-harmful.rst states not to mark struct members volatile since > you usually need more that than (very true often) and yet, I also heard that > READ_ONCE/WRITE_ONCE is very encouraged to be used to fields that are used in lockless > algorithms, even when not strictly needed, > so why not to just mark the field and then use it normally? I guess that > explicit READ_ONCE/WRITE_ONCE is much more readable/visible that a volatile > in some header file. Are you asking about this PI field in particular, or for any field in general? In this particular case, visibility and documentation is really the only difference, functionally the result is the same. But that's also very much related to why this case gets the exception listed above. The "use it normally" part is also why I don't want to tag the field volatile since writing the field absolutely cannot be done "normally", it must be done atomically, and volatile doesn't capture that detail. If you're asking about fields in general, the "volatile is harmful" guideline is to deter usage of volatile for cases where the field/variable in question is not intrinsically volatile. As the docs call out, using volatile in those cases often leads to worse code generation because the compiler is disallowed from optimizing accesses that are protected through other mechanisms. A good example in x86 KVM is the READ_ONCE(sp->unsync) in mmu_try_to_unsync_pages() to force the compiler to emit a load of sp->unsync after acquiring mmu_unsync_pages_lock. Tagging "unsync" as volatile is unnecessary since the vast majority of its usage is protected by holding a spinlock for write, and would prevent optimizing references in kvm_mmu_get_page() and other flows that are protected by mmu_lock in the legacy MMU. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel