On 1/6/22 8:08 PM, mingzhe.zou@xxxxxxxxxxxx wrote:
From: Zou Mingzhe <mingzhe.zou@xxxxxxxxxxxx>
When attaching a cached device (a.k.a backing device) to a cache
device, bch_sectors_dirty_init() is called to count dirty sectors
and stripes (see what bcache_dev_sectors_dirty_add() does) on the
cache device.
When bch_sectors_dirty_init() is called, set_bit(stripe,
d->full_dirty_stripes) or clear_bit(stripe, d->full_dirty_stripes)
operation will always be performed. In full_dirty_stripes, each 1bit
represents stripe_size (8192) sectors (512B), so 1bit=4MB (8192*512),
and each CPU cache line=64B=512bit=2048MB. When 20 threads process
a cached disk with 100G dirty data, a single thread processes about
23M at a time, and 20 threads total 460M. The full_dirty_stripes bit
of these data is likely to fall in the same CPU cache line. This will
cause CPU false sharing problem and reduce performance.
I am fine with the patch, but why "this will cause CPU false sharing" if
"the full_dirty_stripes bit of these day is likely to fall in the same
CPU cache line" ?
This patch tries to test_bit before set_bit or clear_bit operation.
There are 8192 sectors per 1bit, and the number of sectors processed
by a single bch_sectors_dirty_init() call is only 8, 16, 32, etc.
So the set_bit or clear_bit operations will be greatly reduced.
Indeed, force setting value before testing is something to be avoided.
Thanks for caching this. If you may simplify the commit log to explain
your change is to avoid force setting before testing, it should be fine
enough for me.
Thanks.
Coly Li
Signed-off-by: Mingzhe Zou <mingzhe.zou@xxxxxxxxxxxx>
---
drivers/md/bcache/writeback.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 68e75c692dd4..4afe22875d4f 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -596,10 +596,13 @@ void bcache_dev_sectors_dirty_add(struct cache_set *c, unsigned int inode,
sectors_dirty = atomic_add_return(s,
d->stripe_sectors_dirty + stripe);
- if (sectors_dirty == d->stripe_size)
- set_bit(stripe, d->full_dirty_stripes);
- else
- clear_bit(stripe, d->full_dirty_stripes);
+ if (sectors_dirty == d->stripe_size) {
+ if (!test_bit(stripe, d->full_dirty_stripes))
+ set_bit(stripe, d->full_dirty_stripes);
+ } else {
+ if (test_bit(stripe, d->full_dirty_stripes))
+ clear_bit(stripe, d->full_dirty_stripes);
+ }
nr_sectors -= s;
stripe_offset = 0;
