[PATCH rfc 0/5] generic adaptive IRQ moderation library for I/O devices

[PATCH rfc 0/5] generic adaptive IRQ moderation library for I/O devices

[Sagi Grimberg]

Adaptive IRQ moderation (also called adaptive IRQ coalescing) has been widely used
in the networking stack for over 20 years and has become a standard default
setting.

Adaptive moderation is a feature supported by the device to delay an interrupt
for a either a period of time, or number of completions (packets for networking
devices) in order to optimize the effective throughput by mitigating the cost of
handling interrupts which can be expensive in modern rates in networking and/or
storage devices.

The basic concept of adaptive moderation is to provide time and packet based control
of when the device generates an interrupt in an adaptive fashion, attempting to
identify the current workload based on online gathered statistics instead of a
predefined configuration. When done correctly, adaptive moderation can win the
best of both throughput

This rfc patchset introduces a generic library that provides the mechanics for
online statistics monitoring and decision making for the consumer which simply
need to program the actual device while still keeping room for device specific
tuning.

In the networking stack, each device driver implements adaptive IRQ moderation
on its own. The approach here is a bit different, it tries to take the common denominator,
which is per-queue statistics gathering and workload change identification
(basically decides if the moderation scheme needs to change).

The library is targeted to multi-queue devices, but should work on single queue
devices as well, however I'm not sure that these devices will need something
like interrupt moderation.

The model used in the proposed implementation requires the consumer (a.k.a the
device driver) to initialize an irq adaptive moderator context (a.k.a irq_am) per
its desired context which will most likely be a completion queue.

The moderator is initialized with a set of am (adaptive moderation) levels,
which are essentially the abstraction of the device specific moderation parameters.
Usually the different levels will map to different pairs of
(time <usecs>, completion-count) which are left specific to the consumer.

The moderator assumes that the am levels are sorted in an increasing order when the
lowest level corresponds to the optimum latency tuning (short time and low
completion-count) and gradually increasing towards the throughput optimum tuning
(longer time and higher completion-count). So there is a trend and tuning direction
tracked by the moderator. When the moderator collects sufficient statistics (also
controlled by the consumer defining nr_events), it compares the current stats with the
previous stats and if a significant changed was observed in the load, the moderator
attempts to increment/decrement its current level (called a step) and schedules a program
dispatch work.

The main reason why this implementation is different then the common networking devices
implementation (and kept separate) is that in my mind at least, network devices are different
animals than other I/O devices in the sense that:
(a) network devices rely heavily on byte count of raw ethernet frames for adaptive moderation
    while in storage or I/O, the byte count is often a result of a submission/completion transaction
    and sometimes even known only to the application on top of the infrastructure (like in the
    rdma case).
(b) Performance characteristics and expectations in representative workloads.
(c) network devices collect all sort of stats for different functionalities (where adaptive moderation
    is only one use-case) and I'm not sure at all that a subset of stats could easily migrate to a different
    context.

Having said that, if sufficient reasoning comes along, I could be convinced otherwise if unification
of the implementations between networking and I/O is desired.

Additionally, I hooked two consumers into this framework:
1. irq-poll - interrupt polling library (used by various rdma consumers and can be extended to others)
2. rdma cq - generic rdma cq polling abstraction library

With this, both RDMA initiator mode consumers and RDMA target mode consumers
are able to utilize the framework (nvme, iser, srp, nfs). Moreover, I currently do
not see any reason why other HBAs (or devices in general) that support interrupt
moderation wouldn't be able to hook into this framework as well.

Note that the code is in *RFC* level, attempting to convey the concept.
If the direction is acceptable, the setup can be easily modified and
cleanups can be performed.

Initial benchmarking shows promising results with 50% improvement in throughput
when testing high load of small I/O. The experiment taken used nvme-rdma host vs. nvmet-rdma
target exposing a null_blk device. The workload ran multithreaded fio run with high queue-depth
and block size of 512B read I/O (4K block size would exceed 100 Gbe wire speed).

The results without adaptive moderation reaches ~8M IOPs bottlenecking the host and target cpu.
With adaptive moderation enabled, IOPs quickly converge to ~12M IOPs (at the expense of slightly
higher latencies obviously) and debugfs stats show that the moderation level reached the
throughput optimum level. There is currently a known issue I've observed in some conditions
converging back to latency optimum (after reaching throughput optimum am levels) and I'll work
to fix the tuning algorithm. Thanks to Idan Burstein for running some benchmarks on his
performance setup.

I've also tested this locally with my single core VMs and saw similar improvement of ~50%
in throughput in a similar workload (355 KIOPs vs. 235 KIOPs). More testing will help
a lot to confirm and improve the implementation.

QD=1 Latency tests showed a marginal regression of up to 2% in latency (lightly tested though).
The reason at this point is that the moderator still bounces in the low latency am levels
constantly (would like to improve that).

Another observed issue is the presence of user context polling (IOCB_HIPRI) which does not update
the irq_am stats (mainly because its not interrupt driven). This can cause the moderator to do
the wrong thing as its based on partial view of the load (optimize for latency instead of getting
out of the poller's way). However, recent discussions raised the possibility that polling requests
will be executed on a different set of queues with interrupts disabled altogether, which would make
this a non-issue.

None the less, I would like to get some initial feedback on the approach. Also, I'm not an expert
in tuning the algorithm. The basic approach was inspired by the mlx5 driver implementation which
seemed the closest to fit the abstraction level that I was aiming for. So I'd also love to get some
ideas on how to tune the algorithm better for various workloads (hence the RFC).

Lastly, I have also attempted to hook this into nvme (pcie), but that wasn't successful mainly
because the coalescing set_feature is global to the controller and not per-queue.
I'll be looking to bringing per-queue coalescing to the NVMe TWG (in case the community is
interested in supporting this).

Feedback would be highly appreciated, as well as a test drive with the code in case anyone
is interested :)

Sagi Grimberg (5):
  irq-am: Introduce helper library for adaptive moderation
    implementation
  irq-am: add some debugfs exposure on tuning state
  irq_poll: wire up irq_am and allow to initialize it
  IB/cq: add adaptive moderation support
  IB/cq: wire up adaptive moderation to workqueue based completion
    queues

 drivers/infiniband/core/cq.c |  73 ++++++++++-
 include/linux/irq-am.h       | 118 ++++++++++++++++++
 include/linux/irq_poll.h     |   9 ++
 include/rdma/ib_verbs.h      |   9 +-
 lib/Kconfig                  |   6 +
 lib/Makefile                 |   1 +
 lib/irq-am.c                 | 291 +++++++++++++++++++++++++++++++++++++++++++
 lib/irq_poll.c               |  30 ++++-
 8 files changed, 529 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/irq-am.h
 create mode 100644 lib/irq-am.c

-- 
2.14.1

[PATCH rfc 1/5] irq-am: Introduce library implementing generic adaptive moderation

[Sagi Grimberg]

irq-am library helps I/O devices implement interrupt moderation in
an adaptive fashion, based on online stats.

The consumer can initialize an irq-am context with a callback that
performs the device specific moderation programming and also the number
of am (adaptive moderation) levels which are also, abstracted and allows
for device specific tuning.

The irq-am code will sample once every nr_events and will check for significant
change in workload characteristics (completions per second, events per second)
and if it detects one, will perform an am level update(called a step).

