linux-kernel - Re: [patch 1/5]thp: improve the error code path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1320904609.22361.239.camel@sli10-conroe>
Date:	Thu, 10 Nov 2011 13:56:49 +0800
From:	Shaohua Li <shaohua.li@...el.com>
To:	David Rientjes <rientjes@...gle.com>
Cc:	Andrea Arcangeli <aarcange@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-mm <linux-mm@...ck.org>,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: [patch 1/5]thp: improve the error code path

On Thu, 2011-11-10 at 12:43 +0800, David Rientjes wrote:
> On Thu, 10 Nov 2011, Andrea Arcangeli wrote:
> 
> > Before after won't matter much I guess... If you really want to clean
> > the code, I wonder what is exactly the point of those dummy functions
> > if we can't call those outside of #ifdefs.
> 
> You can, you just need to declare the actuals that you pass to the dummy 
> functions for CONFIG_SYSFS=n as well.  Or, convert the dummy functions to 
> do
> 
> 	#define sysfs_remove_group(kobj, grp) do {} while (0)
> 
> but good luck getting that passed Andrew :)
> 
> > I mean a cleanup that adds
> > more #ifdefs when there are explicit dummy functions which I assume
> > are meant to be used outside of #ifdef CONFIG_SYSFS doesn't sound so
> > clean in the first place. I understand you need to refactor the code
> > above to call those outside of #ifdefs but hey if you're happy with
> > #ifdef I'm happy too :). It just looks fishy to read sysfs.h dummy
> > functions and #ifdefs. When I wrote the code I hardly could have
> > wondered about the sysfs #ifdefs but at this point it's only cleanups
> > I'm seeing so I actually noticed that.
> > 
> 
> The cleaniest solution would probably be to just extract all the calls 
> that depend on CONFIG_SYSFS out of hugepage_init(), call it 
> hugepage_sysfs_init(), and then return a failure code if it fails to setup 
> then do the error handling there.  hugepage_sysfs_init() would be defined 
> right after the attributes are defined.
ok, make the code better.

Improve the error code path. Delete unnecessary sysfs file for example.
Also remove the #ifdef xxx to make code better.

Signed-off-by: Shaohua Li <shaohua.li@...el.com>

---
 mm/huge_memory.c |   63 ++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 17 deletions(-)

Index: linux/mm/huge_memory.c
===================================================================
--- linux.orig/mm/huge_memory.c	2011-11-07 13:52:48.000000000 +0800
+++ linux/mm/huge_memory.c	2011-11-10 13:52:08.000000000 +0800
@@ -487,41 +487,68 @@ static struct attribute_group khugepaged
 	.attrs = khugepaged_attr,
 	.name = "khugepaged",
 };
-#endif /* CONFIG_SYSFS */
 
-static int __init hugepage_init(void)
+static struct kobject *hugepage_kobj;
+static int __init hugepage_init_sysfs(void)
 {
 	int err;
-#ifdef CONFIG_SYSFS
-	static struct kobject *hugepage_kobj;
-#endif
 
-	err = -EINVAL;
-	if (!has_transparent_hugepage()) {
-		transparent_hugepage_flags = 0;
-		goto out;
-	}
-
-#ifdef CONFIG_SYSFS
-	err = -ENOMEM;
 	hugepage_kobj = kobject_create_and_add("transparent_hugepage", mm_kobj);
 	if (unlikely(!hugepage_kobj)) {
 		printk(KERN_ERR "hugepage: failed kobject create\n");
-		goto out;
+		return -ENOMEM;
 	}
 
 	err = sysfs_create_group(hugepage_kobj, &hugepage_attr_group);
 	if (err) {
 		printk(KERN_ERR "hugepage: failed register hugeage group\n");
-		goto out;
+		goto delete_obj;
 	}
 
 	err = sysfs_create_group(hugepage_kobj, &khugepaged_attr_group);
 	if (err) {
 		printk(KERN_ERR "hugepage: failed register hugeage group\n");
-		goto out;
+		goto remove_hp_group;
 	}
-#endif
+
+	return 0;
+
+remove_hp_group:
+	sysfs_remove_group(hugepage_kobj, &hugepage_attr_group);
+delete_obj:
+	kobject_put(hugepage_kobj);
+	return err;
+}
+
+static void __init hugepage_exit_sysfs(void)
+{
+	sysfs_remove_group(hugepage_kobj, &khugepaged_attr_group);
+	sysfs_remove_group(hugepage_kobj, &hugepage_attr_group);
+	kobject_put(hugepage_kobj);
+}
+#else
+static inline int hugepage_init_sysfs(void)
+{
+	return 0;
+}
+
+static inline void hugepage_exit_sysfs(void)
+{
+}
+#endif /* CONFIG_SYSFS */
+
+static int __init hugepage_init(void)
+{
+	int err;
+
+	if (!has_transparent_hugepage()) {
+		transparent_hugepage_flags = 0;
+		return -EINVAL;
+	}
+
+	err = hugepage_init_sysfs();
+	if (err)
+		return err;
 
 	err = khugepaged_slab_init();
 	if (err)
@@ -545,7 +572,9 @@ static int __init hugepage_init(void)
 
 	set_recommended_min_free_kbytes();
 
+	return 0;
 out:
+	hugepage_exit_sysfs();
 	return err;
 }
 module_init(hugepage_init)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/