On Thu, Jun 29, 2017 at 12:31:05PM -0500, Brian King wrote:
> On 06/29/2017 11:25 AM, Ming Lei wrote:
> > On Thu, Jun 29, 2017 at 11:58 PM, Jens Axboe <ax...@kernel.dk> wrote:
> >> On 06/29/2017 02:40 AM, Ming Lei wrote:
> >>> On Thu, Jun 29, 2017 at 5:49 AM, Jens Axboe <ax...@kernel.dk> wrote:
> >>>> On 06/28/2017 03:12 PM, Brian King wrote:
> >>>>> This patch converts the in_flight counter in struct hd_struct from a
> >>>>> pair of atomics to a pair of percpu counters. This eliminates a couple
> >>>>> of atomics from the hot path. When running this on a Power system, to
> >>>>> a single null_blk device with 80 submission queues, irq mode 0, with
> >>>>> 80 fio jobs, I saw IOPs go from 1.5M IO/s to 11.4 IO/s.
> >>>>
> >>>> This has been done before, but I've never really liked it. The reason is
> >>>> that it means that reading the part stat inflight count now has to
> >>>> iterate over every possible CPU. Did you use partitions in your testing?
> >>>> How many CPUs were configured? When I last tested this a few years ago
> >>>> on even a quad core nehalem (which is notoriously shitty for cross-node
> >>>> latencies), it was a net loss.
> >>>
> >>> One year ago, I saw null_blk's IOPS can be decreased to 10%
> >>> of non-RQF_IO_STAT on a dual socket ARM64(each CPU has
> >>> 96 cores, and dual numa nodes) too, the performance can be
> >>> recovered basically if per numa-node counter is introduced and
> >>> used in this case, but the patch was never posted out.
> >>> If anyone is interested in that, I can rebase the patch on current
> >>> block tree and post out. I guess the performance issue might be
> >>> related with system cache coherency implementation more or less.
> >>> This issue on ARM64 can be observed with the following userspace
> >>> atomic counting test too:
> >>>
> >>> http://kernel.ubuntu.com/~ming/test/cache/
> >>
> >> How well did the per-node thing work? Doesn't seem to me like it would
> >
> > Last time, on ARM64, I remembered that the IOPS was basically recovered,
> > but now I don't have a such machine to test. Could Brian test the attached
> > patch
> > to see if it works on big Power machine?
> >
> > And the idea is simple, just make the atomic counter per-node.
>
> I tried loading the patch and get an oops right away on boot. Haven't been
> able to
> debug anything yet. This is on top of an older kernel, so not sure if that is
> the issue or not. I can try upstream and see if i have different results...
>
> [-1;-1fUbuntu 16.04[-1;-1f. . . .Unable to handle kernel paging request
> for data at address 0xc00031313a333532
> Faulting instruction address: 0xc0000000002552c4
> Oops: Kernel access of bad area, sig: 11 [#1]
> SMP NR_CPUS=1024 NUMA
> PowerNV
> Modules linked in: dm_round_robin vmx_crypto powernv_rng leds_powernv
> powernv_op_panel led_class rng_core dm_multipath autofs4 raid10 raid456
> async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq
> multipath dm_mirror dm_region_hash dm_log cxlflash bnx2x mdio libcrc32c nvme
> nvme_core lpfc cxl crc_t10dif crct10dif_generic crct10dif_common
> CPU: 9 PID: 3485 Comm: multipathd Not tainted 4.9.10-dirty #7
> task: c000000fba0d0000 task.stack: c000000fb8a1c000
> NIP: c0000000002552c4 LR: c000000000255274 CTR: 0000000000000000
> REGS: c000000fb8a1f350 TRAP: 0300 Not tainted (4.9.10-dirty)
> MSR: 900000000280b033 <SF,HV,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24028848
> XER: 00000000
> CFAR: c000000000008a60 DAR: c00031313a333532 DSISR: 40000000 SOFTE: 1
> GPR00: c0000000001f8980 c000000fb8a1f5d0 c000000000f24300 c000000fc4007c00
> GPR04: 00000000024000c0 c0000000002fc0ac 000000000000025d c000000fc5d346e0
> GPR08: 0000000fc50d6000 0000000000000000 c00031313a333532 c000000fbc836240
> GPR12: 0000000000002200 c00000000ff02400 00003fff79cfebf0 0000000000000000
> GPR16: 00000000c138fd03 000001003a12bf70 00003fff79ae75e0 00003fff79d269e8
> GPR20: 0000000000000001 00003fff79cfebf0 000001003a393aa0 00003fff79d067b8
> GPR24: 0000000000000000 c000000000b04948 000000000000a1ff c0000000002fc0ac
> GPR28: 00000000024000c0 0000000000000007 c0000000002fc0ac c000000fc4007c00
> NIP [c0000000002552c4] __kmalloc_track_caller+0x94/0x2f0
> LR [c000000000255274] __kmalloc_track_caller+0x44/0x2f0
> Call Trace:
> [c000000fb8a1f5d0] [c000000fb8a1f610] 0xc000000fb8a1f610 (unreliable)
> [c000000fb8a1f620] [c0000000001f8980] kstrdup+0x50/0xb0
> [c000000fb8a1f660] [c0000000002fc0ac] __kernfs_new_node+0x4c/0x140
> [c000000fb8a1f6b0] [c0000000002fd9f4] kernfs_new_node+0x34/0x80
> [c000000fb8a1f6e0] [c000000000300708] kernfs_create_link+0x38/0xf0
> [c000000fb8a1f720] [c000000000301cb8]
> sysfs_do_create_link_sd.isra.0+0xa8/0x160
> [c000000fb8a1f770] [c00000000054a658] device_add+0x2b8/0x740
> [c000000fb8a1f830] [c00000000054ae58] device_create_groups_vargs+0x178/0x190
> [c000000fb8a1f890] [c0000000001fcd70] bdi_register+0x80/0x1d0
> [c000000fb8a1f8c0] [c0000000001fd244] bdi_register_owner+0x44/0x80
> [c000000fb8a1f940] [c000000000453bbc] device_add_disk+0x1cc/0x500
> [c000000fb8a1f9f0] [c000000000764dec] dm_create+0x33c/0x5f0
> [c000000fb8a1fa90] [c00000000076d9ac] dev_create+0x8c/0x3e0
> [c000000fb8a1fb40] [c00000000076d1fc] ctl_ioctl+0x38c/0x580
> [c000000fb8a1fd20] [c00000000076d410] dm_ctl_ioctl+0x20/0x30
> [c000000fb8a1fd40] [c0000000002799ac] do_vfs_ioctl+0xcc/0x8f0
> [c000000fb8a1fde0] [c00000000027a230] SyS_ioctl+0x60/0xc0
> [c000000fb8a1fe30] [c00000000000bfe0] system_call+0x38/0xfc
> Instruction dump:
> 39290008 7cc8482a e94d0030 e9230000 7ce95214 7d49502a e9270010 2faa0000
> 419e007c 2fa90000 419e0074 e93f0022 <7f4a482a> 39200000 88ad023a 992d023a
> ---[ end trace dcdac2d3f668d033 ]---
>
Thanks for the test!
Looks there is one double free in patch, could you test the new one?
---
diff --git a/block/partition-generic.c b/block/partition-generic.c
index c5ec8246e25e..bd6644bf9643 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -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",
+ pnode_counter_read_all(&p->in_flight[0]),
+ pnode_counter_read_all(&p->in_flight[1]));
}
#ifdef CONFIG_FAIL_MAKE_REQUEST
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 96bd13e581cd..aac0b2235410 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -521,7 +521,7 @@ static void start_io_acct(struct dm_io *io)
cpu = part_stat_lock();
part_round_stats(cpu, &dm_disk(md)->part0);
part_stat_unlock();
- atomic_set(&dm_disk(md)->part0.in_flight[rw],
+ pnode_counter_set(&dm_disk(md)->part0.in_flight[rw],
atomic_inc_return(&md->pending[rw]));
if (unlikely(dm_stats_used(&md->stats)))
@@ -550,7 +550,7 @@ 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);
+ pnode_counter_set(&dm_disk(md)->part0.in_flight[rw], pending);
pending += atomic_read(&md->pending[rw^0x1]);
/* nudge anyone waiting on suspend queue */
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index e619fae2f037..40c9bc74a120 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -15,6 +15,7 @@
#include <linux/slab.h>
#include <linux/percpu-refcount.h>
#include <linux/uuid.h>
+#include <linux/pernode_counter.h>
#ifdef CONFIG_BLOCK
@@ -120,7 +121,7 @@ struct hd_struct {
int make_it_fail;
#endif
unsigned long stamp;
- atomic_t in_flight[2];
+ struct pnode_counter in_flight[2];
#ifdef CONFIG_SMP
struct disk_stats __percpu *dkstats;
#else
@@ -364,21 +365,22 @@ static inline void free_part_stats(struct hd_struct *part)
static inline void part_inc_in_flight(struct hd_struct *part, int rw)
{
- atomic_inc(&part->in_flight[rw]);
+ pnode_counter_inc(&part->in_flight[rw]);
if (part->partno)
- atomic_inc(&part_to_disk(part)->part0.in_flight[rw]);
+ pnode_counter_inc(&part_to_disk(part)->part0.in_flight[rw]);
}
static inline void part_dec_in_flight(struct hd_struct *part, int rw)
{
- atomic_dec(&part->in_flight[rw]);
+ pnode_counter_dec(&part->in_flight[rw]);
if (part->partno)
- atomic_dec(&part_to_disk(part)->part0.in_flight[rw]);
+ pnode_counter_dec(&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]);
+ return pnode_counter_read_all(&part->in_flight[0]) + \
+ pnode_counter_read_all(&part->in_flight[1]);
}
static inline struct partition_meta_info *alloc_part_info(struct gendisk *disk)
@@ -627,11 +629,34 @@ extern ssize_t part_fail_store(struct device *dev,
const char *buf, size_t count);
#endif /* CONFIG_FAIL_MAKE_REQUEST */
+static inline int hd_counter_init(struct hd_struct *part)
+{
+ if (pnode_counter_init(&part->in_flight[0], GFP_KERNEL))
+ return -ENOMEM;
+ if (pnode_counter_init(&part->in_flight[1], GFP_KERNEL)) {
+ pnode_counter_deinit(&part->in_flight[0]);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static inline void hd_counter_deinit(struct hd_struct *part)
+{
+ pnode_counter_deinit(&part->in_flight[0]);
+ pnode_counter_deinit(&part->in_flight[1]);
+}
+
static inline int hd_ref_init(struct hd_struct *part)
{
+ if (hd_counter_init(part))
+ return -ENOMEM;
+
if (percpu_ref_init(&part->ref, __delete_partition, 0,
- GFP_KERNEL))
+ GFP_KERNEL)) {
+ hd_counter_deinit(part);
return -ENOMEM;
+ }
return 0;
}
@@ -659,6 +684,7 @@ static inline void hd_free_part(struct hd_struct *part)
{
free_part_stats(part);
free_part_info(part);
+ hd_counter_deinit(part);
percpu_ref_exit(&part->ref);
}
diff --git a/include/linux/pernode_counter.h b/include/linux/pernode_counter.h
new file mode 100644
index 000000000000..e95b5f2d1d9c
--- /dev/null
+++ b/include/linux/pernode_counter.h
@@ -0,0 +1,117 @@
+#ifndef _LINUX_PERNODE_COUNTER_H
+#define _LINUX_PERNODE_COUNTER_H
+/*
+ * A simple per node atomic counter for use in block io accounting.
+ */
+
+#include <linux/smp.h>
+#include <linux/percpu.h>
+#include <linux/types.h>
+#include <linux/gfp.h>
+
+struct node_counter {
+ atomic_t counter_in_node;
+};
+
+struct pnode_counter {
+ struct node_counter * __percpu *counter;
+ struct node_counter **nodes;
+};
+
+static inline int pnode_counter_init(struct pnode_counter *pnc, gfp_t gfp)
+{
+ struct node_counter **nodes;
+ int i, j, cpu;
+
+ nodes = kzalloc(nr_node_ids * sizeof(struct node_counter *), gfp);
+ if (!nodes)
+ goto err_nodes;
+
+ for_each_node(i) {
+ nodes[i] = kzalloc_node(sizeof(struct node_counter), gfp, i);
+ if (!nodes[i])
+ goto err_node_counter;
+ }
+
+ pnc->counter = alloc_percpu_gfp(struct node_counter *, gfp);
+ if (!pnc->counter)
+ goto err_node_counter;
+
+ for_each_possible_cpu(cpu)
+ *per_cpu_ptr(pnc->counter, cpu) = nodes[cpu_to_node(cpu)];
+
+ pnc->nodes = nodes;
+
+ return 0;
+
+ err_node_counter:
+ for (j = 0; j < i; j++)
+ kfree(nodes[j]);
+ kfree(nodes);
+ err_nodes:
+ return -ENOMEM;
+}
+
+static inline void pnode_counter_deinit(struct pnode_counter *pnc)
+{
+ int i;
+
+ for_each_node(i) {
+ struct node_counter *node = pnc->nodes[i];
+
+ kfree(node);
+ }
+ free_percpu(pnc->counter);
+ kfree(pnc->nodes);
+}
+
+static inline void pnode_counter_inc(struct pnode_counter *pnc)
+{
+ struct node_counter *node = this_cpu_read(*pnc->counter);
+
+ atomic_inc(&node->counter_in_node);
+}
+
+static inline void pnode_counter_inc_cpu(struct pnode_counter *pnc, int cpu)
+{
+ struct node_counter *node = *per_cpu_ptr(pnc->counter, cpu);
+
+ atomic_inc(&node->counter_in_node);
+}
+
+static inline void pnode_counter_dec(struct pnode_counter *pnc)
+{
+ struct node_counter *node = this_cpu_read(*pnc->counter);
+
+ atomic_dec(&node->counter_in_node);
+}
+
+static inline void pnode_counter_dec_cpu(struct pnode_counter *pnc, int cpu)
+{
+ struct node_counter *node = *per_cpu_ptr(pnc->counter, cpu);
+
+ atomic_dec(&node->counter_in_node);
+}
+
+static inline void pnode_counter_set(struct pnode_counter *pnc, int val)
+{
+ int i;
+ struct node_counter *node = this_cpu_read(*pnc->counter);
+
+ for_each_node(i)
+ atomic_set(&pnc->nodes[i]->counter_in_node, 0);
+ atomic_set(&node->counter_in_node, val);
+}
+
+static inline long pnode_counter_read_all(struct pnode_counter *pnc)
+{
+ int i;
+ long val = 0;
+
+ for_each_node(i)
+ val += atomic_read(&pnc->nodes[i]->counter_in_node);
+
+ return val;
+}
+
+#endif /* _LINUX_PERNODE_COUNTER_H */
Thanks,
Ming
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