The irq-am code  assumes that the am levels are sorted in an increasing order when
the lowest level corresponds to the optimum latency tuning (short time and low
completion-count) and gradually increasing towards the throughput optimum tuning
(longer time and higher completion-count). So there is a trend and tuning direction
tracked by the moderator. When the moderator collects sufficient statistics (also
controlled by the consumer defining nr_events), it compares the current stats with the
previous stats and if a significant changed was observed in the load, the moderator
attempts to increment/decrement its current level (step) and schedules a program
dispatch work.

Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 include/linux/irq-am.h | 116 +++++++++++++++++++++++++++++++
 lib/Kconfig            |   5 ++
 lib/Makefile           |   1 +
 lib/irq-am.c           | 182 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 304 insertions(+)
 create mode 100644 include/linux/irq-am.h
 create mode 100644 lib/irq-am.c

diff --git a/include/linux/irq-am.h b/include/linux/irq-am.h
new file mode 100644
index 000000000000..5ddd5ca268aa
--- /dev/null
+++ b/include/linux/irq-am.h
@@ -0,0 +1,116 @@
+/*
+ * Adaptive moderation support for I/O devices.
+ * Copyright (c) 2018 Lightbits Labs.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+#ifndef _IRQ_AM_H
+#define _IRQ_AM_H
+
+#include <linux/ktime.h>
+#include <linux/workqueue.h>
+
+struct irq_am;
+typedef int (irq_am_fn)(struct irq_am *, unsigned short level);
+
+/*
+ * struct irq_am_sample_stats - sample stats for adpative moderation
+ * @cps:        completions per-second
+ * @eps:        events per-second
+ * @cpe:	completions per event
+ */
+struct irq_am_sample_stats {
+	u32 cps;
+	u32 eps;
+	u32 cpe;
+};
+
+/*
+ * struct irq_am_sample - per-irq interrupt batch sample unit
+ * @time:         current time
+ * @comps:     completions count since last sample
+ * @events:    events count since the last sample
+ */
+struct irq_am_sample {
+	ktime_t	time;
+	u64	comps;
+	u64	events;
+};
+
+/*
+ * enum irq_am_state - adaptive moderation monitor states
+ * @IRQ_AM_START_MEASURING:        collect first sample (start_sample)
+ * @IRQ_AM_MEASURING:              measurement in progress
+ * @IRQ_AM_PROGRAM_MODERATION:     moderatio program scheduled
+ *                                 so we should not react to any stats
+ *                                 from the old moderation profile.
+ */
+enum irq_am_state {
+	IRQ_AM_START_MEASURING,
+	IRQ_AM_MEASURING,
+	IRQ_AM_PROGRAM_MODERATION,
+};
+
+enum irq_am_tune_state {
+	IRQ_AM_GOING_UP,
+	IRQ_AM_GOING_DOWN,
+};
+
+enum irq_am_relative_diff {
+	IRQ_AM_STATS_WORSE,
+	IRQ_AM_STATS_SAME,
+	IRQ_AM_STATS_BETTER,
+};
+
+struct irq_am_stats {
+	u64	events;
+	u64	comps;
+};
+
+/*
+ * struct irq_am - irq adaptive moderation monitor
+ * @state:             adaptive moderation monitor state
+ * @tune_state:        tuning state of the moderation monitor
+ * @am_stats:          overall completions and events counters
+ * @start_sample:      first sample in moderation batch
+ * @prev_stats:        previous stats for trend detection
+ * @nr_events:         number of events between samples
+ * @nr_levels:         number of moderation levels
+ * @curr_level:        current moderation level
+ * @work:              schedule moderation program
+ * @program:           moderation program handler
+ */
+struct irq_am {
+	enum irq_am_state		state;
+	enum irq_am_tune_state		tune_state;
+
+	struct irq_am_stats		am_stats;
+	struct irq_am_sample		start_sample;
+	struct irq_am_sample_stats	prev_stats;
+
+	u16				nr_events;
+	unsigned short			nr_levels;
+	unsigned short			curr_level;
+
+	struct work_struct		work;
+	irq_am_fn			*program;
+};
+
+void irq_am_add_event(struct irq_am *am);
+static inline void irq_am_add_comps(struct irq_am *am, u64 n)
+{
+	am->am_stats.comps += n;
+}
+
+void irq_am_cleanup(struct irq_am *am);
+void irq_am_init(struct irq_am *am, unsigned int nr_events,
+	unsigned short nr_levels, unsigned short start_level, irq_am_fn *fn);
+
+#endif
diff --git a/lib/Kconfig b/lib/Kconfig
index 4dd5c11366f9..bbb4c9eea84d 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -504,6 +504,11 @@ config DDR
 	  information. This data is useful for drivers handling
 	  DDR SDRAM controllers.
 
+config IRQ_AM
+	bool "IRQ adaptive moderation library"
+	help
+	  Helper library to implement adaptive moderation for I/O devices.
+
 config IRQ_POLL
 	bool "IRQ polling library"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index d11c48ec8ffd..795583a685b9 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -193,6 +193,7 @@ obj-$(CONFIG_SG_SPLIT) += sg_split.o
 obj-$(CONFIG_SG_POOL) += sg_pool.o
 obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
 obj-$(CONFIG_IRQ_POLL) += irq_poll.o
+obj-$(CONFIG_IRQ_AM) += irq-am.o
 
 obj-$(CONFIG_STACKDEPOT) += stackdepot.o
 KASAN_SANITIZE_stackdepot.o := n
