Date:   Thu, 2 Mar 2023 11:20:49 -0500
From:   Peter Xu <peterx@redhat.com>
To:     Marcelo Tosatti <mtosatti@redhat.com>
Cc:     David Hildenbrand <david@redhat.com>,
        Christoph Lameter <cl@linux.com>,
        Aaron Tomlin <atomlin@atomlin.com>,
        Frederic Weisbecker <frederic@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v2 08/11] mm/vmstat: switch counter modification to
 cmpxchg
Message-ID: <ZADM4U49g+g4S5Xf@x1n>
References: <20230209150150.380060673@redhat.com>
 <20230209153204.846239718@redhat.com>
 <3331790c-95a1-6ab9-2667-86aae3d28d7d@redhat.com>
 <ZAC3BxPIxAplvTzT@tpad>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ZAC3BxPIxAplvTzT@tpad>
Precedence: bulk

On Thu, Mar 02, 2023 at 11:47:35AM -0300, Marcelo Tosatti wrote:
> So it will be:
> 
> #ifdef CONFIG_HAVE_CMPXCHG_LOCAL
> mod_zone_page_state
> inc_zone_page_state
> dec_zone_page_state
> mod_node_page_state
> inc_node_page_state
> dec_node_page_state
> __mod_zone_page_state (new function, calls mod_zone_page_state).
> __mod_node_page_state (new function, calls mod_node_page_state).
> __inc_zone_page_state
> __inc_node_page_state
> __dec_zone_page_state
> __dec_node_page_state
> #else
> __mod_zone_page_state (old, shared function for both CONFIG_HAVE_CMPXCHG_LOCAL and not)
> __mod_node_page_state
> __inc_zone_page_state
> __inc_node_page_state
> __dec_zone_page_state
> __dec_node_page_state
> mod_zone_page_state
> inc_zone_page_state
> dec_zone_page_state
> mod_node_page_state
> inc_node_page_state
> dec_node_page_state
> #endif
> 
> Any suggestion on how to split this into multiple patchsets for easier
> reviewing? (can't think of anything obvious).

I figured this out before saw this, but it did take me some time to read
carefully into the code base..  maybe it'll be a good idea to mention
something like above in the commit message to ease future reviewers (and
more likelyhood to attract the experts to start chim in)?

One fundamental (but maybe another naive.. :) question on this code piece
(so not directly related to the changeset but maybe it is still..):

AFAICT CONFIG_HAVE_CMPXCHG_LOCAL only means we can do cmpxchg() without
locking memory bus, however when !CONFIG_HAVE_CMPXCHG_LOCAL here we're not
using non-local version but using preempt_disable_nested() to make sure the
read is atomic.  Does it really worth it?  What happens if we use cmpxchg()
unconditionally, but just use local (e.g. no "LOCK" prefix) version when
CONFIG_HAVE_CMPXCHG_LOCAL?

-- 
Peter Xu