[patch 1/5]thp: improve the error code path

[patch 1/5]thp: improve the error code path

[Shaohua Li]

Improve the error code path. Delete unnecessary sysfs file for example.

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

---
 mm/huge_memory.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Index: linux/mm/huge_memory.c
===================================================================
--- linux.orig/mm/huge_memory.c	2011-10-19 14:07:27.000000000 +0800
+++ linux/mm/huge_memory.c	2011-10-24 19:24:31.000000000 +0800
@@ -512,25 +512,23 @@ static int __init hugepage_init(void)
 	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
 
 	err = khugepaged_slab_init();
 	if (err)
-		goto out;
+		goto remove_khp_group;
 
 	err = mm_slots_hash_init();
-	if (err) {
-		khugepaged_slab_free();
-		goto out;
-	}
+	if (err)
+		goto free_slab;
 
 	/*
 	 * By default disable transparent hugepages on smaller systems,
@@ -544,7 +542,18 @@ static int __init hugepage_init(void)
 
 	set_recommended_min_free_kbytes();
 
+	return err;
+free_slab:
+	khugepaged_slab_free();
+remove_khp_group:
+#ifdef CONFIG_SYSFS
+	sysfs_remove_group(hugepage_kobj, &khugepaged_attr_group);
+remove_hp_group:
+	sysfs_remove_group(hugepage_kobj, &hugepage_attr_group);
+delete_obj:
+	kobject_put(hugepage_kobj);
 out:
+#endif
 	return err;
 }
 module_init(hugepage_init)


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrea Arcangeli]

Hello,

On Tue, Oct 25, 2011 at 10:58:41AM +0800, Shaohua Li wrote:
> +#ifdef CONFIG_SYSFS
> +	sysfs_remove_group(hugepage_kobj, &khugepaged_attr_group);
> +remove_hp_group:
> +	sysfs_remove_group(hugepage_kobj, &hugepage_attr_group);
> +delete_obj:
> +	kobject_put(hugepage_kobj);
>  out:
> +#endif

Adding an ifdef is making the code worse, the whole point of having
these functions become noops at build time is to avoid having to add
ifdefs in the callers.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Shaohua Li]

On Tue, 2011-10-25 at 19:44 +0800, Andrea Arcangeli wrote:
> Hello,
> 
> On Tue, Oct 25, 2011 at 10:58:41AM +0800, Shaohua Li wrote:
> > +#ifdef CONFIG_SYSFS
> > +	sysfs_remove_group(hugepage_kobj, &khugepaged_attr_group);
> > +remove_hp_group:
> > +	sysfs_remove_group(hugepage_kobj, &hugepage_attr_group);
> > +delete_obj:
> > +	kobject_put(hugepage_kobj);
> >  out:
> > +#endif
> 
> Adding an ifdef is making the code worse, the whole point of having
> these functions become noops at build time is to avoid having to add
> ifdefs in the callers.
yes, but hugepage_attr_group is defined in CONFIG_SYSFS. And the
functions are inline functions. They really should be a '#define xxx'.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Shaohua Li]

On Wed, 2011-10-26 at 09:48 +0800, Shaohua Li wrote:
> On Tue, 2011-10-25 at 19:44 +0800, Andrea Arcangeli wrote:
> > Hello,
> > 
> > On Tue, Oct 25, 2011 at 10:58:41AM +0800, Shaohua Li wrote:
> > > +#ifdef CONFIG_SYSFS
> > > +	sysfs_remove_group(hugepage_kobj, &khugepaged_attr_group);
> > > +remove_hp_group:
> > > +	sysfs_remove_group(hugepage_kobj, &hugepage_attr_group);
> > > +delete_obj:
> > > +	kobject_put(hugepage_kobj);
> > >  out:
> > > +#endif
> > 
> > Adding an ifdef is making the code worse, the whole point of having
> > these functions become noops at build time is to avoid having to add
> > ifdefs in the callers.
> yes, but hugepage_attr_group is defined in CONFIG_SYSFS. And the
> functions are inline functions. They really should be a '#define xxx'.
ping, any comments for the 5 patches?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrea Arcangeli]

Hi Shaohua,

On Mon, Nov 07, 2011 at 01:17:29PM +0800, Shaohua Li wrote:
> On Wed, 2011-10-26 at 09:48 +0800, Shaohua Li wrote:
> > On Tue, 2011-10-25 at 19:44 +0800, Andrea Arcangeli wrote:
> > > Hello,
> > > 
> > > On Tue, Oct 25, 2011 at 10:58:41AM +0800, Shaohua Li wrote:
> > > > +#ifdef CONFIG_SYSFS
> > > > +	sysfs_remove_group(hugepage_kobj, &khugepaged_attr_group);
> > > > +remove_hp_group:
> > > > +	sysfs_remove_group(hugepage_kobj, &hugepage_attr_group);
> > > > +delete_obj:
> > > > +	kobject_put(hugepage_kobj);
> > > >  out:
> > > > +#endif
> > > 
> > > Adding an ifdef is making the code worse, the whole point of having
> > > these functions become noops at build time is to avoid having to add
> > > ifdefs in the callers.
> > yes, but hugepage_attr_group is defined in CONFIG_SYSFS. And the
> > functions are inline functions. They really should be a '#define xxx'.

hugepage_attr_group is defined even if CONFIG_SYSFS is not set and I
just made a build with CONFIG_SYSFS=n and it builds just fine without
any change.

$ grep CONFIG_SYSFS .config
# CONFIG_SYSFS is not set

So we can drop 1/5 above.

> ping, any comments for the 5 patches?

Apologies for the delay in the answer! I had a few other open items
and the plenty of emails on 5/5 required a bit more time to think
about :). Expect a reply on the other 4 soon.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Shaohua Li]

On Thu, 2011-11-10 at 10:18 +0800, Andrea Arcangeli wrote:
> Hi Shaohua,
> 
> On Mon, Nov 07, 2011 at 01:17:29PM +0800, Shaohua Li wrote:
> > On Wed, 2011-10-26 at 09:48 +0800, Shaohua Li wrote:
> > > On Tue, 2011-10-25 at 19:44 +0800, Andrea Arcangeli wrote:
> > > > Hello,
> > > > 
> > > > On Tue, Oct 25, 2011 at 10:58:41AM +0800, Shaohua Li wrote:
> > > > > +#ifdef CONFIG_SYSFS
> > > > > +	sysfs_remove_group(hugepage_kobj, &khugepaged_attr_group);
> > > > > +remove_hp_group:
> > > > > +	sysfs_remove_group(hugepage_kobj, &hugepage_attr_group);
> > > > > +delete_obj:
> > > > > +	kobject_put(hugepage_kobj);
> > > > >  out:
> > > > > +#endif
> > > > 
> > > > Adding an ifdef is making the code worse, the whole point of having
> > > > these functions become noops at build time is to avoid having to add
> > > > ifdefs in the callers.
> > > yes, but hugepage_attr_group is defined in CONFIG_SYSFS. And the
> > > functions are inline functions. They really should be a '#define xxx'.
> 
> hugepage_attr_group is defined even if CONFIG_SYSFS is not set and I
> just made a build with CONFIG_SYSFS=n and it builds just fine without
> any change.

> $ grep CONFIG_SYSFS .config
> # CONFIG_SYSFS is not set
> 
> So we can drop 1/5 above.
this isn't the case in the code. And the code uses hugepage_attr_group
is already within CONFIG_SYSFS, so your build success.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

On Thu, 10 Nov 2011, Shaohua Li wrote:

> > hugepage_attr_group is defined even if CONFIG_SYSFS is not set and I
> > just made a build with CONFIG_SYSFS=n and it builds just fine without
> > any change.
> 
> > $ grep CONFIG_SYSFS .config
> > # CONFIG_SYSFS is not set
> > 
> > So we can drop 1/5 above.
> this isn't the case in the code. And the code uses hugepage_attr_group
> is already within CONFIG_SYSFS, so your build success.
> 

You're right, but I agree that the #ifdef's just make the function error 
handling much too complex.  Would you mind adding sysfs_*_out labels at 
the end of the function to handle these errors instead?  And I think we 
should be doing khugepaged_slab_init() and mm_slots_hash_init() before 
initializing sysfs.

Something like

	out:
		khugepaged_slab_free();
		mm_slots_hash_free();	<-- after you remove it from #if 0
		return err;

	#ifdef CONFIG_SYSFS
	sysfs_khugepaged_out:
		sysfs_remove_group(hugepage_kobj, &khugepaged_attr_group);
	sysfs_hugepage_out:
		sysfs_remove_group(hugepage_kobj, &hugepage_attr_group);
		...
		goto out;
	#endif

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrea Arcangeli]

On Wed, Nov 09, 2011 at 06:43:58PM -0800, David Rientjes wrote:
> You're right, but I agree that the #ifdef's just make the function error 
> handling much too complex.  Would you mind adding sysfs_*_out labels at 
> the end of the function to handle these errors instead?  And I think we 
> should be doing khugepaged_slab_init() and mm_slots_hash_init() before 
> initializing sysfs.
> 
> Something like
> 
> 	out:
> 		khugepaged_slab_free();
> 		mm_slots_hash_free();	<-- after you remove it from #if 0
> 		return err;
> 
> 	#ifdef CONFIG_SYSFS
> 	sysfs_khugepaged_out:
> 		sysfs_remove_group(hugepage_kobj, &khugepaged_attr_group);
> 	sysfs_hugepage_out:
> 		sysfs_remove_group(hugepage_kobj, &hugepage_attr_group);
> 		...
> 		goto out;
> 	#endif

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. 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Shaohua Li]

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@intel.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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

On Thu, 10 Nov 2011, Shaohua Li wrote:

> 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);
> +}

As mentioned previously, if khugepaged_slab_init() and 
mm_slots_hash_init() is done first before sysfs is initialized, then you 
shouldn't need hugepage_exit_sysfs() at all; all its error handling should 
be localized to hugepage_init().

> +#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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Shaohua Li]

On Thu, 2011-11-10 at 14:08 +0800, David Rientjes wrote:
> On Thu, 10 Nov 2011, Shaohua Li wrote:
> 
> > 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);
> > +}
> 
> As mentioned previously, if khugepaged_slab_init() and 
> mm_slots_hash_init() is done first before sysfs is initialized, then you 
> shouldn't need hugepage_exit_sysfs() at all; all its error handling should 
> be localized to hugepage_init().
then we just move some error handling code to hugepage_init(). It
doesn't mean this one is better or that one is better, but I really see
no point to refresh the patch again.

Thanks,
Shaohua

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrea Arcangeli]

On Thu, Nov 10, 2011 at 01:56:49PM +0800, Shaohua Li wrote:
> +static struct kobject *hugepage_kobj;

It's minor nitpick but we don't need to put this in .bss, passing it
as hugepage_init_sysfs(struct kobject **hugepage_kobj) and storing it
there in case the error returned by hugepage_init_sysfs is zero should
be fine. The feature cannot be unloaded so we can lose the pointer
after init is complete.

Then I think we can be done with this... and it looks better now.

> +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;

  	struct kobject *hugepage_kobj;

> +
> +	if (!has_transparent_hugepage()) {
> +		transparent_hugepage_flags = 0;
> +		return -EINVAL;
> +	}
> +
> +	err = hugepage_init_sysfs();

	err = hugepage_init_sysfs(&hugepage_kobj);


> +	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();

	hugepage_exit_sysfs(hugepage_kobj);

>  	return err;
>  }
>  module_init(hugepage_init)
> 
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Shaohua Li]

On Thu, 2011-11-10 at 22:14 +0800, Andrea Arcangeli wrote:
> On Thu, Nov 10, 2011 at 01:56:49PM +0800, Shaohua Li wrote:
> > +static struct kobject *hugepage_kobj;
> 
> It's minor nitpick but we don't need to put this in .bss, passing it
> as hugepage_init_sysfs(struct kobject **hugepage_kobj) and storing it
> there in case the error returned by hugepage_init_sysfs is zero should
> be fine. The feature cannot be unloaded so we can lose the pointer
> after init is complete.
> 
> Then I think we can be done with this... and it looks better now.
all right. here it is.

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@intel.com>

---
 mm/huge_memory.c |   71 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 21 deletions(-)

Index: linux/mm/huge_memory.c
===================================================================
--- linux.orig/mm/huge_memory.c	2011-11-10 15:51:03.000000000 +0800
+++ linux/mm/huge_memory.c	2011-11-11 13:32:44.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 int __init hugepage_init_sysfs(struct kobject **hugepage_kobj)
 {
 	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)) {
+	*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);
+	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);
+	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(struct kobject *hugepage_kobj)
+{
+	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(struct kobject **hugepage_kobj)
+{
+	return 0;
+}
+
+static inline void hugepage_exit_sysfs(struct kobject *hugepage_kobj)
+{
+}
+#endif /* CONFIG_SYSFS */
+
+static int __init hugepage_init(void)
+{
+	int err;
+	struct kobject *hugepage_kobj;
+
+	if (!has_transparent_hugepage()) {
+		transparent_hugepage_flags = 0;
+		return -EINVAL;
+	}
+
+	err = hugepage_init_sysfs(&hugepage_kobj);
+	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(hugepage_kobj);
 	return err;
 }
 module_init(hugepage_init)


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrea Arcangeli]