diff --git a/lib/irq-am.c b/lib/irq-am.c
new file mode 100644
index 000000000000..ed7befd7a560
--- /dev/null
+++ b/lib/irq-am.c
@@ -0,0 +1,182 @@
+/*
+ * Adaptive moderation support for I/O devices.
+ * Copyright (c) 2018 Lightbits Labs.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+#include <linux/irq-am.h>
+
+static void irq_am_try_step(struct irq_am *am)
+{
+	if (am->tune_state == IRQ_AM_GOING_UP &&
+	    am->curr_level != am->nr_levels - 1) {
+		am->curr_level++;
+	} else if (am->tune_state == IRQ_AM_GOING_DOWN &&
+		   am->curr_level != 0) {
+		am->curr_level--;
+	}
+}
+
+static inline bool irq_am_on_edge(struct irq_am *am)
+{
+	return am->curr_level == 0 || am->curr_level == am->nr_levels - 1;
+}
+
+static void irq_am_turn(struct irq_am *am)
+{
+	am->tune_state = am->tune_state == IRQ_AM_GOING_UP ?
+		IRQ_AM_GOING_DOWN : IRQ_AM_GOING_UP;
+	irq_am_try_step(am);
+}
+
+#define IRQ_AM_SIGNIFICANT_DIFF(val, ref) \
+	(((100 * abs((val) - (ref))) / (ref)) > 20) /* more than 20% difference */
+
+static int irq_am_stats_compare(struct irq_am *am, struct irq_am_sample_stats *curr)
+{
+	struct irq_am_sample_stats *prev = &am->prev_stats;
+
+	/* first stat */
+	if (!prev->cps)
+		return IRQ_AM_STATS_SAME;
+
+	/* more completions per second is better */
+	if (IRQ_AM_SIGNIFICANT_DIFF(curr->cps, prev->cps))
+		return (curr->cps > prev->cps) ? IRQ_AM_STATS_BETTER :
+						 IRQ_AM_STATS_WORSE;
+
+	/* less events per second is better */
+	if (IRQ_AM_SIGNIFICANT_DIFF(curr->eps, prev->eps))
+		return (curr->eps < prev->eps) ? IRQ_AM_STATS_BETTER :
+						 IRQ_AM_STATS_WORSE;
+
+	/*
+	 * we get 1 completion per event, no point in trying to aggregate
+	 * any further, start declining moderation
+	 */
+	if (curr->cpe == 1 && am->curr_level)
+		return am->tune_state == IRQ_AM_GOING_UP ?
+			IRQ_AM_STATS_WORSE : IRQ_AM_STATS_BETTER;
+
+	return IRQ_AM_STATS_SAME;
+}
+
+static bool irq_am_decision(struct irq_am *am,
+		struct irq_am_sample_stats *curr_stats)
+{
+	unsigned short prev_level = am->curr_level;
+	enum irq_am_relative_diff diff;
+	bool changed;
+
+	diff = irq_am_stats_compare(am, curr_stats);
+	switch (diff) {
+	default:
+	case IRQ_AM_STATS_SAME:
+		/* fall through */
+		break;
+	case IRQ_AM_STATS_WORSE:
+		irq_am_turn(am);
+		break;
+	case IRQ_AM_STATS_BETTER:
+		irq_am_try_step(am);
+		break;
+	}
+
+	changed = am->curr_level != prev_level || irq_am_on_edge(am);
+	if (changed || !am->prev_stats.cps)
+		am->prev_stats = *curr_stats;
+
+	return changed;
+}
+
+static void irq_am_sample(struct irq_am *am, struct irq_am_sample *s)
+{
+	s->time = ktime_get();
+	s->events = am->am_stats.events;
+	s->comps = am->am_stats.comps;
+}
+
+static void irq_am_calc_stats(struct irq_am *am, struct irq_am_sample *start,
+		struct irq_am_sample *end,
+		struct irq_am_sample_stats *curr_stats)
+{
+	/* u32 holds up to 71 minutes, should be enough */
+	u32 delta_us = ktime_us_delta(end->time, start->time);
+	u32 ncomps = end->comps - start->comps;
+
+	if (!delta_us)
+		return;
+
+	curr_stats->cps = DIV_ROUND_UP(ncomps * USEC_PER_SEC, delta_us);
+	curr_stats->eps = DIV_ROUND_UP(am->nr_events * USEC_PER_SEC, delta_us);
+	curr_stats->cpe = DIV_ROUND_UP(ncomps, am->nr_events);
+}
+
+void irq_am_add_event(struct irq_am *am)
+{
+	struct irq_am_sample end_sample;
+	struct irq_am_sample_stats curr_stats;
+	u16 nr_events;
+
+	am->am_stats.events++;
+
+	switch (am->state) {
+	case IRQ_AM_MEASURING:
+		nr_events = am->am_stats.events - am->start_sample.events;
+		if (nr_events < am->nr_events)
+			break;
+
+		irq_am_sample(am, &end_sample);
+		irq_am_calc_stats(am, &am->start_sample, &end_sample,
+				    &curr_stats);
+		if (irq_am_decision(am, &curr_stats)) {
+			am->state = IRQ_AM_PROGRAM_MODERATION;
+			schedule_work(&am->work);
+			break;
+		}
+		/* fall through */
+	case IRQ_AM_START_MEASURING:
+		irq_am_sample(am, &am->start_sample);
+		am->state = IRQ_AM_MEASURING;
+		break;
+	case IRQ_AM_PROGRAM_MODERATION:
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(irq_am_add_event);
+
+static void irq_am_program_moderation_work(struct work_struct *w)
+{
+	struct irq_am *am = container_of(w, struct irq_am, work);
+
+	WARN_ON_ONCE(am->program(am, am->curr_level));
+	am->state = IRQ_AM_START_MEASURING;
+}
+
+
+void irq_am_cleanup(struct irq_am *am)
+{
+	flush_work(&am->work);
+}
+EXPORT_SYMBOL_GPL(irq_am_cleanup);
+
+void irq_am_init(struct irq_am *am, unsigned int nr_events,
+	unsigned short nr_levels, unsigned short start_level, irq_am_fn *fn)
+{
+	memset(am, 0, sizeof(*am));
+	am->state = IRQ_AM_START_MEASURING;
+	am->tune_state = IRQ_AM_GOING_UP;
+	am->nr_levels = nr_levels;
+	am->nr_events = nr_events;
+	am->curr_level = start_level;
+	am->program = fn;
+	INIT_WORK(&am->work, irq_am_program_moderation_work);
+}
+EXPORT_SYMBOL_GPL(irq_am_init);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rfc 1/5] irq-am: Introduce library implementing generic adaptive moderation

[Or Gerlitz]

On Tue, Feb 6, 2018 at 12:03 AM, Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org> wrote:
> irq-am library helps I/O devices implement interrupt moderation in
> an adaptive fashion, based on online stats.
>
> The consumer can initialize an irq-am context with a callback that
> performs the device specific moderation programming and also the number
> of am (adaptive moderation) levels which are also, abstracted and allows
> for device specific tuning.
>
> The irq-am code will sample once every nr_events and will check for significant
> change in workload characteristics (completions per second, events per second)
> and if it detects one, will perform an am level update(called a step).
>
> The irq-am code  assumes that the am levels are sorted in an increasing order when
> the lowest level corresponds to the optimum latency tuning (short time and low
> completion-count) and gradually increasing towards the throughput optimum tuning
> (longer time and higher completion-count). So there is a trend and tuning direction
> tracked by the moderator. When the moderator collects sufficient statistics (also
> controlled by the consumer defining nr_events), it compares the current stats with the
> previous stats and if a significant changed was observed in the load, the moderator
> attempts to increment/decrement its current level (step) and schedules a program
> dispatch work.
>
> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> ---
>  include/linux/irq-am.h | 116 +++++++++++++++++++++++++++++++

Talking to Tal, it seems that this is what landed in upstream as
include/linux/net_dim.h
and can have few adjustments for you, I suggest you take a look
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rfc 2/5] irq-am: add some debugfs exposure on tuning state

[Sagi Grimberg]

Useful for local debugging

Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 include/linux/irq-am.h |   2 +
 lib/irq-am.c           | 109 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+)

diff --git a/include/linux/irq-am.h b/include/linux/irq-am.h
index 5ddd5ca268aa..18df315633a4 100644
--- a/include/linux/irq-am.h
+++ b/include/linux/irq-am.h
@@ -101,6 +101,8 @@ struct irq_am {
 
 	struct work_struct		work;
 	irq_am_fn			*program;
+	struct dentry			*debugfs_dir;
+	int				id;
 };
 
 void irq_am_add_event(struct irq_am *am);
diff --git a/lib/irq-am.c b/lib/irq-am.c
index ed7befd7a560..6cd2f131f5e8 100644
--- a/lib/irq-am.c
+++ b/lib/irq-am.c
@@ -12,6 +12,61 @@
  * more details.
  */
 #include <linux/irq-am.h>
