Re: Re: [PATCH v6 01/14] mm: Introduce Data Access MONitor (DAMON)

On Tue, 10 Mar 2020 08:54:05 +0000 Jonathan Cameron [off-list ref] wrote:

Apologies if anyone gets these twice. I had an email server throttling
issue yesterday.

On Mon, 24 Feb 2020 13:30:34 +0100
SeongJae Park [off-list ref] wrote:

quoted

From: SeongJae Park <redacted>

This commit introduces a kernel module named DAMON.  Note that this
commit is implementing only the stub for the module load/unload, basic
data structures, and simple manipulation functions of the structures to
keep the size of commit small.  The core mechanisms of DAMON will be
implemented one by one by following commits.

Interesting piece of work.  I'm reviewing this partly as an exercise in
understanding it, but I'll point out minor stuff on the basis I might
as well whilst I'm here. ;)  Note I review bottom up so some comments
won't make much sense read from the top.

Thanks for review, Jonathan :)  I added reply in line below, but agree to your
whole suggestion.  Will apply those in next spin.

quoted

Brief Introduction
==================

I'd keep this level of intro for the cover letter / docs.  It's not
particularly useful in commit message it git.

Agreed.

quoted

[...]

quoted

 
+config DAMON
+	tristate "Data Access Monitor"
+	depends on MMU
+	default n

No need to specify a default of n.

Got it.

quoted

+	help
+	  Provides data access monitoring.
+
+	  DAMON is a kernel module that allows users to monitor the actual
+	  memory access pattern of specific user-space processes.  It aims to
+	  be 1) accurate enough to be useful for performance-centric domains,
+	  and 2) sufficiently light-weight so that it can be applied online.
+
 endmenu

[...]

quoted

+/*
+ * Construct a damon_region struct
+ *
+ * Returns the pointer to the new struct if success, or NULL otherwise
+ */
+static struct damon_region *damon_new_region(struct damon_ctx *ctx,
+				unsigned long vm_start, unsigned long vm_end)
+{
+	struct damon_region *ret;

I'd give this a different variable name.  Expectation in kernel is often
that ret is simply an magic handle to be passed on.  Don't normally expect
to set elements of it.  I'd go long hand and call it region.

Nice point, will change the name to 'region'.

quoted

+
+	ret = kmalloc(sizeof(struct damon_region), GFP_KERNEL);

sizeof(*ret)

Thanks for catching it!  Will apply to other similar cases.

quoted

+	if (!ret)
+		return NULL;

blank line.

Good suggestion.

quoted

+	ret->vm_start = vm_start;
+	ret->vm_end = vm_end;
+	ret->nr_accesses = 0;
+	ret->sampling_addr = damon_rand(ctx, vm_start, vm_end);
+	INIT_LIST_HEAD(&ret->list);
+
+	return ret;
+}
+
+/*
+ * Add a region between two other regions

Interestingly even the list.h comments for __list_add call this
function "insert".   No idea why it isn't simply called that..

Perhaps damon_insert_region would be clearer and avoid need
for comment?

I just wanted to make the name consistent with the 'list.h' file, but your
suggestion sounds better.  Will change so.

quoted

+ */
+static inline void damon_add_region(struct damon_region *r,
+		struct damon_region *prev, struct damon_region *next)
+{
+	__list_add(&r->list, &prev->list, &next->list);
+}
+
+/*
+ * Append a region to a task's list of regions

I'd argue the naming is sufficient that the comment adds little.

Yes, will delete it.

quoted

+ */
+static void damon_add_region_tail(struct damon_region *r, struct damon_task *t)
+{
+	list_add_tail(&r->list, &t->regions_list);
+}
+
+/*
+ * Delete a region from its list

The list is an implementation detail. I'd not mention that in the comments.

Nice suggestion.

quoted

+ */
+static void damon_del_region(struct damon_region *r)
+{
+	list_del(&r->list);
+}
+
+/*
+ * De-allocate a region

Obvious comment - seem rot risk note below.

Agreed.

quoted

+ */
+static void damon_free_region(struct damon_region *r)
+{
+	kfree(r);
+}
+
+static void damon_destroy_region(struct damon_region *r)
+{
+	damon_del_region(r);
+	damon_free_region(r);
+}
+
+/*
+ * Construct a damon_task struct
+ *
+ * Returns the pointer to the new struct if success, or NULL otherwise
+ */
+static struct damon_task *damon_new_task(unsigned long pid)
+{
+	struct damon_task *t;
+
+	t = kmalloc(sizeof(struct damon_task), GFP_KERNEL);

sizeof(*t) is probably less error prone if this code is maintained
in the long run.

Good point, will apply to other cases, either.

quoted

+	if (!t)
+		return NULL;

blank line.

Will add it.

quoted

+	t->pid = pid;
+	INIT_LIST_HEAD(&t->regions_list);
+
+	return t;
+}
+
+/* Returns n-th damon_region of the given task */
+struct damon_region *damon_nth_region_of(struct damon_task *t, unsigned int n)
+{
+	struct damon_region *r;
+	unsigned int i;
+
+	i = 0;

	unsigned int i = 0;

Yes, it must be much better.

quoted

+	damon_for_each_region(r, t) {
+		if (i++ == n)
+			return r;
+	}

blank line helps readability a little.

Yes, indeed.

quoted

+	return NULL;
+}
+
+static void damon_add_task_tail(struct damon_ctx *ctx, struct damon_task *t)

