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;
Message-ID: <05a6e961-b1a3-cdb2-6baf-ec0559185a5f@gmail.com>
Date:   Tue, 28 Mar 2023 15:46:23 +0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v1] Documentation: Add document for false sharing
Content-Language: en-US
To:     Feng Tang <feng.tang@intel.com>
Cc:     Jonathan Corbet <corbet@lwn.net>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, Randy Dunlap <rdunlap@infradead.org>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Joe Mario <jmario@redhat.com>, Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Eric Dumazet <edumazet@google.com>,
        Shakeel Butt <shakeelb@google.com>, dave.hansen@intel.com,
        ying.huang@intel.com, tim.c.chen@intel.com, andi.kleen@intel.com
References: <20230324071316.167337-1-feng.tang@intel.com>
 <ZB2baIDIPhxj5Vdl@debian.me> <ZCDl1o16eZDx1HW1@feng-clx>
From:   Bagas Sanjaya <bagasdotme@gmail.com>
In-Reply-To: <ZCDl1o16eZDx1HW1@feng-clx>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/27/23 07:39, Feng Tang wrote:
> Hi Bagas Sanjaya,
> 
> Many thanks for the reviews!
> 
> On Fri, Mar 24, 2023 at 07:45:28PM +0700, Bagas Sanjaya wrote:
>> On Fri, Mar 24, 2023 at 03:13:16PM +0800, Feng Tang wrote:
>>> +There are many real-world cases of performance regressions caused by
>>> +false sharing, and one is a rw_semaphore 'mmap_lock' inside struct
>> "... . One of these is rw_semaphore 'mmap_lock' ..."
> 
> OK, will use this.
> 
>> But I think in English we commonly name things as "foobar struct"
>> instead of "struct foobar" (that is, common noun follow the proper noun
>> that names something).
> 
> I can change that. And IIRC, I saw 'struct XXX' and 'XXX struct' both
> frequently used in kernel. I just run '# git log | grep -w struct'
> and the majority use 'struct XXX'
> 
>>> +* A global datum accessed (shared) by many CPUs
>> Global data?
> 
> In RFC version, I used 'data' and Randy suggested 'datum'. TBH, I 
> looked it up in a dictionary :),  and found:
> 	"Data" is the Latin plural form of "datum"
> 

OK, I understand.

>>> +  #perf c2c record -ag sleep 3
>>> +  #perf c2c report --call-graph none -k vmlinux
>>
>> Are these commands really run as root?
> 
> You are right, people can run it as 'root' or a normal user. And I
> guess this won't confuse kernel developers.
> 
> My original version is kind of too long and full of explainations,
> and some kernel developer suggested that this doc is under
> 'kernel-hacking' and its audience is kernel developers, and I should
> make it clear and short, and not make it look like a wiki page or
> man page.
> 

So something like below, right?

```
$ perf <command> <args>...
$ perf <command> <args>...
```

>>> +* Replace 'write' with 'read' when possible, especially in loops.
>>> +  Like for some global variable, use compare(read)-then-write instead
>>> +  of unconditional write. For example, use:
>> "... For example, write::"
> 
> The following is a coding pattern (for bit operation, atomic, etc.),
> and I think 'use' may also be good?
> 

I tend to say "write" when the context is typing code.

>>> +
>>> +	if (!test_bit(XXX))
>>> +		set_bit(XXX);
>>> +
>>> +  instead of directly "set_bit(XXX);", similarly for atomic_t data.
>>> +
>>> +  Commit 7b1002f7cfe5 ("bcache: fixup bcache_dev_sectors_dirty_add() multithreaded CPU false sharing")
>>> +  Commit 292648ac5cf1 ("mm: gup: allow FOLL_PIN to scale in SMP")
>>> +
>>> +* Turn hot global data to 'per-cpu data + global data' when possible,
>>> +  or reasonably increase the threshold for syncing per-cpu data to
>>> +  global data, to reduce or postpone the 'write' to that global data.
>>> +
>>> +  Commit 520f897a3554 ("ext4: use percpu_counters for extent_status cache hits/misses")
>>> +  Commit 56f3547bfa4d ("mm: adjust vm_committed_as_batch according to vm overcommit policy")
>>
>> IMO it's odd to jump to specifying example commits without some sort of
>> conjuction (e.g. "for example, see commit <commit>").
> 
> I agree, and I had the same concern, but I was also afraid of that
> too many repeating of this, so the previous 
> "Following 'mitigation' section provides real-world examples." 
> in last section (which you helped to improve) was added trying
> to address this.
> 

OK.

And see you in v2!

-- 
An old man doll... just what I always wanted! - Clara