+#include <linux/debugfs.h>
+
+static DEFINE_IDA(am_ida);
+
+#ifdef CONFIG_DEBUG_FS
+struct dentry *irq_am_debugfs_root;
+
+struct irq_am_debugfs_attr {
+	const char *name;
+	umode_t mode;
+	int (*show)(void *, struct seq_file *);
+	ssize_t (*write)(void *, const char __user *, size_t, loff_t *);
+};
+
+static int irq_am_tune_state_show(void *data, struct seq_file *m)
+{
+	struct irq_am *am = data;
+
+	seq_printf(m, "%d\n", am->tune_state);
+	return 0;
+}
+
+static int irq_am_curr_level_show(void *data, struct seq_file *m)
+{
+	struct irq_am *am = data;
+
+	seq_printf(m, "%d\n", am->curr_level);
+	return 0;
+}
+
+static int irq_am_debugfs_show(struct seq_file *m, void *v)
+{
+	const struct irq_am_debugfs_attr *attr = m->private;
+	void *data = d_inode(m->file->f_path.dentry->d_parent)->i_private;
+
+	return attr->show(data, m);
+}
+
+static int irq_am_debugfs_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, irq_am_debugfs_show, inode->i_private);
+}
+
+static const struct file_operations irq_am_debugfs_fops = {
+	.open		= irq_am_debugfs_open,
+	.read		= seq_read,
+	.release	= seq_release,
+};
+
+static const struct irq_am_debugfs_attr irq_am_attrs[] = {
+	{"tune_state", 0400, irq_am_tune_state_show},
+	{"curr_level", 0400, irq_am_curr_level_show},
+	{},
+};
+#endif
 
 static void irq_am_try_step(struct irq_am *am)
 {
@@ -160,10 +215,51 @@ static void irq_am_program_moderation_work(struct work_struct *w)
 	am->state = IRQ_AM_START_MEASURING;
 }
 
+#ifdef CONFIG_DEBUG_FS
+static bool debugfs_create_files(struct dentry *parent, void *data,
+				 const struct irq_am_debugfs_attr *attr)
+{
+	d_inode(parent)->i_private = data;
+
+	for (; attr->name; attr++) {
+		if (!debugfs_create_file(attr->name, attr->mode, parent,
+					 (void *)attr, &irq_am_debugfs_fops))
+			return false;
+	}
+	return true;
+}
+
+static int irq_am_register_debugfs(struct irq_am *am)
+{
+	char name[20];
+
+	snprintf(name, sizeof(name), "am%u", am->id);
+	am->debugfs_dir = debugfs_create_dir(name,
+				irq_am_debugfs_root);
+	if (!am->debugfs_dir)
+		return -ENOMEM;
+
+	if (!debugfs_create_files(am->debugfs_dir, am,
+				irq_am_attrs))
+		return -ENOMEM;
+	return 0;
+}
+
+static void irq_am_deregister_debugfs(struct irq_am *am)
+{
+	debugfs_remove_recursive(am->debugfs_dir);
+}
+
+#else
+static int irq_am_register_debugfs(struct irq_am *am) {}
+static void irq_am_deregister_debugfs(struct irq_am *am) {}
+#endif
 
 void irq_am_cleanup(struct irq_am *am)
 {
 	flush_work(&am->work);
+	irq_am_deregister_debugfs(am);
+	ida_simple_remove(&am_ida, am->id);
 }
 EXPORT_SYMBOL_GPL(irq_am_cleanup);
 
@@ -177,6 +273,19 @@ void irq_am_init(struct irq_am *am, unsigned int nr_events,
 	am->nr_events = nr_events;
 	am->curr_level = start_level;
 	am->program = fn;
+	am->id = ida_simple_get(&am_ida, 0, 0, GFP_KERNEL);
+	WARN_ON(am->id < 0);
 	INIT_WORK(&am->work, irq_am_program_moderation_work);
+	if (irq_am_register_debugfs(am))
+		pr_warn("irq-am %d failed to register debugfs\n", am->id);
 }
 EXPORT_SYMBOL_GPL(irq_am_init);
+
+static __init int irq_am_setup(void)
+{
+#ifdef CONFIG_DEBUG_FS
+	irq_am_debugfs_root = debugfs_create_dir("irq_am", NULL);
+#endif
+	return 0;
+}
+subsys_initcall(irq_am_setup);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rfc 2/5] irq-am: add some debugfs exposure on tuning state

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3261 bytes --]

Hi Sagi,

I love your patch! Yet something to improve:

[auto build test ERROR on v4.15]
[also build test ERROR on next-20180206]
[cannot apply to linus/master rdma/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sagi-Grimberg/irq-am-Introduce-library-implementing-generic-adaptive-moderation/20180206-224501
config: i386-randconfig-s0-201805 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   lib/irq-am.c:17:8: error: type defaults to 'int' in declaration of 'DEFINE_IDA' [-Werror=implicit-int]
    static DEFINE_IDA(am_ida);
           ^~~~~~~~~~
   lib/irq-am.c:17:1: warning: parameter names (without types) in function declaration
    static DEFINE_IDA(am_ida);
    ^~~~~~
   lib/irq-am.c: In function 'irq_am_cleanup':
>> lib/irq-am.c:262:2: error: implicit declaration of function 'ida_simple_remove' [-Werror=implicit-function-declaration]
     ida_simple_remove(&am_ida, am->id);
     ^~~~~~~~~~~~~~~~~
   lib/irq-am.c:262:21: error: 'am_ida' undeclared (first use in this function)
     ida_simple_remove(&am_ida, am->id);
                        ^~~~~~
   lib/irq-am.c:262:21: note: each undeclared identifier is reported only once for each function it appears in
   lib/irq-am.c: In function 'irq_am_init':
>> lib/irq-am.c:276:11: error: implicit declaration of function 'ida_simple_get' [-Werror=implicit-function-declaration]
     am->id = ida_simple_get(&am_ida, 0, 0, GFP_KERNEL);
              ^~~~~~~~~~~~~~
   lib/irq-am.c:276:27: error: 'am_ida' undeclared (first use in this function)
     am->id = ida_simple_get(&am_ida, 0, 0, GFP_KERNEL);
                              ^~~~~~
   lib/irq-am.c: At top level:
   lib/irq-am.c:17:8: warning: 'DEFINE_IDA' declared 'static' but never defined [-Wunused-function]
    static DEFINE_IDA(am_ida);
           ^~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/ida_simple_remove +262 lib/irq-am.c

   257	
   258	void irq_am_cleanup(struct irq_am *am)
   259	{
   260		flush_work(&am->work);
   261		irq_am_deregister_debugfs(am);
 > 262		ida_simple_remove(&am_ida, am->id);
   263	}
   264	EXPORT_SYMBOL_GPL(irq_am_cleanup);
   265	
   266	void irq_am_init(struct irq_am *am, unsigned int nr_events,
   267		unsigned short nr_levels, unsigned short start_level, irq_am_fn *fn)
   268	{
   269		memset(am, 0, sizeof(*am));
   270		am->state = IRQ_AM_START_MEASURING;
   271		am->tune_state = IRQ_AM_GOING_UP;
   272		am->nr_levels = nr_levels;
   273		am->nr_events = nr_events;
   274		am->curr_level = start_level;
   275		am->program = fn;
 > 276		am->id = ida_simple_get(&am_ida, 0, 0, GFP_KERNEL);
   277		WARN_ON(am->id < 0);
   278		INIT_WORK(&am->work, irq_am_program_moderation_work);
   279		if (irq_am_register_debugfs(am))
   280			pr_warn("irq-am %d failed to register debugfs\n", am->id);
   281	}
   282	EXPORT_SYMBOL_GPL(irq_am_init);
   283	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25162 bytes --]

Re: [PATCH rfc 2/5] irq-am: add some debugfs exposure on tuning state

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2580 bytes --]

Hi Sagi,

I love your patch! Yet something to improve:

