Re: [PATCH v1] Documentation: Add document for false sharing

[Bagas Sanjaya]

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