On Thu, Mar 02, 2023 at 04:11:32PM -0300, Marcelo Tosatti wrote: > On Thu, Mar 02, 2023 at 11:20:49AM -0500, Peter Xu wrote: > > 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, > > CONFIG_HAVE_CMPXCHG_LOCAL means cmpxchg_local is implemented (that is > cmpxchg which is atomic with respect to local CPU). > > LOCK cmpxchg is necessary for cmpxchg to be atomic on SMP. > > > 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? > > Can't use local version of cmpxchg because the vmstat counters are supposed > to be accessed from different CPUs simultaneously (this is the objective > of the patchset): > > CPU-0 CPU-1 > > vmstat_shepherd mod_zone_page_state > xchg location LOCK cmpxchg location > > xchg locks memory bus implicitly. > > Is this what you are thinking about or did i misunderstood what you > mean? Yes, I think I wrongly interpreted cmpxchg_local() before on assuming it's still atomic but an accelerated version of cmpxchg() that was only supported on a few archs. I double checked the code and spec on x86 - I believe you're right. Thanks, -- Peter Xu