[auto build test ERROR on v4.15]
[also build test ERROR on next-20180206]
[cannot apply to linus/master rdma/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sagi-Grimberg/irq-am-Introduce-library-implementing-generic-adaptive-moderation/20180206-224501
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All error/warnings (new ones prefixed by >>):

>> lib/irq-am.c:17:8: error: type defaults to 'int' in declaration of 'DEFINE_IDA' [-Werror=implicit-int]
    static DEFINE_IDA(am_ida);
           ^~~~~~~~~~
>> lib/irq-am.c:17:1: warning: parameter names (without types) in function declaration
    static DEFINE_IDA(am_ida);
    ^~~~~~
   lib/irq-am.c: In function 'irq_am_cleanup':
>> lib/irq-am.c:262:2: error: implicit declaration of function 'ida_simple_remove'; did you mean 'simple_rename'? [-Werror=implicit-function-declaration]
     ida_simple_remove(&am_ida, am->id);
     ^~~~~~~~~~~~~~~~~
     simple_rename
>> lib/irq-am.c:262:21: error: 'am_ida' undeclared (first use in this function)
     ida_simple_remove(&am_ida, am->id);
                        ^~~~~~
   lib/irq-am.c:262:21: note: each undeclared identifier is reported only once for each function it appears in
   lib/irq-am.c: In function 'irq_am_init':
>> lib/irq-am.c:276:11: error: implicit declaration of function 'ida_simple_get'; did you mean 'simple_open'? [-Werror=implicit-function-declaration]
     am->id = ida_simple_get(&am_ida, 0, 0, GFP_KERNEL);
              ^~~~~~~~~~~~~~
              simple_open
   lib/irq-am.c:276:27: error: 'am_ida' undeclared (first use in this function)
     am->id = ida_simple_get(&am_ida, 0, 0, GFP_KERNEL);
                              ^~~~~~
   lib/irq-am.c: At top level:
   lib/irq-am.c:17:8: warning: 'DEFINE_IDA' declared 'static' but never defined [-Wunused-function]
    static DEFINE_IDA(am_ida);
           ^~~~~~~~~~
   cc1: some warnings being treated as errors

vim +17 lib/irq-am.c

    16	
  > 17	static DEFINE_IDA(am_ida);
    18	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 47585 bytes --]

Re: [PATCH rfc 2/5] irq-am: add some debugfs exposure on tuning state

[Bart Van Assche]