On Fri, Nov 11, 2011 at 02:33:37PM +0800, Shaohua Li wrote:
> 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@intel.com>
> 
> ---
>  mm/huge_memory.c |   71 ++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 50 insertions(+), 21 deletions(-)

Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrea Arcangeli]

On Thu, Nov 10, 2011 at 10:33:15AM +0800, Shaohua Li wrote:
> On Thu, 2011-11-10 at 10:18 +0800, Andrea Arcangeli wrote:
> > Hi Shaohua,
> > 
> > On Mon, Nov 07, 2011 at 01:17:29PM +0800, Shaohua Li wrote:
> > > On Wed, 2011-10-26 at 09:48 +0800, Shaohua Li wrote:
> > > > On Tue, 2011-10-25 at 19:44 +0800, Andrea Arcangeli wrote:
> > > > > Hello,
> > > > > 
> > > > > On Tue, Oct 25, 2011 at 10:58:41AM +0800, Shaohua Li wrote:
> > > > > > +#ifdef CONFIG_SYSFS
> > > > > > +	sysfs_remove_group(hugepage_kobj, &khugepaged_attr_group);
> > > > > > +remove_hp_group:
> > > > > > +	sysfs_remove_group(hugepage_kobj, &hugepage_attr_group);
> > > > > > +delete_obj:
> > > > > > +	kobject_put(hugepage_kobj);
> > > > > >  out:
> > > > > > +#endif
> > > > > 
> > > > > Adding an ifdef is making the code worse, the whole point of having
> > > > > these functions become noops at build time is to avoid having to add
> > > > > ifdefs in the callers.
> > > > yes, but hugepage_attr_group is defined in CONFIG_SYSFS. And the
> > > > functions are inline functions. They really should be a '#define xxx'.
> > 
> > hugepage_attr_group is defined even if CONFIG_SYSFS is not set and I
> > just made a build with CONFIG_SYSFS=n and it builds just fine without
> > any change.
> 
> > $ grep CONFIG_SYSFS .config
> > # CONFIG_SYSFS is not set
> > 
> > So we can drop 1/5 above.
> this isn't the case in the code. And the code uses hugepage_attr_group
> is already within CONFIG_SYSFS, so your build success.

I thought it was related to a SYSFS=n build failure, so when I
verified it build just fine I didn't really see the point of messing
with sysfs adding more #ifdefs but I see you want to avoid leaking
those two entries if the allocation doesn't succeed.

If the later allocations don't succeed we can panic() and be done with
it. Adding more code will just grow the zImage a bit, but I can
appreciate the more theoretical correctness so I'm ok with it now that
I see the point of it.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>