On Fri, 5 Feb 2021 16:29:41 +0100 Greg KH [off-list ref] wrote:

On Thu, Feb 04, 2021 at 04:31:43PM +0100, SeongJae Park wrote:

quoted

From: SeongJae Park <redacted>

DAMON is designed to be used by kernel space code such as the memory
management subsystems, and therefore it provides only kernel space API.
That said, letting the user space control DAMON could provide some
benefits to them.  For example, it will allow user space to analyze
their specific workloads and make their own special optimizations.

For such cases, this commit implements a simple DAMON application kernel
module, namely 'damon-dbgfs', which merely wraps the DAMON api and
exports those to the user space via the debugfs.

'damon-dbgfs' exports three files, ``attrs``, ``target_ids``, and
``monitor_on`` under its debugfs directory, ``<debugfs>/damon/``.

[...]

quoted

---
 include/linux/damon.h |   3 +
 mm/damon/Kconfig      |   9 +
 mm/damon/Makefile     |   1 +
 mm/damon/core.c       |  47 +++++
 mm/damon/dbgfs.c      | 387 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 447 insertions(+)
 create mode 100644 mm/damon/dbgfs.c

[...]

quoted

diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
new file mode 100644
index 000000000000..db15380737d1
--- /dev/null
+++ b/mm/damon/dbgfs.c

[...]

quoted

+
+static int dbgfs_fill_ctx_dir(struct dentry *dir, struct damon_ctx *ctx)
+{
+	const char * const file_names[] = {"attrs", "target_ids"};
+	const struct file_operations *fops[] = {&attrs_fops, &target_ids_fops};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(file_names); i++) {
+		if (!debugfs_create_file(file_names[i], 0600, dir,
+					ctx, fops[i])) {
+			pr_err("failed to create %s file\n", file_names[i]);
+			return -ENOMEM;

No need to check the return value of this function, just keep going and
ignore it as there's nothing to do and kernel code should not do
different things based on the output of any debugfs calls.

Also, this check is totally wrong and doesn't do what you think it is
doing...

Ok, I will drop the check.

quoted

+static int __init __damon_dbgfs_init(void)
+{
+	struct dentry *dbgfs_root;
+	const char * const file_names[] = {"monitor_on"};
+	const struct file_operations *fops[] = {&monitor_on_fops};
+	int i;
+
+	dbgfs_root = debugfs_create_dir("damon", NULL);
+	if (IS_ERR(dbgfs_root)) {
+		pr_err("failed to create the dbgfs dir\n");
+		return PTR_ERR(dbgfs_root);

Again, no need to check anything, just pass the result of a debugfs call
back into another one just fine.

Ok.

quoted

+	}
+
+	for (i = 0; i < ARRAY_SIZE(file_names); i++) {
+		if (!debugfs_create_file(file_names[i], 0600, dbgfs_root,
+					NULL, fops[i])) {

Again, this isn't checking what you think it is, so please don't do it.

Got it.

I will fix those as you suggested in the next version.


Thanks,
SeongJae Park

thanks,

greg k-h