block: account io: kill atomic operations

PATCH In review Ming Lei 3 participants 12 messages Jul 16, 2015

Subsystems (3)

Block layer Device mapper libnvdimm: non-volatile memory device

lore

Share & tools

lore

Ming Lei

author

2015-07-16 03:19 UTC #1 lore raw

PATCH 1/4 block: partition: introduce hd_free_part()

So the helper can be used in both generic partition case and part0 case.

Signed-off-by Ming Lei <[email protected]>

---
 block/genhd.c             | 3 +--
 block/partition-generic.c | 3 +--
 include/linux/genhd.h     | 6 ++++++
 3 files changed, 8 insertions(+), 4 deletions(-)

block/genhd.c +1 -2

index e552e1b..ed3f5b9 100644
@@ -1110,8 +1110,7 @@ static void disk_release(struct device *dev)
 	disk_release_events(disk);
 	kfree(disk->random);
 	disk_replace_part_tbl(disk, NULL);
-	free_part_stats(&disk->part0);
-	free_part_info(&disk->part0);
+	hd_free_part(&disk->part0);
 	if (disk->queue)
 		blk_put_queue(disk->queue);
 	kfree(disk);

block/partition-generic.c +1 -2

index 0d9e5f9..eca0d02 100644
@@ -212,8 +212,7 @@ static void part_release(struct device *dev)
 {
 	struct hd_struct *p = dev_to_part(dev);
 	blk_free_devt(dev->devt);
-	free_part_stats(p);
-	free_part_info(p);
+	hd_free_part(p);
 	kfree(p);
 }

include/linux/genhd.h +6 -0

index ec274e0..a221220 100644
@@ -663,6 +663,12 @@ static inline void hd_struct_put(struct hd_struct *part)
 		__delete_partition(part);
 }
 
+static inline void hd_free_part(struct hd_struct *part)
+{
+	free_part_stats(part);
+	free_part_info(part);
+}
+
 /*
  * Any access of part->nr_sects which is not protected by partition
  * bd_mutex or gendisk bdev bd_mutex, should be done using this

Signature · 1 line

1.9.1

Ming Lei

author

2015-07-16 03:19 UTC #2 lore raw

PATCH 2/4 block: partition: convert percpu ref

In reply to Ming Lei

Percpu refcount is the perfect match for partition's case, and the conversion is quite straight.

With the convertion, one pair of atomic inc/dec can be saved for accounting block I/O, which is run in hot path of block I/O.

Signed-off-by Ming Lei <[email protected]>

---
 block/genhd.c             |  6 +++++-
 block/partition-generic.c |  9 +++++----
 include/linux/genhd.h     | 27 +++++++++++++++++----------
 3 files changed, 27 insertions(+), 15 deletions(-)

block/genhd.c +5 -1

index ed3f5b9..3213b66 100644
@@ -1284,7 +1284,11 @@ struct gendisk *alloc_disk_node(int minors, int node_id)
 		 * converted to make use of bd_mutex and sequence counters.
 		 */
 		seqcount_init(&disk->part0.nr_sects_seq);
-		hd_ref_init(&disk->part0);
+		if (hd_ref_init(&disk->part0)) {
+			hd_free_part(&disk->part0);
+			kfree(disk);
+			return NULL;
+		}
 
 		disk->minors = minors;
 		rand_initialize_disk(disk);

block/partition-generic.c +5 -4

index eca0d02..e771113 100644
@@ -232,8 +232,9 @@ static void delete_partition_rcu_cb(struct rcu_head *head)
 	put_device(part_to_dev(part));
 }
 
-void __delete_partition(struct hd_struct *part)
+void __delete_partition(struct percpu_ref *ref)
 {
+	struct hd_struct *part = container_of(ref, struct hd_struct, ref);
 	call_rcu(&part->rcu_head, delete_partition_rcu_cb);
 }

@@ -254,7 +255,7 @@ void delete_partition(struct gendisk *disk, int partno)
 	kobject_put(part->holder_dir);
 	device_del(part_to_dev(part));
 
