Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Aug 2023 21:28:39 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     "Michael Kelley (LINUX)" <mikelley@microsoft.com>
Cc:     "levymitchell0@gmail.com" <levymitchell0@gmail.com>,
        KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "mikelly@microsoft.com" <mikelly@microsoft.com>,
        Boqun Feng <boqun.feng@gmail.com>
Subject: Re: [PATCH] hv_balloon: Update the balloon driver to use the SBRM API
Message-ID: <20230802192839.GB231007@hirez.programming.kicks-ass.net>
References: <20230726-master-v1-1-b2ce6a4538db@gmail.com>
 <BYAPR21MB16889DB462CEA394895BEBDBD70BA@BYAPR21MB1688.namprd21.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BYAPR21MB16889DB462CEA394895BEBDBD70BA@BYAPR21MB1688.namprd21.prod.outlook.com>
Precedence: bulk

On Wed, Aug 02, 2023 at 05:47:55PM +0000, Michael Kelley (LINUX) wrote:
> From: Mitchell Levy via B4 Relay <devnull+levymitchell0.gmail.com@kernel.org> Sent: Tuesday, July 25, 2023 5:24 PM
> >
> > This patch is intended as a proof-of-concept for the new SBRM
> > machinery[1]. For some brief background, the idea behind SBRM is using
> > the __cleanup__ attribute to automatically unlock locks (or otherwise
> > release resources) when they go out of scope, similar to C++ style RAII.
> > This promises some benefits such as making code simpler (particularly
> > where you have lots of goto fail; type constructs) as well as reducing
> > the surface area for certain kinds of bugs.
> > 
> > The changes in this patch should not result in any difference in how the
> > code actually runs (i.e., it's purely an exercise in this new syntax
> > sugar). In one instance SBRM was not appropriate, so I left that part
> > alone, but all other locking/unlocking is handled automatically in this
> > patch.
> > 
> > Link: https://lore.kernel.org/all/20230626125726.GU4253@hirez.programming.kicks-ass.net/ [1]
> 
> I haven't previously seen the "[1]" footnote-style identifier used with the
> Link: tag.  Usually the "[1]" goes at the beginning of the line with the
> additional information, but that conflicts with the Link: tag.  Maybe I'm
> wrong, but you might either omit the footnote-style identifier, or the Link:
> tag, instead of trying to use them together.
> 
> Separately, have you built a kernel for ARM64 with these changes in
> place?  The Hyper-V balloon driver is used on both x86 and ARM64
> architectures.  There's nothing obviously architecture specific here,
> but given that SBRM is new, it might be wise to verify that all is good
> when building and running on ARM64.

The only issue that has popped up so far is that __cleanup__ and
asm-goto don't interact nicely. GCC will silently mis-compile but clang
will issue a compile error/warning.

Specifically, GCC seems to have implemented asm-goto like the 'labels as
values' extention and loose the source context of the edge or something.
With result that the actual goto does not pass through the __cleanup__.

Other than that, it seems to work as expected across the platforms.

A brief look through the patch didn't show me anything odd, should be
ok. Although my primary purpose was to get rid of 'unlock' labels and
error handling, simple usage like this is perfectly fine too.