[PATCH] vmevent: Use 'struct vmevent_attr' for vmevent_fd() ABI

[PATCH] vmevent: Use 'struct vmevent_attr' for vmevent_fd() ABI

[Pekka Enberg]

This patch introduces 'struct vmevent_attr' and converts the vmevent_fd() ABI
to use it which makes the ABI much more flexible.

Originally-by: Leonid Moiseichuk <leonid.moiseichuk@nokia.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Anton Vorontsov <anton.vorontsov@linaro.org>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 include/linux/vmevent.h              |   66 +++++++++----
 mm/vmevent.c                         |  186 +++++++++++++++++++++++-----------
 tools/testing/vmevent/vmevent-test.c |   54 +++++++---
 3 files changed, 212 insertions(+), 94 deletions(-)

diff --git a/include/linux/vmevent.h b/include/linux/vmevent.h
index 4f577ee..64357e4 100644
--- a/include/linux/vmevent.h
+++ b/include/linux/vmevent.h
@@ -3,53 +3,83 @@
 
 #include <linux/types.h>
 
+/*
+ * Types of memory attributes which could be monitored through vmevent API
+ */
 enum {
-	VMEVENT_TYPE_FREE_THRESHOLD	= 1ULL << 0,
-	VMEVENT_TYPE_SAMPLE		= 1ULL << 1,
+	VMEVENT_ATTR_NR_AVAIL_PAGES	= 1UL,
+	VMEVENT_ATTR_NR_FREE_PAGES	= 2UL,
+	VMEVENT_ATTR_NR_SWAP_PAGES	= 3UL,
+
+	VMEVENT_ATTR_MAX		/* non-ABI */
 };
 
+/*
+ * Attribute state bits for threshold
+ */
 enum {
-	VMEVENT_EATTR_NR_AVAIL_PAGES	= 1ULL << 0,
-	VMEVENT_EATTR_NR_FREE_PAGES	= 1ULL << 1,
-	VMEVENT_EATTR_NR_SWAP_PAGES	= 1ULL << 2,
+	/*
+	 * Sample value is less than user-specified value
+	 */
+	VMEVENT_ATTR_STATE_VALUE_LT	= (1UL << 0),
 };
 
