Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: Isl8z6UY0KpLTxS6jF5n1NSDuiRTpqJiDuBKR7FLfaxRKrl5b2afXK6vOVniDTuhyKJ6ahvX9v
 y24PXynPyJGA==
From:   SeongJae Park <sjpark@amazon.com>
To:     SeongJae Park <sjpark@amazon.com>
CC:     <akpm@linux-foundation.org>, <Jonathan.Cameron@Huawei.com>,
        <aarcange@redhat.com>, <acme@kernel.org>,
        <alexander.shishkin@linux.intel.com>, <amit@kernel.org>,
        <benh@kernel.crashing.org>, <brendan.d.gregg@gmail.com>,
        <brendanhiggins@google.com>, <cai@lca.pw>,
        <colin.king@canonical.com>, <corbet@lwn.net>, <david@redhat.com>,
        <dwmw@amazon.com>, <fan.du@intel.com>, <foersleo@amazon.de>,
        <gthelen@google.com>, <irogers@google.com>, <jolsa@redhat.com>,
        <kirill@shutemov.name>, <mark.rutland@arm.com>, <mgorman@suse.de>,
        <minchan@kernel.org>, <mingo@redhat.com>, <namhyung@kernel.org>,
        <peterz@infradead.org>, <rdunlap@infradead.org>,
        <riel@surriel.com>, <rientjes@google.com>, <rostedt@goodmis.org>,
        <rppt@kernel.org>, <sblbir@amazon.com>, <shakeelb@google.com>,
        <shuah@kernel.org>, <sj38.park@gmail.com>, <snu@amazon.de>,
        <vbabka@suse.cz>, <vdavydov.dev@gmail.com>,
        <yang.shi@linux.alibaba.com>, <ying.huang@intel.com>,
        <linux-damon@amazon.com>, <linux-mm@kvack.org>,
        <linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v19 09/15] mm/damon: Implement a debugfs interface
Date:   Mon, 10 Aug 2020 09:41:43 +0200
Message-ID: <20200810074143.26863-1-sjpark@amazon.com>
In-Reply-To: <20200804091416.31039-10-sjpark@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 4 Aug 2020 11:14:10 +0200 SeongJae Park <sjpark@amazon.com> wrote:

> From: SeongJae Park <sjpark@amazon.de>
> 
> This commit implements a debugfs interface for DAMON.  It works for the
> virtual address spaces monitoring.
[...]
> +
> +#define targetid_is_pid(ctx)	\
> +	(ctx->target_valid == kdamond_vm_target_valid)
> +
[...]
> +
> +static ssize_t debugfs_target_ids_write(struct file *file,
> +		const char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct damon_ctx *ctx = &damon_user_ctx;
> +	char *kbuf;
> +	unsigned long *targets;
> +	ssize_t nr_targets;
> +	ssize_t ret = count;
> +	struct damon_target *target;
> +	int i;
> +	int err;
> +
> +	kbuf = user_input_str(buf, count, ppos);
> +	if (IS_ERR(kbuf))
> +		return PTR_ERR(kbuf);
> +
> +	targets = str_to_target_ids(kbuf, ret, &nr_targets);
> +	if (!targets) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	if (targetid_is_pid(ctx)) {
> +		for (i = 0; i < nr_targets; i++)
> +			targets[i] = (unsigned long)find_get_pid(
> +					(int)targets[i]);
> +	}
> +
> +	mutex_lock(&ctx->kdamond_lock);
> +	if (ctx->kdamond) {
> +		ret = -EINVAL;
> +		goto unlock_out;
> +	}
> +
> +	if (targetid_is_pid(ctx)) {
> +		damon_for_each_target(target, ctx)
> +			put_pid((struct pid *)target->id);

If non-pid target ids were set before by the kernel API, this will cause a
problem.  Therefore, the DAMON users should cleanup there target ids properly.
However, I found that this could be easily missed.  Indeed, my new test code
missed the cleanup.  Moreover, it would be hard to do that when concurrent
DAMON users exist.

One straightforward fix would be making 'damon_set_targets()' to remember last
target id type and do 'put_pid()' if the last target id type was pid, instead
of here.  This will work, but make the address space independent part to be
coupled with the dependent part.

Or, we could add another callback for cleanup and let debugfs code to register
a function doing 'put_pid()' and remove of the targets as the callback.  This
approach will allow the address space independent part to be remain
independent.

I will fix this problem with the second approach in the next spin.


Thanks,
SeongJae Park