On Tue, 2018-02-06 at 00:03 +0200, Sagi Grimberg wrote:
> +static int irq_am_register_debugfs(struct irq_am *am)
> +{
> +	char name[20];
> +
> +	snprintf(name, sizeof(name), "am%u", am->id);
> +	am->debugfs_dir = debugfs_create_dir(name,
> +				irq_am_debugfs_root);

How is a user expected to figure out which driver am%u is associated with?
Please make sure that these directories have a name that makes it easy for
users to figure out what driver the moderation settings apply to.

Thanks,

Bart.

Re: [PATCH rfc 2/5] irq-am: add some debugfs exposure on tuning state

[Sagi Grimberg]

>> +static int irq_am_register_debugfs(struct irq_am *am)
>> +{
>> +	char name[20];
>> +
>> +	snprintf(name, sizeof(name), "am%u", am->id);
>> +	am->debugfs_dir = debugfs_create_dir(name,
>> +				irq_am_debugfs_root);
> 
> How is a user expected to figure out which driver am%u is associated with?
> Please make sure that these directories have a name that makes it easy for
> users to figure out what driver the moderation settings apply to.

You're right... It was just for me for local debugging at this point and
had every intention to get the debugfs into shape once I collect an
initial round of reviews.

[PATCH rfc 3/5] irq_poll: wire up irq_am

[Sagi Grimberg]

Update online stats for fired event and completions on
each poll cycle.

Also expose am initialization interface. The irqpoll consumer will
initialize the irq-am context of the irq-poll context.

Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 include/linux/irq_poll.h |  9 +++++++++
 lib/Kconfig              |  1 +
 lib/irq_poll.c           | 30 +++++++++++++++++++++++++++++-
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/linux/irq_poll.h b/include/linux/irq_poll.h
index 16aaeccb65cb..de3055359fd8 100644
--- a/include/linux/irq_poll.h
+++ b/include/linux/irq_poll.h
@@ -2,14 +2,20 @@
 #ifndef IRQ_POLL_H
 #define IRQ_POLL_H
 
+#include <linux/irq-am.h>
+
 struct irq_poll;
 typedef int (irq_poll_fn)(struct irq_poll *, int);
+typedef int (irq_poll_am_fn)(struct irq_poll *, unsigned short);
 
 struct irq_poll {
 	struct list_head list;
 	unsigned long state;
 	int weight;
 	irq_poll_fn *poll;
+
+	struct irq_am am;
+	irq_poll_am_fn *amfn;
 };
 
 enum {
@@ -23,4 +29,7 @@ extern void irq_poll_complete(struct irq_poll *);
 extern void irq_poll_enable(struct irq_poll *);
 extern void irq_poll_disable(struct irq_poll *);
 
+extern void irq_poll_init_am(struct irq_poll *iop, unsigned int nr_events,
+        unsigned short nr_levels, unsigned short start_level,
+	irq_poll_am_fn *amfn);
 #endif
diff --git a/lib/Kconfig b/lib/Kconfig
index bbb4c9eea84d..d495b21cd241 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -511,6 +511,7 @@ config IRQ_AM
 
 config IRQ_POLL
 	bool "IRQ polling library"
+	select IRQ_AM
 	help
 	  Helper library to poll interrupt mitigation using polling.
 
diff --git a/lib/irq_poll.c b/lib/irq_poll.c
index 86a709954f5a..6bc86a677d8c 100644
--- a/lib/irq_poll.c
+++ b/lib/irq_poll.c
@@ -53,6 +53,7 @@ static void __irq_poll_complete(struct irq_poll *iop)
 	list_del(&iop->list);
 	smp_mb__before_atomic();
 	clear_bit_unlock(IRQ_POLL_F_SCHED, &iop->state);
+	irq_am_add_event(&iop->am);
 }
 
 /**
@@ -106,8 +107,10 @@ static void __latent_entropy irq_poll_softirq(struct softirq_action *h)
 
 		weight = iop->weight;
 		work = 0;
-		if (test_bit(IRQ_POLL_F_SCHED, &iop->state))
+		if (test_bit(IRQ_POLL_F_SCHED, &iop->state)) {
 			work = iop->poll(iop, weight);
+			irq_am_add_comps(&iop->am, work);
+		}
 
 		budget -= work;
 
@@ -144,6 +147,7 @@ static void __latent_entropy irq_poll_softirq(struct softirq_action *h)
  **/
 void irq_poll_disable(struct irq_poll *iop)
 {
+	irq_am_cleanup(&iop->am);
 	set_bit(IRQ_POLL_F_DISABLE, &iop->state);
 	while (test_and_set_bit(IRQ_POLL_F_SCHED, &iop->state))
 		msleep(1);
@@ -185,6 +189,30 @@ void irq_poll_init(struct irq_poll *iop, int weight, irq_poll_fn *poll_fn)
 }
 EXPORT_SYMBOL(irq_poll_init);
 
+static int irq_poll_am(struct irq_am *am, unsigned short level)
+{
+	struct irq_poll *iop = container_of(am, struct irq_poll, am);
+
+	return iop->amfn(iop, level);
+}
+
+/**
+ * irq_poll_init_am - Initialize adaptive moderation parameters on this @iop
+ * @iop:      The parent iopoll structure
+ * @weight:   The default weight (or command completion budget)
+ * @poll_fn:  The handler to invoke
+ *
+ * Description:
+ *     Initialize adaptive moderation for this irq_poll structure.
+ **/
+void irq_poll_init_am(struct irq_poll *iop, unsigned int nr_events,
+        unsigned short nr_levels, unsigned short start_level, irq_poll_am_fn *amfn)
+{
+	iop->amfn = amfn;
+	irq_am_init(&iop->am, nr_events, nr_levels, start_level, irq_poll_am);
+}
+EXPORT_SYMBOL(irq_poll_init_am);
+
 static int irq_poll_cpu_dead(unsigned int cpu)
 {
 	/*
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rfc 3/5] irq_poll: wire up irq_am

[Bart Van Assche]

On Tue, 2018-02-06 at 00:03 +0200, Sagi Grimberg wrote:
> +void irq_poll_init_am(struct irq_poll *iop, unsigned int nr_events,
> +        unsigned short nr_levels, unsigned short start_level, irq_poll_am_fn *amfn)
> +{
> +	iop->amfn = amfn;
> +	irq_am_init(&iop->am, nr_events, nr_levels, start_level, irq_poll_am);
> +}

This function has a large number of parameters and most of them are passed verbatim to
irq_am_init(). Please consider to introduce a structure for these parameters such that
the number of function arguments stays reasonable.

Thanks,

Bart.

Re: [PATCH rfc 3/5] irq_poll: wire up irq_am

[Sagi Grimberg]

Hey Bart,

>> +void irq_poll_init_am(struct irq_poll *iop, unsigned int nr_events,
>> +        unsigned short nr_levels, unsigned short start_level, irq_poll_am_fn *amfn)
>> +{
>> +	iop->amfn = amfn;
>> +	irq_am_init(&iop->am, nr_events, nr_levels, start_level, irq_poll_am);
>> +}
> 
> This function has a large number of parameters and most of them are passed verbatim to
> irq_am_init(). Please consider to introduce a structure for these parameters such that
> the number of function arguments stays reasonable.

I can definitely change that.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rfc 4/5] IB/cq: add adaptive moderation support

[Sagi Grimberg]

Currently activated via modparam, obviously we will want to
find a more generic way to control this.

Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 drivers/infiniband/core/cq.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/infiniband/core/cq.c b/drivers/infiniband/core/cq.c
index f2ae75fa3128..270801d28f9d 100644
--- a/drivers/infiniband/core/cq.c
+++ b/drivers/infiniband/core/cq.c
@@ -25,6 +25,51 @@
 #define IB_POLL_FLAGS \
 	(IB_CQ_NEXT_COMP | IB_CQ_REPORT_MISSED_EVENTS)
 
+#define IB_CQ_AM_NR_LEVELS	8
+#define IB_CQ_AM_NR_EVENTS	64
+
+struct ib_cq_moder {
+	u16 usec;
+	u16 comps;
+};
+
+/* XXX: magic adaptive moderation profiles */
+static const struct ib_cq_moder am_prof[IB_CQ_AM_NR_LEVELS] = {
+	{1,  1},
+	{1,  2},
+	{2,  4},
+	{4,  8},
+	{8, 16},
+	{16, 32},
+	{32, 48},
+	{32, 64},
+};
+
+static bool use_am = true;
+module_param(use_am, bool, 0444);
+MODULE_PARM_DESC(use_am, "Use cq adaptive moderation");
+
+static int ib_cq_am(struct ib_cq *cq, unsigned short level)
+{
+	u16 usec;
+	u16 comps;
+
+	if (!use_am)
+		return 0;
+
+	usec = am_prof[level].usec;
+	comps = am_prof[level].comps;
+
+	return cq->device->modify_cq(cq, comps, usec);
+}
+
+static int ib_cq_irqpoll_am(struct irq_poll *iop, unsigned short level)
+{
+	struct ib_cq *cq = container_of(iop, struct ib_cq, iop);
+
+	return ib_cq_am(cq, level);
+}
+
 static int __ib_process_cq(struct ib_cq *cq, int budget)
 {
 	int i, n, completed = 0;
@@ -162,6 +207,9 @@ struct ib_cq *ib_alloc_cq(struct ib_device *dev, void *private,
 		cq->comp_handler = ib_cq_completion_softirq;
 
 		irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ, ib_poll_handler);
+		if (cq->device->modify_cq)
+			irq_poll_init_am(&cq->iop, IB_CQ_AM_NR_EVENTS,
+				IB_CQ_AM_NR_LEVELS, 0, ib_cq_irqpoll_am);
 		ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
 		break;
 	case IB_POLL_WORKQUEUE:
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rfc 0/5] generic adaptive IRQ moderation library for I/O devices

[Or Gerlitz]

On Tue, Feb 6, 2018 at 12:03 AM, Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org> wrote:
[...]
> In the networking stack, each device driver implements adaptive IRQ moderation
> on its own. The approach here is a bit different, it tries to take the common denominator,
> which is per-queue statistics gathering and workload change identification
> (basically decides if the moderation scheme needs to change).

not any more, Andy and Tal made the mlx5 AM code a kernel library
which is called DIM

f4e5f0e MAINTAINERS: add entry for Dynamic Interrupt Moderation
6a8788f bnxt_en: add support for software dynamic interrupt moderation
8115b75 net/dim: use struct net_dim_sample as arg to net_dim
4c4dbb4 net/mlx5e: Move dynamic interrupt coalescing code to include/linux
9a31742 net/mlx5e: Change Mellanox references in DIM code
b9c872f net/mlx5e: Move generic functions to new file
f5e7f67 net/mlx5e: Move AM logic enums
138968e net/mlx5e: Remove rq references in mlx5e_rx_am
f58ee09 net/mlx5e: Move interrupt moderation forward declarations
98dd1ed net/mlx5e: Move interrupt moderation structs to new file

can you make use of that? cc-ing them in case you have questions/comments


> The library is targeted to multi-queue devices, but should work on single queue
> devices as well, however I'm not sure that these devices will need something
> like interrupt moderation.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rfc 0/5] generic adaptive IRQ moderation library for I/O devices

[Sagi Grimberg]

Hey Or,

> not any more, Andy and Tal made the mlx5 AM code a kernel library
> which is called DIM
> 
> f4e5f0e MAINTAINERS: add entry for Dynamic Interrupt Moderation
> 6a8788f bnxt_en: add support for software dynamic interrupt moderation
> 8115b75 net/dim: use struct net_dim_sample as arg to net_dim
> 4c4dbb4 net/mlx5e: Move dynamic interrupt coalescing code to include/linux
> 9a31742 net/mlx5e: Change Mellanox references in DIM code
> b9c872f net/mlx5e: Move generic functions to new file
> f5e7f67 net/mlx5e: Move AM logic enums
> 138968e net/mlx5e: Remove rq references in mlx5e_rx_am
> f58ee09 net/mlx5e: Move interrupt moderation forward declarations
> 98dd1ed net/mlx5e: Move interrupt moderation structs to new file
> 
> can you make use of that? cc-ing them in case you have questions/comments

Thanks a lot for the pointer, I'm not following net-next regularly.
This seems to be a copy of the mlx5 code.

So as mentioned in the cover letter, the implementation was inspired
from the mlx5 driver as it had some of the abstraction properties I was
aiming to achieve.

What I didn't mention, was that I started from using the mlx5
implementation as is (slightly modified). Unfortunately in the
workloads that I've tested, I was not able to get the am level to
converge. So I went in a slightly different direction which gave me
better results (although still not perfect and lightly tested).

This led me to believe that mlx5 offered strategy might not suite
storage I/O workloads (although I do acknowledge that I could be proven
wrong here).

For example, a design choice I took was that the moderation scheme would
be more aggressive towards latency on the expense of throughput
optimization since high end storage devices are often expected to meet
strict latency requirements. Does that makes sense for network devices?
I don't know.

Another difference is that unlike net devices, storage I/O transactions
usually look like request/response where the data transfer is offloaded
by the controller/hba (net devices moderate on raw RX datagrams).
Moreover, it might not be trivial to track the bytes/sec from a
completion at all (might be embedded somewhere in the consumer context).

So overall, at this point I'm a bit skeptical that it will makes sense
to commonise the two implementations. I'd like to hear some more input
if this is something that the community is interested in first. If this
is something people are interested in, we can look if it makes sense to
reuse net_dim.h or not.

Re: [PATCH rfc 0/5] generic adaptive IRQ moderation library for I/O devices

[Or Gerlitz]

On Tue, Feb 6, 2018 at 11:25 AM, Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org> wrote:
> Hey Or,
>
>> not any more, Andy and Tal made the mlx5 AM code a kernel library
>> which is called DIM
>>
>> f4e5f0e MAINTAINERS: add entry for Dynamic Interrupt Moderation
>> 6a8788f bnxt_en: add support for software dynamic interrupt moderation
>> 8115b75 net/dim: use struct net_dim_sample as arg to net_dim
>> 4c4dbb4 net/mlx5e: Move dynamic interrupt coalescing code to include/linux
>> 9a31742 net/mlx5e: Change Mellanox references in DIM code
>> b9c872f net/mlx5e: Move generic functions to new file
>> f5e7f67 net/mlx5e: Move AM logic enums
>> 138968e net/mlx5e: Remove rq references in mlx5e_rx_am
>> f58ee09 net/mlx5e: Move interrupt moderation forward declarations
>> 98dd1ed net/mlx5e: Move interrupt moderation structs to new file
>>
>> can you make use of that? cc-ing them in case you have questions/comments
>
>
> Thanks a lot for the pointer, I'm not following net-next regularly.
> This seems to be a copy of the mlx5 code.

Just to make it clear, the code was moved to a library and not copied... so
there's single instance of the code upstream and lets try to find a way for
your use case to take advantage of it possibly under some modifications.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rfc 0/5] generic adaptive IRQ moderation library for I/O devices

[Or Gerlitz]

On Tue, Feb 6, 2018 at 12:03 AM, Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org> wrote:

> The main reason why this implementation is different then the common networking devices
> implementation (and kept separate) is that in my mind at least, network devices are different
> animals than other I/O devices in the sense that:

Oh, I see now  that you do refer to the netdev library, I was confused
by "In the networking stack, each device driver
implements adaptive IRQ moderation on its own" comment.

> (a) network devices rely heavily on byte count of raw ethernet frames for adaptive moderation
>     while in storage or I/O, the byte count is often a result of a submission/completion transaction
>     and sometimes even known only to the application on top of the infrastructure (like in the
>     rdma case).

> (b) Performance characteristics and expectations in representative workloads.
> (c) network devices collect all sort of stats for different functionalities (where adaptive moderation
>     is only one use-case) and I'm not sure at all that a subset of stats could easily migrate to a different
>     context.

I think Tal has idea/s on how the existing library can be changed to
support more modes/models
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rfc 0/5] generic adaptive IRQ moderation library for I/O devices