-struct vmevent_config {
+struct vmevent_attr {
 	/*
-	 * Size of the struct for ABI extensibility.
+	 * Value in pages delivered with pointed attribute
 	 */
-	__u32			size;
+	__u64			value;
 
 	/*
-	 * Notification type bitmask
+	 * Type of profiled attribute from VMEVENT_ATTR_XXX
 	 */
-	__u64			type;
+	__u32			type;
+
+        /*
+	 * Bitmask of current attribute value (see VMEVENT_ATTR_STATE_XXX)
+	*/
+	__u32			state;
+};
 
+#define VMEVENT_CONFIG_MAX_ATTRS	32
+
+/*
+ * Configuration structure to get notifications and attributes values
+ */
+struct vmevent_config {
 	/*
-	 * Attributes that are delivered as part of events.
+	 * Size of the struct for ABI extensibility.
 	 */
-	__u64			event_attrs;
+	__u32			size;
 
 	/*
-	 * Threshold of free pages in the system.
+	 * Counter of number monitored attributes
 	 */
-	__u32			free_pages_threshold;
+	__u32			counter;
 
 	/*
 	 * Sample period in nanoseconds
 	 */
 	__u64			sample_period_ns;
+
+	/*
+	 * Attributes that are monitored and delivered as part of events
+	 */
+	struct vmevent_attr	attrs[VMEVENT_CONFIG_MAX_ATTRS];
 };
 
 struct vmevent_event {
 	/*
-	 * Size of the struct for ABI extensibility.
+	 * Counter of attributes in this VM event
 	 */
-	__u32			size;
+	__u32			counter;
 
-	__u64			attrs;
+	__u32			padding;
 
-	__u64			attr_values[];
+	/*
+	 * Attributes for this VM event
+	 */
+	struct vmevent_attr	attrs[];
 };
 
 #endif /* _LINUX_VMEVENT_H */
diff --git a/mm/vmevent.c b/mm/vmevent.c
index 37d2c5f..ab6a043 100644
--- a/mm/vmevent.c
+++ b/mm/vmevent.c
@@ -24,10 +24,10 @@ struct vmevent_watch {
 	bool				pending;
 
 	/*
- 	 * Attributes
- 	 */
+	 * Attributes that are exported as part of delivered VM events.
+	 */
 	unsigned long			nr_attrs;
-	u64				attr_values[64];
+	struct vmevent_attr		*sample_attrs;
 
 	/* sampling */
 	struct hrtimer			timer;
@@ -36,54 +36,87 @@ struct vmevent_watch {
 	wait_queue_head_t		waitq;
 };
 
-static bool vmevent_match(struct vmevent_watch *watch,
-			   struct vmevent_watch_event *event)
+typedef u64 (*vmevent_attr_sample_fn)(struct vmevent_watch *watch);
+
+static u64 vmevent_attr_swap_pages(struct vmevent_watch *watch)
 {
-	if (watch->config.type & VMEVENT_TYPE_FREE_THRESHOLD) {
-		if (event->nr_free_pages > watch->config.free_pages_threshold)
-			return false;
-	}
+#ifdef CONFIG_SWAP
+	struct sysinfo si;
+
+	si_swapinfo(&si);
 
-	return true;
+	return si.totalswap;
+#else
+	return 0;
+#endif
 }
 
-static void vmevent_sample(struct vmevent_watch *watch)
+static u64 vmevent_attr_free_pages(struct vmevent_watch *watch)
+{
+	return global_page_state(NR_FREE_PAGES);
+}
+
+static u64 vmevent_attr_avail_pages(struct vmevent_watch *watch)
 {
-	struct vmevent_watch_event event;
 	struct sysinfo si;
-	int n = 0;
 
-	memset(&event, 0, sizeof(event));
+	si_meminfo(&si);
 
-	event.nr_free_pages	= global_page_state(NR_FREE_PAGES);
+	return si.totalram;
+}
 
-	si_meminfo(&si);
-	event.nr_avail_pages	= si.totalram;
+static vmevent_attr_sample_fn attr_samplers[] = {
+	[VMEVENT_ATTR_NR_AVAIL_PAGES]   = vmevent_attr_avail_pages,
+	[VMEVENT_ATTR_NR_FREE_PAGES]    = vmevent_attr_free_pages,
+	[VMEVENT_ATTR_NR_SWAP_PAGES]    = vmevent_attr_swap_pages,
+};
 
-#ifdef CONFIG_SWAP
-	if (watch->config.event_attrs & VMEVENT_EATTR_NR_SWAP_PAGES) {
-		si_swapinfo(&si);
-		event.nr_swap_pages	= si.totalswap;
+static u64 vmevent_sample_attr(struct vmevent_watch *watch, struct vmevent_attr *attr)
+{
+	vmevent_attr_sample_fn fn = attr_samplers[attr->type];
+
+	return fn(watch);
+}
+
+static bool vmevent_match(struct vmevent_watch *watch)
+{
+	struct vmevent_config *config = &watch->config;
+	int i;
+
+	for (i = 0; i < config->counter; i++) {
+		struct vmevent_attr *attr = &config->attrs[i];
+		u64 value;
+
+		if (!attr->state)
+			continue;
+
+		value = vmevent_sample_attr(watch, attr);
+
+		if (attr->state & VMEVENT_ATTR_STATE_VALUE_LT) {
+			if (value < attr->value)
+				return true;
+		}
 	}
-#endif
 
-	if (!vmevent_match(watch, &event))
+	return false;
+}
+
+static void vmevent_sample(struct vmevent_watch *watch)
+{
+	int i;
+
+	if (!vmevent_match(watch))
 		return;
 
 	mutex_lock(&watch->mutex);
 
 	watch->pending = true;
 
-	if (watch->config.event_attrs & VMEVENT_EATTR_NR_AVAIL_PAGES)
-		watch->attr_values[n++] = event.nr_avail_pages;
-
-	if (watch->config.event_attrs & VMEVENT_EATTR_NR_FREE_PAGES)
-		watch->attr_values[n++] = event.nr_free_pages;
-
-	if (watch->config.event_attrs & VMEVENT_EATTR_NR_SWAP_PAGES)
-		watch->attr_values[n++] = event.nr_swap_pages;
+	for (i = 0; i < watch->nr_attrs; i++) {
+		struct vmevent_attr *attr = &watch->sample_attrs[i];
 
-	watch->nr_attrs = n;
+		attr->value = vmevent_sample_attr(watch, attr);
+	}
 
 	mutex_unlock(&watch->mutex);
 }
@@ -132,43 +165,45 @@ static unsigned int vmevent_poll(struct file *file, poll_table *wait)
 static ssize_t vmevent_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 {
 	struct vmevent_watch *watch = file->private_data;
-	struct vmevent_event event;
+	struct vmevent_event *event;
 	ssize_t ret = 0;
-	u64 attr_size;
-
-	mutex_lock(&watch->mutex);
+	u32 size;
+	int i;
 
-	if (!watch->pending)
-		goto out_unlock;
+	size = sizeof(*event) + watch->nr_attrs * sizeof(struct vmevent_attr);
 
-	attr_size = watch->nr_attrs * sizeof(u64);
+	if (count < size)
+		return -EINVAL;
 
-	memset(&event, 0, sizeof(event));
-	event.size	= sizeof(struct vmevent_event) + attr_size;
-	event.attrs	= watch->config.event_attrs;
+	mutex_lock(&watch->mutex);
 
-	if (count < sizeof(event))
+	if (!watch->pending)
 		goto out_unlock;
 
-	if (copy_to_user(buf, &event, sizeof(event))) {
-		ret = -EFAULT;
+	event = kmalloc(size, GFP_KERNEL);
+	if (!event) {
+		ret = -ENOMEM;
 		goto out_unlock;
 	}
 
-	count -= sizeof(event);
+	for (i = 0; i < watch->nr_attrs; i++) {
+		memcpy(&event->attrs[i], &watch->sample_attrs[i], sizeof(struct vmevent_attr));
+	}
 
-	if (count > attr_size)
-		count = attr_size;
+	event->counter = watch->nr_attrs;
 
-	if (copy_to_user(buf + sizeof(event), watch->attr_values, count)) {
+	if (copy_to_user(buf, event, size)) {
 		ret = -EFAULT;
-		goto out_unlock;
+		goto out_free;
 	}
 
 	ret = count;
 
 	watch->pending = false;
 
+out_free:
+	kfree(event);
+
 out_unlock:
 	mutex_unlock(&watch->mutex);
 
@@ -207,6 +242,42 @@ static struct vmevent_watch *vmevent_watch_alloc(void)
 	return watch;
 }
 
+static int vmevent_setup_watch(struct vmevent_watch *watch)
+{
+	struct vmevent_config *config = &watch->config;
+	struct vmevent_attr *attrs = NULL;
+	unsigned long nr;
+	int i;
+
+	nr = 0;
+
+	for (i = 0; i < config->counter; i++) {
+		struct vmevent_attr *attr = &config->attrs[i];
+		size_t size;
+		void *new;
+
+		if (attr->type >= VMEVENT_ATTR_MAX)
+			continue;
+
+		size = sizeof(struct vmevent_attr) * (nr + 1);
+
+		new = krealloc(attrs, size, GFP_KERNEL);
+		if (!new) {
+			kfree(attrs);
+			return -ENOMEM;
+		}
+
+		attrs = new;
+
+		attrs[nr++].type = attr->type;
+	}
+
+	watch->sample_attrs	= attrs;
+	watch->nr_attrs		= nr;
+
+	return 0;
+}
+
 static int vmevent_copy_config(struct vmevent_config __user *uconfig,
 				struct vmevent_config *config)
 {
@@ -216,14 +287,6 @@ static int vmevent_copy_config(struct vmevent_config __user *uconfig,
 	if (ret)
 		return -EFAULT;
 
-	if (!config->type)
-		return -EINVAL;
-
-	if (config->type & VMEVENT_TYPE_SAMPLE) {
-		if (config->sample_period_ns < NSEC_PER_MSEC)
-			return -EINVAL;
-	}
-
 	return 0;
 }
 
@@ -243,6 +306,10 @@ SYSCALL_DEFINE1(vmevent_fd,
 	if (err)
 		goto err_free;
 
+	err = vmevent_setup_watch(watch);
+	if (err)
+		goto err_free;
+
 	fd = get_unused_fd_flags(O_RDONLY);
 	if (fd < 0) {
 		err = fd;
@@ -257,8 +324,7 @@ SYSCALL_DEFINE1(vmevent_fd,
 
 	fd_install(fd, file);
 
-	if (watch->config.type & VMEVENT_TYPE_SAMPLE)
-		vmevent_start_timer(watch);
+	vmevent_start_timer(watch);
 
 	return fd;
 
diff --git a/tools/testing/vmevent/vmevent-test.c b/tools/testing/vmevent/vmevent-test.c
index f268034..534f827 100644
--- a/tools/testing/vmevent/vmevent-test.c
+++ b/tools/testing/vmevent/vmevent-test.c
@@ -32,12 +32,24 @@ int main(int argc, char *argv[])
 	printf("Physical pages: %ld\n", phys_pages);
 
 	config = (struct vmevent_config) {
-		.type			= VMEVENT_TYPE_SAMPLE | VMEVENT_TYPE_FREE_THRESHOLD,
-		.event_attrs		= VMEVENT_EATTR_NR_AVAIL_PAGES
-					| VMEVENT_EATTR_NR_FREE_PAGES
-					| VMEVENT_EATTR_NR_SWAP_PAGES,
 		.sample_period_ns	= 1000000000L,
-		.free_pages_threshold	= phys_pages,
+		.counter		= 4,
+		.attrs			= {
+			[0]			= {
+				.type	= VMEVENT_ATTR_NR_FREE_PAGES,
+				.state	= VMEVENT_ATTR_STATE_VALUE_LT,
+				.value	= phys_pages,
+			},
+			[1]			= {
+				.type	= VMEVENT_ATTR_NR_AVAIL_PAGES,
+			},
+			[2]			= {
+				.type	= VMEVENT_ATTR_NR_SWAP_PAGES,
+			},
+			[3]			= {
+				.type	= 0xffff, /* invalid */
+			},
+		},
 	};
 
 	fd = sys_vmevent_fd(&config);
@@ -47,9 +59,10 @@ int main(int argc, char *argv[])
 	}
 
 	for (i = 0; i < 10; i++) {
-		char buffer[sizeof(struct vmevent_event) + 3 * sizeof(uint64_t)];
+		char buffer[sizeof(struct vmevent_event) + 4 * sizeof(struct vmevent_attr)];
 		struct vmevent_event *event;
 		int n = 0;
+		int idx;
 
 		pollfd.fd		= fd;
 		pollfd.events		= POLLIN;
@@ -68,16 +81,25 @@ int main(int argc, char *argv[])
 
 		event = (void *) buffer;
 
-		printf("VM event (%Lu bytes):\n", event->size);
-
-		if (event->attrs & VMEVENT_EATTR_NR_AVAIL_PAGES)
-			printf("  VMEVENT_EATTR_NR_AVAIL_PAGES: %Lu\n", event->attr_values[n++]);
-
-		if (event->attrs & VMEVENT_EATTR_NR_FREE_PAGES)
-			printf("  VMEVENT_EATTR_NR_FREE_PAGES : %Lu\n", event->attr_values[n++]);
-
-		if (event->attrs & VMEVENT_EATTR_NR_SWAP_PAGES)
-			printf("  VMEVENT_EATTR_NR_SWAP_PAGES : %Lu\n", event->attr_values[n++]);
+		printf("VM event (%u attributes):\n", event->counter);
+
+		for (idx = 0; idx < event->counter; idx++) {
+			struct vmevent_attr *attr = &event->attrs[idx];
+
+			switch (attr->type) {
+			case VMEVENT_ATTR_NR_AVAIL_PAGES:
+				printf("  VMEVENT_ATTR_NR_AVAIL_PAGES: %Lu\n", attr->value);
+				break;
+			case VMEVENT_ATTR_NR_FREE_PAGES:
+				printf("  VMEVENT_ATTR_NR_FREE_PAGES: %Lu\n", attr->value);
+				break;
+			case VMEVENT_ATTR_NR_SWAP_PAGES:
+				printf("  VMEVENT_ATTR_NR_SWAP_PAGES: %Lu\n", attr->value);
+				break;
+			default:
+				printf("  Unknown attribute: %Lu\n", attr->value);
+			}
+		}
 	}
 	if (close(fd) < 0) {
 		perror("close failed");
-- 
1.7.6.5

Re: [PATCH] vmevent: Use 'struct vmevent_attr' for vmevent_fd() ABI

[Anton Vorontsov]

On Tue, Mar 06, 2012 at 10:51:19PM +0200, Pekka Enberg wrote:
> This patch introduces 'struct vmevent_attr' and converts the vmevent_fd() ABI
> to use it which makes the ABI much more flexible.
> 
> Originally-by: Leonid Moiseichuk <leonid.moiseichuk@nokia.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Anton Vorontsov <anton.vorontsov@linaro.org>
> Signed-off-by: Pekka Enberg <penberg@kernel.org>
> ---

Sorry for hijacking this thread, but speaking of big
changes. Are there any plans or ideas to add other methods
of sampling, i.e. something not timer-based?

I like how it's done for cgroups, i.e. via rate-limited events
(and the events are "page alloc/free").

But doing this cgroup-way would probably dismiss the point of
"lightweight" notifications.

Though, current vmevent seems not so lightweigh in sense of
battery usage and accuracy (i.e. how quick we're able to
detect the crossed threshold). To get better accuracy we
would need to run timer at higher frequencies, but then we
would waste more battery.

Sure, vmevent is still lightweight in sense that it does not
cause much runtime overhead or memory wastage (unlike cgroups).

The only idea I have for vmevent is to make some hybrid: timer
plus shrinker API. That way we would detect "low memory" events
fast enough via shrinker API, and thus run timer at low freq.

But we would still have a running timer, which would wake up
a system periodically, which is still quite bad. :-/

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

RE: [PATCH] vmevent: Use 'struct vmevent_attr' for vmevent_fd() ABI

[leonid.moiseichuk]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1505 bytes --]

> -----Original Message-----
> From: ext Anton Vorontsov [mailto:anton.vorontsov@linaro.org]
> Sent: 07 March, 2012 01:00
> To: Pekka Enberg
> Cc: linux-kernel@vger.kernel.org; Moiseichuk Leonid (Nokia-MP/Espoo);
...
> Sorry for hijacking this thread, but speaking of big changes. Are there any
> plans or ideas to add other methods of sampling, i.e. something not timer-
> based?
...
> Though, current vmevent seems not so lightweigh in sense of battery usage
> and accuracy (i.e. how quick we're able to detect the crossed threshold). To
> get better accuracy we would need to run timer at higher frequencies, but
> then we would waste more battery.

One of patch I sent was about switching to deferred timer. This API should not be 100% accurate first, and using deferred
timers will allow to sleep properly when system has no activity. I hope Pekka will accept the next patch as well.

> 
> Sure, vmevent is still lightweight in sense that it does not cause much
> runtime overhead or memory wastage (unlike cgroups).

"Timer work" is really lightweight.

> 
> The only idea I have for vmevent is to make some hybrid: timer plus shrinker
> API. That way we would detect "low memory" events fast enough via
> shrinker API, and thus run timer at low freq.

I used the same, see post about memnotify. Shrinker roll timer to be triggered soon.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] vmevent: Use 'struct vmevent_attr' for vmevent_fd() ABI

[Pekka Enberg]

On Wed, 7 Mar 2012, leonid.moiseichuk@nokia.com wrote:
> > -----Original Message-----
> > From: ext Anton Vorontsov [mailto:anton.vorontsov@linaro.org]
> > Sent: 07 March, 2012 01:00
> > To: Pekka Enberg
> > Cc: linux-kernel@vger.kernel.org; Moiseichuk Leonid (Nokia-MP/Espoo);
> ...
> > Sorry for hijacking this thread, but speaking of big changes. Are there any
> > plans or ideas to add other methods of sampling, i.e. something not timer-
> > based?
> ...
> > Though, current vmevent seems not so lightweigh in sense of battery usage
> > and accuracy (i.e. how quick we're able to detect the crossed threshold). To
> > get better accuracy we would need to run timer at higher frequencies, but
> > then we would waste more battery.
> 
> One of patch I sent was about switching to deferred timer. This API should not be 100% accurate first, and using deferred
> timers will allow to sleep properly when system has no activity. I hope Pekka will accept the next patch as well.

I merged that patch yesterday:

http://git.kernel.org/?p=linux/kernel/git/penberg/linux.git;a=commitdiff;h=7d5e14c4b0421f7716e759934c0c2524b1d763ea

			Pekka

Re: [PATCH] vmevent: Use 'struct vmevent_attr' for vmevent_fd() ABI

[Pekka Enberg]

On Wed, 7 Mar 2012, Anton Vorontsov wrote:
> Sorry for hijacking this thread, but speaking of big
> changes. Are there any plans or ideas to add other methods
> of sampling, i.e. something not timer-based?

Yes and no.

Linux already has non-timer based "vm events" via tracepoints. There's 
absolutely no need to reinvent the wheel so we should integrate with perf 
more closely.

The biggest open question is whether or not a new syscall is needed for 
configuring memory thresholds and such.

			Pekka

Re: [PATCH] vmevent: Use 'struct vmevent_attr' for vmevent_fd() ABI

[Anton Vorontsov]

On Wed, Mar 07, 2012 at 07:53:37AM +0000, leonid.moiseichuk@nokia.com wrote:
> > -----Original Message-----
> > From: ext Anton Vorontsov [mailto:anton.vorontsov@linaro.org]
> > Sent: 07 March, 2012 01:00
> > To: Pekka Enberg
> > Cc: linux-kernel@vger.kernel.org; Moiseichuk Leonid (Nokia-MP/Espoo);
> ...
> > Sorry for hijacking this thread, but speaking of big changes. Are there any
> > plans or ideas to add other methods of sampling, i.e. something not timer-
> > based?
> ...
> > Though, current vmevent seems not so lightweigh in sense of battery usage
> > and accuracy (i.e. how quick we're able to detect the crossed threshold). To
> > get better accuracy we would need to run timer at higher frequencies, but
> > then we would waste more battery.
> 
> One of patch I sent was about switching to deferred timer. This API should not be 100% accurate first, and using deferred
> timers will allow to sleep properly when system has no activity. I hope Pekka will accept the next patch as well.

Nice idea!

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com