-	hd_struct_put(part);
+	hd_struct_kill(part);
 }
 
 static ssize_t whole_disk_show(struct device *dev,

@@ -355,8 +356,8 @@ struct hd_struct *add_partition(struct gendisk *disk, int partno,
 	if (!dev_get_uevent_suppress(ddev))
 		kobject_uevent(&pdev->kobj, KOBJ_ADD);
 
-	hd_ref_init(p);
-	return p;
+	if (!hd_ref_init(p))
+		return p;
 
 out_free_info:
 	free_part_info(p);

include/linux/genhd.h +17 ~~-10~~

index a221220..2adbfa6 100644
@@ -13,6 +13,7 @@
 #include <linux/kdev_t.h>
 #include <linux/rcupdate.h>
 #include <linux/slab.h>
+#include <linux/percpu-refcount.h>
 
 #ifdef CONFIG_BLOCK

@@ -124,7 +125,7 @@ struct hd_struct {
 #else
 	struct disk_stats dkstats;
 #endif
-	atomic_t ref;
+	struct percpu_ref ref;
 	struct rcu_head rcu_head;
 };

@@ -611,7 +612,7 @@ extern struct hd_struct * __must_check add_partition(struct gendisk *disk,
 						     sector_t len, int flags,
 						     struct partition_meta_info
 						       *info);
-extern void __delete_partition(struct hd_struct *);
+extern void __delete_partition(struct percpu_ref *);
 extern void delete_partition(struct gendisk *, int);
 extern void printk_all_partitions(void);

@@ -640,33 +641,39 @@ extern ssize_t part_fail_store(struct device *dev,
 			       const char *buf, size_t count);
 #endif /* CONFIG_FAIL_MAKE_REQUEST */
 
-static inline void hd_ref_init(struct hd_struct *part)
+static inline int hd_ref_init(struct hd_struct *part)
 {
-	atomic_set(&part->ref, 1);
-	smp_mb();
+	if (percpu_ref_init(&part->ref, __delete_partition, 0,
+				GFP_KERNEL))
+		return -ENOMEM;
+	return 0;
 }
 
 static inline void hd_struct_get(struct hd_struct *part)
 {
-	atomic_inc(&part->ref);
-	smp_mb__after_atomic();
+	percpu_ref_get(&part->ref);
 }
 
 static inline int hd_struct_try_get(struct hd_struct *part)
 {
-	return atomic_inc_not_zero(&part->ref);
+	return percpu_ref_tryget_live(&part->ref);
 }
 
 static inline void hd_struct_put(struct hd_struct *part)
 {
-	if (atomic_dec_and_test(&part->ref))
-		__delete_partition(part);
+	percpu_ref_put(&part->ref);
+}
+
+static inline void hd_struct_kill(struct hd_struct *part)
+{
+	percpu_ref_kill(&part->ref);
 }
 
 static inline void hd_free_part(struct hd_struct *part)
 {
 	free_part_stats(part);
 	free_part_info(part);
+	percpu_ref_exit(&part->ref);
 }
 
 /*

Signature · 1 line

1.9.1

Tejun Heo

Acked-by on v1 2/4

2015-07-16 14:36 UTC #3 lore raw

Re: [PATCH 2/4] block: partition: convert percpu ref

Quoted 7 lines · Ming Lei

Percpu refcount is the perfect match for partition's case, and the conversion is quite straight.
With the convertion, one pair of atomic inc/dec can be saved for accounting block I/O, which is run in hot path of block I/O.
Signed-off-by Ming Lei <[email protected]>

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Ming Lei

author

2015-07-16 03:19 UTC #4 lore raw

PATCH 3/4 block: partition: introduce 'cpu' para to part_inc|dec_in_flight

In reply to Ming Lei

So that it is easier to convert part->in_flight[rw] into percpu variable in the following patch.

Signed-off-by Ming Lei <[email protected]>

---
 block/bio.c           | 4 ++--
 block/blk-core.c      | 4 ++--
 block/blk-merge.c     | 2 +-
 drivers/nvdimm/core.c | 4 ++--
 include/linux/genhd.h | 4 ++--
 5 files changed, 9 insertions(+), 9 deletions(-)

block/bio.c +2 -2

index 2a00d34..fe8807f 100644
@@ -1724,7 +1724,7 @@ void generic_start_io_acct(int rw, unsigned long sectors,
 	part_round_stats(cpu, part);
 	part_stat_inc(cpu, part, ios[rw]);
 	part_stat_add(cpu, part, sectors[rw], sectors);
-	part_inc_in_flight(part, rw);
+	part_inc_in_flight(cpu, part, rw);
 
 	part_stat_unlock();
 }

@@ -1738,7 +1738,7 @@ void generic_end_io_acct(int rw, struct hd_struct *part,
 
 	part_stat_add(cpu, part, ticks[rw], duration);
 	part_round_stats(cpu, part);
-	part_dec_in_flight(part, rw);
+	part_dec_in_flight(cpu, part, rw);
 
 	part_stat_unlock();
 }

block/blk-core.c +2 -2

index 82819e6..f180a6d 100644
@@ -2194,7 +2194,7 @@ void blk_account_io_done(struct request *req)
 		part_stat_inc(cpu, part, ios[rw]);
 		part_stat_add(cpu, part, ticks[rw], duration);
 		part_round_stats(cpu, part);
-		part_dec_in_flight(part, rw);
+		part_dec_in_flight(cpu, part, rw);
 
 		hd_struct_put(part);
 		part_stat_unlock();

@@ -2252,7 +2252,7 @@ void blk_account_io_start(struct request *rq, bool new_io)
 			hd_struct_get(part);
 		}
 		part_round_stats(cpu, part);
-		part_inc_in_flight(part, rw);
+		part_inc_in_flight(cpu, part, rw);
 		rq->part = part;
 	}

block/blk-merge.c +1 -1

index 30a0d9f..cb7c46d 100644
@@ -449,7 +449,7 @@ static void blk_account_io_merge(struct request *req)
 		part = req->part;
 
 		part_round_stats(cpu, part);
-		part_dec_in_flight(part, rq_data_dir(req));
+		part_dec_in_flight(cpu, part, rq_data_dir(req));
 
 		hd_struct_put(part);
 		part_stat_unlock();

drivers/nvdimm/core.c +2 -2

index cb62ec6..053d026 100644
@@ -224,7 +224,7 @@ void __nd_iostat_start(struct bio *bio, unsigned long *start)
 	part_round_stats(cpu, &disk->part0);
 	part_stat_inc(cpu, &disk->part0, ios[rw]);
 	part_stat_add(cpu, &disk->part0, sectors[rw], bio_sectors(bio));
-	part_inc_in_flight(&disk->part0, rw);
+	part_inc_in_flight(cpu, &disk->part0, rw);
 	part_stat_unlock();
 }
 EXPORT_SYMBOL(__nd_iostat_start);

@@ -238,7 +238,7 @@ void nd_iostat_end(struct bio *bio, unsigned long start)
 
 	part_stat_add(cpu, &disk->part0, ticks[rw], duration);
 	part_round_stats(cpu, &disk->part0);
-	part_dec_in_flight(&disk->part0, rw);
+	part_dec_in_flight(cpu, &disk->part0, rw);
 	part_stat_unlock();
 }
 EXPORT_SYMBOL(nd_iostat_end);

include/linux/genhd.h +2 -2

index 2adbfa6..612ae80 100644
@@ -381,14 +381,14 @@ static inline void free_part_stats(struct hd_struct *part)
 #define part_stat_sub(cpu, gendiskp, field, subnd)			\
 	part_stat_add(cpu, gendiskp, field, -subnd)
 
-static inline void part_inc_in_flight(struct hd_struct *part, int rw)
+static inline void part_inc_in_flight(int cpu, struct hd_struct *part, int rw)
 {
 	atomic_inc(&part->in_flight[rw]);
 	if (part->partno)
 		atomic_inc(&part_to_disk(part)->part0.in_flight[rw]);
 }
 
-static inline void part_dec_in_flight(struct hd_struct *part, int rw)
+static inline void part_dec_in_flight(int cpu, struct hd_struct *part, int rw)
 {
 	atomic_dec(&part->in_flight[rw]);
 	if (part->partno)

Signature · 1 line

1.9.1

Ming Lei

author

2015-07-16 03:20 UTC #5 lore raw

PATCH 4/4 block: account io: convert part->in_fligh[] into percpu variable

In reply to Ming Lei

So the atomic operations for accounting block I/O can be killed completely, and it is OK to add the percpu variables in part_in_flight() because the function is run at most one time in every tick.

Signed-off-by Ming Lei <[email protected]>

---
 block/blk-core.c          |  1 +
 block/partition-generic.c |  5 +++--
 drivers/md/dm.c           | 10 ++++++----
 include/linux/genhd.h     | 24 ++++++++++++++++++------
 4 files changed, 28 insertions(+), 12 deletions(-)

block/blk-core.c +1 -0

index f180a6d..0001d4c 100644
@@ -1344,6 +1344,7 @@ static void part_round_stats_single(int cpu, struct hd_struct *part,
 	if (now == part->stamp)
 		return;
 
+	/* at most one percpu addition per one tick */
 	inflight = part_in_flight(part);
 	if (inflight) {
 		__part_stat_add(cpu, part, time_in_queue,

block/partition-generic.c +3 -2

index e771113..0a553e7 100644
@@ -140,8 +140,9 @@ ssize_t part_inflight_show(struct device *dev,
 {
 	struct hd_struct *p = dev_to_part(dev);
 
-	return sprintf(buf, "%8u %8u\n", atomic_read(&p->in_flight[0]),
-		atomic_read(&p->in_flight[1]));
+	return sprintf(buf, "%8u %8u\n",
+			part_stat_read(p, in_flight[0]),
+			part_stat_read(p, in_flight[1]));
 }
 
 #ifdef CONFIG_FAIL_MAKE_REQUEST

drivers/md/dm.c +6 -4

index de70377..1b6d8be 100644
@@ -651,9 +651,9 @@ static void start_io_acct(struct dm_io *io)
 
 	cpu = part_stat_lock();
 	part_round_stats(cpu, &dm_disk(md)->part0);
+	part_stat_set(cpu, &dm_disk(md)->part0, in_flight[rw],
+			atomic_inc_return(&md->pending[rw]));
 	part_stat_unlock();
-	atomic_set(&dm_disk(md)->part0.in_flight[rw],
-		atomic_inc_return(&md->pending[rw]));
 
 	if (unlikely(dm_stats_used(&md->stats)))
 		dm_stats_account_io(&md->stats, bio->bi_rw, bio->bi_iter.bi_sector,

@@ -665,7 +665,7 @@ static void end_io_acct(struct dm_io *io)
 	struct mapped_device *md = io->md;
 	struct bio *bio = io->bio;
 	unsigned long duration = jiffies - io->start_time;
-	int pending;
+	int pending, cpu;
 	int rw = bio_data_dir(bio);
 
 	generic_end_io_acct(rw, &dm_disk(md)->part0, io->start_time);

@@ -679,7 +679,9 @@ static void end_io_acct(struct dm_io *io)
 	 * a flush.
 	 */
 	pending = atomic_dec_return(&md->pending[rw]);
-	atomic_set(&dm_disk(md)->part0.in_flight[rw], pending);
+	cpu = part_stat_lock();
+	part_stat_set(cpu, &dm_disk(md)->part0, in_flight[rw], pending);
+	part_stat_unlock();
 	pending += atomic_read(&md->pending[rw^0x1]);
 
 	/* nudge anyone waiting on suspend queue */

include/linux/genhd.h +18 -6

index 612ae80..abe5567 100644
@@ -86,6 +86,7 @@ struct disk_stats {
 	unsigned long ticks[2];
 	unsigned long io_ticks;
 	unsigned long time_in_queue;
+	unsigned int  in_flight[2];
 };
 
 #define PARTITION_META_INFO_VOLNAMELTH	64

@@ -119,7 +120,6 @@ struct hd_struct {
 	int make_it_fail;
 #endif
 	unsigned long stamp;
-	atomic_t in_flight[2];
 #ifdef	CONFIG_SMP
 	struct disk_stats __percpu *dkstats;
 #else

@@ -320,6 +320,9 @@ extern struct hd_struct *disk_map_sector_rcu(struct gendisk *disk,
 	res;								\
 })
 
+#define part_stat_set(cpu, part, field, seted)			\
+	(per_cpu_ptr((part)->dkstats, (cpu))->field = (seted))
+
 static inline void part_stat_set_all(struct hd_struct *part, int value)
 {
 	int i;

@@ -351,6 +354,9 @@ static inline void free_part_stats(struct hd_struct *part)
 
 #define part_stat_read(part, field)	((part)->dkstats.field)
 
+#define part_stat_set(cpu, part, field, seted)			\
+	((part)->dkstats.field = (seted))
+
 static inline void part_stat_set_all(struct hd_struct *part, int value)
 {
 	memset(&part->dkstats, value, sizeof(struct disk_stats));

@@ -383,21 +389,27 @@ static inline void free_part_stats(struct hd_struct *part)
 
 static inline void part_inc_in_flight(int cpu, struct hd_struct *part, int rw)
 {
-	atomic_inc(&part->in_flight[rw]);
+	part_stat_inc(cpu, part, in_flight[rw]);
 	if (part->partno)
-		atomic_inc(&part_to_disk(part)->part0.in_flight[rw]);
+		part_stat_inc(cpu, &part_to_disk(part)->part0, in_flight[rw]);
 }
 
 static inline void part_dec_in_flight(int cpu, struct hd_struct *part, int rw)
 {
-	atomic_dec(&part->in_flight[rw]);
+	part_stat_dec(cpu, part, in_flight[rw]);
 	if (part->partno)
-		atomic_dec(&part_to_disk(part)->part0.in_flight[rw]);
+		part_stat_dec(cpu, &part_to_disk(part)->part0, in_flight[rw]);
 }
 
 static inline int part_in_flight(struct hd_struct *part)
 {
-	return atomic_read(&part->in_flight[0]) + atomic_read(&part->in_flight[1]);
+	int res = 0;
+	unsigned int cpu;
+	for_each_possible_cpu(cpu) {
+		res += per_cpu_ptr((part)->dkstats, cpu)->in_flight[0];
+		res += per_cpu_ptr((part)->dkstats, cpu)->in_flight[1];
+	}
+	return res;
 }
 
 static inline struct partition_meta_info *alloc_part_info(struct gendisk *disk)

Signature · 1 line

1.9.1

Tejun Heo

2015-07-16 14:40 UTC #6 lore raw

Re: [PATCH 4/4] block: account io: convert part->in_fligh[] into percpu variable

Hello,

On Thu, Jul 16, 2015 at 11:16:47AM +0800, Ming Lei wrote:
...

drivers/md/dm.c static void start_io_acct(struct dm_io *io) · line 656

Quoted 11 lines

drivers/md/dm.c +2 -2

@@ -651,9 +651,9 @@ static void start_io_acct(struct dm_io *io)
 
 	cpu = part_stat_lock();
 	part_round_stats(cpu, &dm_disk(md)->part0);
+	part_stat_set(cpu, &dm_disk(md)->part0, in_flight[rw],
+			atomic_inc_return(&md->pending[rw]));
 	part_stat_unlock();
-	atomic_set(&dm_disk(md)->part0.in_flight[rw],
-		atomic_inc_return(&md->pending[rw]));

Why is this correct? Isn't the code trying to transfer its stat to block stat verbatim? Why does part_stat_set() have @cpu param at all? Shouldn't it clear the whole thing and set one of the cpu counters to the target value?

drivers/md/dm.c static void end_io_acct(struct dm_io *io) · line 684

Quoted 8 lines

drivers/md/dm.c +3 -1

@@ -679,7 +679,9 @@ static void end_io_acct(struct dm_io *io)
 	 * a flush.
 	 */
 	pending = atomic_dec_return(&md->pending[rw]);
-	atomic_set(&dm_disk(md)->part0.in_flight[rw], pending);
+	cpu = part_stat_lock();
+	part_stat_set(cpu, &dm_disk(md)->part0, in_flight[rw], pending);
+	part_stat_unlock();

Ditto.

Thanks.

Signature · 1 line

tejun

Jens Axboe

maintainer

2015-07-16 14:48 UTC #7 lore raw

In reply to Ming Lei

Quoted 8 lines · Ming Lei

Hi,
This patches kills two kinds of atomic operations in block accounting I/O.
The 1st two patches convert atomic refcount of partition into percpu refcount.

Quoted 6 lines

The 2nd two patches converts partition->in_flight[] into percpu variable.
With this change, ~15% throughput improvement can be observed when running fio(randread) over null blk in a dual-socket environment.

I've played with this before, but always ran into the hurdle of making part_in_flight() too expensive ended up hurting results in the end. Making the inc/dec parts of accounting percpu is a no-brainer, unfortunately the summing then becomes pretty expensive. I'll run this through some testing and see what kind of results I get.

Signature · 1 line

Jens Axboe

Tejun Heo

2015-07-16 14:59 UTC #8 lore raw

Quoted 5 lines · Jens Axboe

I've played with this before, but always ran into the hurdle of making part_in_flight() too expensive ended up hurting results in the end. Making the inc/dec parts of accounting percpu is a no-brainer, unfortunately the summing then becomes pretty expensive. I'll run this through some testing and see what kind of results I get.

The only place which could be a problem is part_round_stats() and we can do that *way* lazier. I don't think we're using that internally. Why are we even invoking it from IO issue / completion path?

Thanks.

Signature · 1 line

tejun

Jens Axboe

maintainer

2015-07-16 15:02 UTC #9 lore raw

Quoted 10 lines · Tejun Heo

Quoted 5 lines · Jens Axboe
I've played with this before, but always ran into the hurdle of making part_in_flight() too expensive ended up hurting results in the end. Making the inc/dec parts of accounting percpu is a no-brainer, unfortunately the summing then becomes pretty expensive. I'll run this through some testing and see what kind of results I get.
The only place which could be a problem is part_round_stats() and we can do that *way* lazier. I don't think we're using that internally. Why are we even invoking it from IO issue / completion path?

Right, that's where it's called in the fast path. Right now, it's per-jiffy lazy. As to how/when it's called, I don't believe that has changed since those stats were introduced. If we can move that out of the fast path, then the percpu changes are straight forward.

Signature · 1 line

Jens Axboe

Ming Lei

author

2015-07-16 15:01 UTC #10 lore raw

In reply to Jens Axboe

Quoted 13 lines · Jens Axboe

Quoted 12 lines · Ming Lei
Hi,
This patches kills two kinds of atomic operations in block accounting I/O.
The 1st two patches convert atomic refcount of partition into percpu refcount.
The 2nd two patches converts partition->in_flight[] into percpu variable.

Quoted 7 lines

Quoted 3 lines
With this change, ~15% throughput improvement can be observed when running fio(randread) over null blk in a dual-socket environment.
I've played with this before, but always ran into the hurdle of making part_in_flight() too expensive ended up hurting results in the end. Making

Yes, it is a bit expensive, but it is only run at most one time per tick for one partition.

Quoted 3 lines

the inc/dec parts of accounting percpu is a no-brainer, unfortunately the summing then becomes pretty expensive. I'll run this through some testing and see what kind of results I get.

The first two patches should be fine, and it still can get ~8% improvement in my test.

Quoted 4 lines

--
Jens Axboe

Signature · 1 line

Ming Lei

Jens Axboe

maintainer

2015-07-16 15:09 UTC #11 lore raw

Quoted 19 lines · Ming Lei

Quoted 18 lines · Jens Axboe
Quoted 15 lines · Ming Lei
Hi,
This patches kills two kinds of atomic operations in block accounting I/O.
The 1st two patches convert atomic refcount of partition into percpu refcount.
The 2nd two patches converts partition->in_flight[] into percpu variable.
With this change, ~15% throughput improvement can be observed when running fio(randread) over null blk in a dual-socket environment.

Quoted 5 lines

Quoted 2 lines
I've played with this before, but always ran into the hurdle of making part_in_flight() too expensive ended up hurting results in the end. Making
Yes, it is a bit expensive, but it is only run at most one time per tick for one partition.

Yup, but that can still be 1000 per second. And up until last year, it was even worse: 7276d02e241dc. So it's not too surprising if there's more low hanging fruit :-)

If we can make the rounding more lazy, then we should go ahead and do that.

Quoted 6 lines

Quoted 3 lines
the inc/dec parts of accounting percpu is a no-brainer, unfortunately the summing then becomes pretty expensive. I'll run this through some testing and see what kind of results I get.
The first two patches should be fine, and it still can get ~8% improvement in my test.

Agree, those can go right in.

Signature · 1 line

Jens Axboe