[Tal Gilboa]

On 2/6/2018 10:54 AM, Or Gerlitz wrote:
> On Tue, Feb 6, 2018 at 12:03 AM, Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org> wrote:
> 
>> The main reason why this implementation is different then the common networking devices
>> implementation (and kept separate) is that in my mind at least, network devices are different
>> animals than other I/O devices in the sense that:
> 
> Oh, I see now  that you do refer to the netdev library, I was confused
> by "In the networking stack, each device driver
> implements adaptive IRQ moderation on its own" comment.
> 
>> (a) network devices rely heavily on byte count of raw ethernet frames for adaptive moderation
>>      while in storage or I/O, the byte count is often a result of a submission/completion transaction
>>      and sometimes even known only to the application on top of the infrastructure (like in the
>>      rdma case).
> 
>> (b) Performance characteristics and expectations in representative workloads.
>> (c) network devices collect all sort of stats for different functionalities (where adaptive moderation
>>      is only one use-case) and I'm not sure at all that a subset of stats could easily migrate to a different
>>      context.
> 
> I think Tal has idea/s on how the existing library can be changed to
> support more modes/models
> 
What I was thinking is allowing DIM algorithm to disregard data which is 
0. Currently if bytes == 0 we return "SAME" immediately. We can change 
it to simply move to the packets check (which may be renamed to 
"completions"). This way you could use DIM while only optimizing to (P1) 
high packet rate and (P2) low interrupt rate.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rfc 0/5] generic adaptive IRQ moderation library for I/O devices

[Sagi Grimberg]

Hi Tal,

>> I think Tal has idea/s on how the existing library can be changed to
>> support more modes/models
>>
> What I was thinking is allowing DIM algorithm to disregard data which is 
> 0. Currently if bytes == 0 we return "SAME" immediately. We can change 
> it to simply move to the packets check (which may be renamed to 
> "completions"). This way you could use DIM while only optimizing to (P1) 
> high packet rate and (P2) low interrupt rate.