I'm curious, do we care that it's on the tail?  If not I'd look on that as an
implementation detail and just call this 

damon_add_task()

I named it to be consistent with 'damon_add_region[_tail]()' functions, but as
you suggested renaming 'damon_add_region()', it doesn't need to.  Will change
the name.

quoted

+{
+	list_add_tail(&t->list, &ctx->tasks_list);
+}
+
+static void damon_del_task(struct damon_task *t)
+{
+	list_del(&t->list);
+}
+
+static void damon_free_task(struct damon_task *t)
+{
+	struct damon_region *r, *next;
+
+	damon_for_each_region_safe(r, next, t)
+		damon_free_region(r);
+	kfree(t);
+}
+
+static void damon_destroy_task(struct damon_task *t)
+{
+	damon_del_task(t);
+	damon_free_task(t);
+}
+
+/*
+ * Returns number of monitoring target tasks

As below, kind of obvious so just room for rot.

Agreed.

quoted

+ */
+static unsigned int nr_damon_tasks(struct damon_ctx *ctx)
+{
+	struct damon_task *t;
+	unsigned int ret = 0;
+
+	damon_for_each_task(ctx, t)
+		ret++;
+	return ret;
+}
+
+/*
+ * Returns the number of target regions for a given target task

Always a trade off between useful comments and possibility of docs
rotting.  I'd drop this comment certainly.
The function name is self explanatory.

Agreed!

quoted

+ */
+static unsigned int nr_damon_regions(struct damon_task *t)
+{
+	struct damon_region *r;
+	unsigned int ret = 0;
+
+	damon_for_each_region(r, t)
+		ret++;

Blank line here would help readability a tiny bit.
Same in other places where we have something followed by a nice
simple return statement.

Yes, indeed.

quoted

+	return ret;
+}
+
+static int __init damon_init(void)
+{
+	pr_info("init\n");

Drop these. They are just noise.

Right, it's just noise, will remove.


Thank you again for kind review, Jonathan!


Thanks,
SeongJae Park

quoted

+
+	return 0;
+}
+
+static void __exit damon_exit(void)
+{
+	pr_info("exit\n");
+}
+
+module_init(damon_init);
+module_exit(damon_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("SeongJae Park [off-list ref]");
+MODULE_DESCRIPTION("DAMON: Data Access MONitor");