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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 28 Mar 2023 15:46:23 +0700
From:   Bagas Sanjaya <bagasdotme@...il.com>
To:     Feng Tang <feng.tang@...el.com>
Cc:     Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, Randy Dunlap <rdunlap@...radead.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Joe Mario <jmario@...hat.com>, Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Shakeel Butt <shakeelb@...gle.com>, dave.hansen@...el.com,
        ying.huang@...el.com, tim.c.chen@...el.com, andi.kleen@...el.com
Subject: Re: [PATCH v1] Documentation: Add document for false sharing

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