That was exactly where I started from. But unfortunately it did not work
well :(

 From my experiments, the moderation was all over the place failing to
converge. At least the workloads that I've tested with, it was more
successful to have a stricter step policy and pulling towards latency
if we are consistently catching single completion per event.

I'm not an expert here at all, but at this point, based on my attempts
so far, I'm not convinced the current net_dim scheme could work.

Re: [PATCH rfc 0/5] generic adaptive IRQ moderation library for I/O devices

[Tal Gilboa]

On 2/6/2018 11:34 AM, Sagi Grimberg wrote:
> Hi Tal,
> 
>>> I think Tal has idea/s on how the existing library can be changed to
>>> support more modes/models
>>>
>> What I was thinking is allowing DIM algorithm to disregard data which 
>> is 0. Currently if bytes == 0 we return "SAME" immediately. We can 
>> change it to simply move to the packets check (which may be renamed to 
>> "completions"). This way you could use DIM while only optimizing to 
>> (P1) high packet rate and (P2) low interrupt rate.
> 
> That was exactly where I started from. But unfortunately it did not work
> well :(
> 
>  From my experiments, the moderation was all over the place failing to
> converge. At least the workloads that I've tested with, it was more
> successful to have a stricter step policy and pulling towards latency
> if we are consistently catching single completion per event.
> 
> I'm not an expert here at all, but at this point, based on my attempts
> so far, I'm not convinced the current net_dim scheme could work.
I do believe we can make it work. I see your addition of the cpe part to 
stats compare. Might not be a bad idea for networking devices. Overall, 
it seems to me like this would be a private case of the general DIM 
optimization, since it doesn't need to account for aggregation, for 
instance, which breaks the "more packets == more data" ratio.

Re: [PATCH rfc 0/5] generic adaptive IRQ moderation library for I/O devices

[Or Gerlitz]

On Tue, Feb 6, 2018 at 11:45 AM, Tal Gilboa <talgi-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> On 2/6/2018 11:34 AM, Sagi Grimberg wrote:
>>
>> Hi Tal,
>>
>>>> I think Tal has idea/s on how the existing library can be changed to
>>>> support more modes/models
>>>>
>>> What I was thinking is allowing DIM algorithm to disregard data which is
>>> 0. Currently if bytes == 0 we return "SAME" immediately. We can change it to
>>> simply move to the packets check (which may be renamed to "completions").
>>> This way you could use DIM while only optimizing to (P1) high packet rate
>>> and (P2) low interrupt rate.
>>
>>
>> That was exactly where I started from. But unfortunately it did not work
>> well :(
>>
>>  From my experiments, the moderation was all over the place failing to
>> converge. At least the workloads that I've tested with, it was more
>> successful to have a stricter step policy and pulling towards latency
>> if we are consistently catching single completion per event.
>>
>> I'm not an expert here at all, but at this point, based on my attempts
>> so far, I'm not convinced the current net_dim scheme could work.
>
> I do believe we can make it work. I see your addition of the cpe part to
> stats compare. Might not be a bad idea for networking devices. Overall, it
> seems to me like this would be a private case of the general DIM
> optimization, since it doesn't need to account for aggregation, for
> instance, which breaks the "more packets == more data" ratio.

Did U2 came to agreement/lead on how to re-use the upstream library
for the matter Sagi is pushing for?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rfc 0/5] generic adaptive IRQ moderation library for I/O devices

[Tal Gilboa]

On 2/13/2018 11:30 AM, Or Gerlitz wrote:
> On Tue, Feb 6, 2018 at 11:45 AM, Tal Gilboa <talgi@mellanox.com> wrote:
>> On 2/6/2018 11:34 AM, Sagi Grimberg wrote:
>>>
>>> Hi Tal,
>>>
>>>>> I think Tal has idea/s on how the existing library can be changed to
>>>>> support more modes/models
>>>>>
>>>> What I was thinking is allowing DIM algorithm to disregard data which is
>>>> 0. Currently if bytes == 0 we return "SAME" immediately. We can change it to
>>>> simply move to the packets check (which may be renamed to "completions").
>>>> This way you could use DIM while only optimizing to (P1) high packet rate
>>>> and (P2) low interrupt rate.
>>>
>>>
>>> That was exactly where I started from. But unfortunately it did not work
>>> well :(
>>>
>>>   From my experiments, the moderation was all over the place failing to
>>> converge. At least the workloads that I've tested with, it was more
>>> successful to have a stricter step policy and pulling towards latency
>>> if we are consistently catching single completion per event.
>>>
>>> I'm not an expert here at all, but at this point, based on my attempts
>>> so far, I'm not convinced the current net_dim scheme could work.
>>
>> I do believe we can make it work. I see your addition of the cpe part to
>> stats compare. Might not be a bad idea for networking devices. Overall, it
>> seems to me like this would be a private case of the general DIM
>> optimization, since it doesn't need to account for aggregation, for
>> instance, which breaks the "more packets == more data" ratio.
> 
> Did U2 came to agreement/lead on how to re-use the upstream library
> for the matter Sagi is pushing for?
> 
I don't think so (yet).
Sagi, I would like to avoid having 2 "net DIM"s if possible. You 
mentioned you tried implementing over net DIM lib and it wasn't working 
well. Can you share this code with me?

[PATCH rfc 5/5] IB/cq: wire up adaptive moderation to workqueue based completion queues

[Sagi Grimberg]

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/infiniband/core/cq.c | 25 ++++++++++++++++++++-----
 include/rdma/ib_verbs.h      |  9 +++++++--
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/core/cq.c b/drivers/infiniband/core/cq.c
index 270801d28f9d..1d984fd77449 100644
--- a/drivers/infiniband/core/cq.c
+++ b/drivers/infiniband/core/cq.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/err.h>
 #include <linux/slab.h>
+#include <linux/irq-am.h>
 #include <rdma/ib_verbs.h>
 
 /* # of WCs to poll for with a single call to ib_poll_cq */
@@ -70,6 +71,13 @@ static int ib_cq_irqpoll_am(struct irq_poll *iop, unsigned short level)
 	return ib_cq_am(cq, level);
 }
 
+static int ib_cq_workqueue_am(struct irq_am *am, unsigned short level)
+{
+	struct ib_cq *cq = container_of(am, struct ib_cq, wq.am);
+
+	return ib_cq_am(cq, level);
+}
+
 static int __ib_process_cq(struct ib_cq *cq, int budget)
 {
 	int i, n, completed = 0;
@@ -147,18 +155,21 @@ static void ib_cq_completion_softirq(struct ib_cq *cq, void *private)
 
 static void ib_cq_poll_work(struct work_struct *work)
 {
-	struct ib_cq *cq = container_of(work, struct ib_cq, work);
+	struct ib_cq *cq = container_of(work, struct ib_cq, wq.work);
 	int completed;
 
 	completed = __ib_process_cq(cq, IB_POLL_BUDGET_WORKQUEUE);
+	irq_am_add_comps(&cq->wq.am, completed);
 	if (completed >= IB_POLL_BUDGET_WORKQUEUE ||
 	    ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
-		queue_work(ib_comp_wq, &cq->work);
+		queue_work(ib_comp_wq, &cq->wq.work);
+	else
+		irq_am_add_event(&cq->wq.am);
 }
 
 static void ib_cq_completion_workqueue(struct ib_cq *cq, void *private)
 {
-	queue_work(ib_comp_wq, &cq->work);
+	queue_work(ib_comp_wq, &cq->wq.work);
 }
 
 /**
@@ -214,7 +225,10 @@ struct ib_cq *ib_alloc_cq(struct ib_device *dev, void *private,
 		break;
 	case IB_POLL_WORKQUEUE:
 		cq->comp_handler = ib_cq_completion_workqueue;
-		INIT_WORK(&cq->work, ib_cq_poll_work);
+		INIT_WORK(&cq->wq.work, ib_cq_poll_work);
+		if (cq->device->modify_cq)
+			irq_am_init(&cq->wq.am, IB_CQ_AM_NR_EVENTS,
+				IB_CQ_AM_NR_LEVELS, 0, ib_cq_workqueue_am);
 		ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
 		break;
 	default:
@@ -250,7 +264,8 @@ void ib_free_cq(struct ib_cq *cq)
 		irq_poll_disable(&cq->iop);
 		break;
 	case IB_POLL_WORKQUEUE:
-		cancel_work_sync(&cq->work);
+		cancel_work_sync(&cq->wq.work);
+		irq_am_cleanup(&cq->wq.am);
 		break;
 	default:
 		WARN_ON_ONCE(1);
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index fd84cda5ed7c..14a93566adb6 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1555,6 +1555,11 @@ enum ib_poll_context {
 	IB_POLL_WORKQUEUE,	/* poll from workqueue */
 };
 
+struct ib_cq_workqueue_poll {
+	struct irq_am		am;
+	struct work_struct	work;
+};
+
 struct ib_cq {
 	struct ib_device       *device;
 	struct ib_uobject      *uobject;
@@ -1566,8 +1571,8 @@ struct ib_cq {
 	enum ib_poll_context	poll_ctx;
 	struct ib_wc		*wc;
 	union {
-		struct irq_poll		iop;
-		struct work_struct	work;
+		struct irq_poll			iop;
+		struct ib_cq_workqueue_poll	wq;
 	};
 };
 
-- 
2.14.1