[PATCH] xen/tmem: cleanup

[PATCH] xen/tmem: cleanup

[Jan Beulich]

Use 'bool' for boolean variables. Do proper section placement.
Eliminate an unnecessary export.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Dan Magenheimer <dan.magenheimer@oracle.com>

---
 drivers/xen/tmem.c |   21 ++++++++-------------
 include/xen/tmem.h |    6 +++++-
 2 files changed, 13 insertions(+), 14 deletions(-)

--- 3.3-rc2/drivers/xen/tmem.c
+++ 3.3-rc2-xen-tmem-cleanup/drivers/xen/tmem.c
@@ -9,7 +9,6 @@
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/pagemap.h>
-#include <linux/module.h>
 #include <linux/cleancache.h>
 
 /* temporary ifdef until include/linux/frontswap.h is upstream */
@@ -128,15 +127,13 @@ static int xen_tmem_flush_object(u32 poo
 	return xen_tmem_op(TMEM_FLUSH_OBJECT, pool_id, oid, 0, 0, 0, 0, 0);
 }
 
-int tmem_enabled __read_mostly;
-EXPORT_SYMBOL(tmem_enabled);
+bool __read_mostly tmem_enabled = false;
 
 static int __init enable_tmem(char *s)
 {
-	tmem_enabled = 1;
+	tmem_enabled = true;
 	return 1;
 }
-
 __setup("tmem", enable_tmem);
 
 #ifdef CONFIG_CLEANCACHE
@@ -229,17 +226,16 @@ static int tmem_cleancache_init_shared_f
 	return xen_tmem_new_pool(shared_uuid, TMEM_POOL_SHARED, pagesize);
 }
 
-static int use_cleancache = 1;
+static bool __initdata use_cleancache = true;
 
 static int __init no_cleancache(char *s)
 {
-	use_cleancache = 0;
+	use_cleancache = false;
 	return 1;
 }
-
 __setup("nocleancache", no_cleancache);
 
-static struct cleancache_ops tmem_cleancache_ops = {
+static struct cleancache_ops __initdata tmem_cleancache_ops = {
 	.put_page = tmem_cleancache_put_page,
 	.get_page = tmem_cleancache_get_page,
 	.flush_page = tmem_cleancache_flush_page,
@@ -356,17 +352,16 @@ static void tmem_frontswap_init(unsigned
 		    xen_tmem_new_pool(private, TMEM_POOL_PERSIST, PAGE_SIZE);
 }
 
-static int __initdata use_frontswap = 1;
+static bool __initdata use_frontswap = true;
 
 static int __init no_frontswap(char *s)
 {
-	use_frontswap = 0;
+	use_frontswap = false;
 	return 1;
 }
-
 __setup("nofrontswap", no_frontswap);
 
-static struct frontswap_ops tmem_frontswap_ops = {
+static struct frontswap_ops __initdata tmem_frontswap_ops = {
 	.put_page = tmem_frontswap_put_page,
 	.get_page = tmem_frontswap_get_page,
 	.flush_page = tmem_frontswap_flush_page,
--- 3.3-rc2/include/xen/tmem.h
+++ 3.3-rc2-xen-tmem-cleanup/include/xen/tmem.h
@@ -1,5 +1,9 @@
 #ifndef _XEN_TMEM_H
 #define _XEN_TMEM_H
+
+#include <linux/types.h>
+
 /* defined in drivers/xen/tmem.c */
-extern int tmem_enabled;
+extern bool tmem_enabled;
+
 #endif /* _XEN_TMEM_H */

RE: [PATCH] xen/tmem: cleanup

[Dan Magenheimer]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Subject: [PATCH] xen/tmem: cleanup
> 
> Use 'bool' for boolean variables. Do proper section placement.
> Eliminate an unnecessary export.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
> 
> -int tmem_enabled __read_mostly;
> -EXPORT_SYMBOL(tmem_enabled);
> +bool __read_mostly tmem_enabled = false;

tmem_enabled is used in xen/drivers/xen-selfballoon.c

RE: [PATCH] xen/tmem: cleanup

[Jan Beulich]

>>> On 04.02.12 at 17:42, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Subject: [PATCH] xen/tmem: cleanup
>> 
>> Use 'bool' for boolean variables. Do proper section placement.
>> Eliminate an unnecessary export.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
>> 
>> -int tmem_enabled __read_mostly;
>> -EXPORT_SYMBOL(tmem_enabled);
>> +bool __read_mostly tmem_enabled = false;
> 
> tmem_enabled is used in xen/drivers/xen-selfballoon.c

Which can't be built as a module, and hence the symbol doesn't need
exporting. This patch (of course, I'm tempted to say) survived build
testing.

Jan

RE: [PATCH] xen/tmem: cleanup

[Dan Magenheimer]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, February 06, 2012 1:13 AM
> To: Dan Magenheimer
> Cc: Jeremy Fitzhardinge; xen-devel@lists.xensource.com; Konrad Wilk; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] xen/tmem: cleanup
> 
> >>> On 04.02.12 at 17:42, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Subject: [PATCH] xen/tmem: cleanup
> >>
> >> Use 'bool' for boolean variables. Do proper section placement.
> >> Eliminate an unnecessary export.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
> >>
> >> -int tmem_enabled __read_mostly;
> >> -EXPORT_SYMBOL(tmem_enabled);
> >> +bool __read_mostly tmem_enabled = false;
> >
> > tmem_enabled is used in xen/drivers/xen-selfballoon.c
> 
> Which can't be built as a module, and hence the symbol doesn't need
> exporting. This patch (of course, I'm tempted to say) survived build
> testing.

Yes, correct.  BUT... I think only the reason xen-selfballoon.c
can't be built as a module is because the MM variable vm_committed_as
(or an access function) is not exported.  Ideally xen-selfballoon.c
probably should be a module but putting a core MM change in
the critical path of a Xen-only-related enhancement seemed
a recipe for sure failure.

Konrad, if you (1) disagree entirely, or (2) want to remove the
tmem_enabled EXPORT_SYMBOL now and add it back later if/when
the core MM change happens, I'll leave that up to you.

If (2), the MM change should be added to the minor-tmem-related-
changes-that-need-to-be-eventually-upstreamed list ;-)

Dan

RE: [PATCH] xen/tmem: cleanup

[Jan Beulich]

>>> On 06.02.12 at 17:37, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, February 06, 2012 1:13 AM
>> To: Dan Magenheimer
>> Cc: Jeremy Fitzhardinge; xen-devel@lists.xensource.com; Konrad Wilk; 
> linux-kernel@vger.kernel.org 
>> Subject: RE: [PATCH] xen/tmem: cleanup
>> 
>> >>> On 04.02.12 at 17:42, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
>> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Subject: [PATCH] xen/tmem: cleanup
>> >>
>> >> Use 'bool' for boolean variables. Do proper section placement.
>> >> Eliminate an unnecessary export.
>> >>
>> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> >> Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
>> >>
>> >> -int tmem_enabled __read_mostly;
>> >> -EXPORT_SYMBOL(tmem_enabled);
>> >> +bool __read_mostly tmem_enabled = false;
>> >
>> > tmem_enabled is used in xen/drivers/xen-selfballoon.c
>> 
>> Which can't be built as a module, and hence the symbol doesn't need
>> exporting. This patch (of course, I'm tempted to say) survived build
>> testing.
> 
> Yes, correct.  BUT... I think only the reason xen-selfballoon.c
> can't be built as a module is because the MM variable vm_committed_as
> (or an access function) is not exported.  Ideally xen-selfballoon.c
> probably should be a module but putting a core MM change in
> the critical path of a Xen-only-related enhancement seemed
> a recipe for sure failure.

No, this isn't the main reason (as you say further down, this could
easily be adjusted for) afaict. The main reason is that
register_xen_selfballooning() is being called from non-modular
code (xen-balloon.c), which could be made a module too, but in
turn has at least one reference that wouldn't be nice to become
an export (balloon_set_new_target()).

Jan

> Konrad, if you (1) disagree entirely, or (2) want to remove the
> tmem_enabled EXPORT_SYMBOL now and add it back later if/when
> the core MM change happens, I'll leave that up to you.
> 
> If (2), the MM change should be added to the minor-tmem-related-
> changes-that-need-to-be-eventually-upstreamed list ;-)
> 
> Dan

Re: [PATCH] xen/tmem: cleanup

[Konrad Rzeszutek Wilk]

On Mon, Feb 06, 2012 at 04:49:42PM +0000, Jan Beulich wrote:
> >>> On 06.02.12 at 17:37, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Monday, February 06, 2012 1:13 AM
> >> To: Dan Magenheimer
> >> Cc: Jeremy Fitzhardinge; xen-devel@lists.xensource.com; Konrad Wilk; 
> > linux-kernel@vger.kernel.org 
> >> Subject: RE: [PATCH] xen/tmem: cleanup
> >> 
> >> >>> On 04.02.12 at 17:42, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> >> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> Subject: [PATCH] xen/tmem: cleanup
> >> >>
> >> >> Use 'bool' for boolean variables. Do proper section placement.
> >> >> Eliminate an unnecessary export.
> >> >>
> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> >> Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
> >> >>
> >> >> -int tmem_enabled __read_mostly;
> >> >> -EXPORT_SYMBOL(tmem_enabled);
> >> >> +bool __read_mostly tmem_enabled = false;
> >> >
> >> > tmem_enabled is used in xen/drivers/xen-selfballoon.c
> >> 
> >> Which can't be built as a module, and hence the symbol doesn't need
> >> exporting. This patch (of course, I'm tempted to say) survived build
> >> testing.
> > 
> > Yes, correct.  BUT... I think only the reason xen-selfballoon.c
> > can't be built as a module is because the MM variable vm_committed_as
> > (or an access function) is not exported.  Ideally xen-selfballoon.c
> > probably should be a module but putting a core MM change in
> > the critical path of a Xen-only-related enhancement seemed
> > a recipe for sure failure.
> 
> No, this isn't the main reason (as you say further down, this could
> easily be adjusted for) afaict. The main reason is that
> register_xen_selfballooning() is being called from non-modular
> code (xen-balloon.c), which could be made a module too, but in
> turn has at least one reference that wouldn't be nice to become
> an export (balloon_set_new_target()).

It would be nice if all of it could become modules. That way HVM
device driver domains could load the whole thing without having much
built-in code in the kernel.

Is it possible to do that?
> 
> Jan
> 
> > Konrad, if you (1) disagree entirely, or (2) want to remove the
> > tmem_enabled EXPORT_SYMBOL now and add it back later if/when
> > the core MM change happens, I'll leave that up to you.
> > 
> > If (2), the MM change should be added to the minor-tmem-related-
> > changes-that-need-to-be-eventually-upstreamed list ;-)
> > 
> > Dan
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] xen/tmem: cleanup

[Dan Magenheimer]

> From: Konrad Rzeszutek Wilk
> Subject: Re: [PATCH] xen/tmem: cleanup
> 
> On Mon, Feb 06, 2012 at 04:49:42PM +0000, Jan Beulich wrote:
> > >>> On 06.02.12 at 17:37, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> > >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> > >> Sent: Monday, February 06, 2012 1:13 AM
> > >> To: Dan Magenheimer
> > >> Cc: Jeremy Fitzhardinge; xen-devel@lists.xensource.com; Konrad Wilk;
> > > linux-kernel@vger.kernel.org
> > >> Subject: RE: [PATCH] xen/tmem: cleanup
> > >>
> > >> >>> On 04.02.12 at 17:42, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> > >> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> > >> >> Subject: [PATCH] xen/tmem: cleanup
> > >> >>
> > >> >> Use 'bool' for boolean variables. Do proper section placement.
> > >> >> Eliminate an unnecessary export.
> > >> >>
> > >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > >> >> Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
> > >> >>
> > >> >> -int tmem_enabled __read_mostly;
> > >> >> -EXPORT_SYMBOL(tmem_enabled);
> > >> >> +bool __read_mostly tmem_enabled = false;
> > >> >
> > >> > tmem_enabled is used in xen/drivers/xen-selfballoon.c
> > >>
> > >> Which can't be built as a module, and hence the symbol doesn't need
> > >> exporting. This patch (of course, I'm tempted to say) survived build
> > >> testing.
> > >
> > > Yes, correct.  BUT... I think only the reason xen-selfballoon.c
> > > can't be built as a module is because the MM variable vm_committed_as
> > > (or an access function) is not exported.  Ideally xen-selfballoon.c
> > > probably should be a module but putting a core MM change in
> > > the critical path of a Xen-only-related enhancement seemed
> > > a recipe for sure failure.
> >
> > No, this isn't the main reason (as you say further down, this could
> > easily be adjusted for) afaict. The main reason is that
> > register_xen_selfballooning() is being called from non-modular
> > code (xen-balloon.c), which could be made a module too, but in
> > turn has at least one reference that wouldn't be nice to become
> > an export (balloon_set_new_target()).

Jan, you're right, I had forgotten about that "chained" dependency.
Thanks and FWIW, on the entire original patch:

Acked-by: Dan Magenheimer <dan.magenheimer@oracle.com>
 
> It would be nice if all of it could become modules. That way HVM
> device driver domains could load the whole thing without having much
> built-in code in the kernel.
> 
> Is it possible to do that?

Konrad, my module expertise is very low, so I will leave that
to Jan or someone else to answer or look into.

Dan

Re: [PATCH] xen/tmem: cleanup

[Jan Beulich]

>>> On 06.02.12 at 18:02, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Feb 06, 2012 at 04:49:42PM +0000, Jan Beulich wrote:
>> >>> On 06.02.12 at 17:37, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
>> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> > tmem_enabled is used in xen/drivers/xen-selfballoon.c
>> >> 
>> >> Which can't be built as a module, and hence the symbol doesn't need
>> >> exporting. This patch (of course, I'm tempted to say) survived build
>> >> testing.
>> > 
>> > Yes, correct.  BUT... I think only the reason xen-selfballoon.c
>> > can't be built as a module is because the MM variable vm_committed_as
>> > (or an access function) is not exported.  Ideally xen-selfballoon.c
>> > probably should be a module but putting a core MM change in
>> > the critical path of a Xen-only-related enhancement seemed
>> > a recipe for sure failure.
>> 
>> No, this isn't the main reason (as you say further down, this could
>> easily be adjusted for) afaict. The main reason is that
>> register_xen_selfballooning() is being called from non-modular
>> code (xen-balloon.c), which could be made a module too, but in
>> turn has at least one reference that wouldn't be nice to become
>> an export (balloon_set_new_target()).
> 
> It would be nice if all of it could become modules. That way HVM
> device driver domains could load the whole thing without having much
> built-in code in the kernel.
> 
> Is it possible to do that?

As outlined above, it is possible, but I'm not certain it is a good idea
to have balloon_set_new_target() exported. If you think that's
acceptable, I can certainly put together a patch doing that (on top
of the one here, and probably not immediately).

Jan

Re: [Xen-devel] [PATCH] xen/tmem: cleanup

[Vasiliy Tolstov]

2012/2/7 Jan Beulich <JBeulich@suse.com>:

>
> As outlined above, it is possible, but I'm not certain it is a good idea
> to have balloon_set_new_target() exported. If you think that's
> acceptable, I can certainly put together a patch doing that (on top
> of the one here, and probably not immediately).
>
> Jan

Exporting balloon_set_new_target can be grate feature to do own
modules for balloon memory inside xen guest, becouse some times we
need more realtime control from dom0 for memory growing speed inside
guest.


-- 
Vasiliy Tolstov,
Clodo.ru
e-mail: v.tolstov@selfip.ru
jabber: vase@selfip.ru

Re: [Xen-devel] [PATCH] xen/tmem: cleanup

[Konrad Rzeszutek Wilk]

> Jan, you're right, I had forgotten about that "chained" dependency.
> Thanks and FWIW, on the entire original patch:
> 
> Acked-by: Dan Magenheimer <dan.magenheimer@oracle.com>
>  

ok, applied to the tree.
> > It would be nice if all of it could become modules. That way HVM
> > device driver domains could load the whole thing without having much
> > built-in code in the kernel.
> > 
> > Is it possible to do that?
> 
> Konrad, my module expertise is very low, so I will leave that
> to Jan or someone else to answer or look into.

Sounds feasible, but would need to export some symbols. Hmm. Not sure.

> 
> Dan
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [Xen-devel] [PATCH] xen/tmem: cleanup

[Konrad Rzeszutek Wilk]

On Tue, Feb 07, 2012 at 02:23:05PM +0400, Vasiliy Tolstov wrote:
> 2012/2/7 Jan Beulich <JBeulich@suse.com>:
> 
> >
> > As outlined above, it is possible, but I'm not certain it is a good idea
> > to have balloon_set_new_target() exported. If you think that's
> > acceptable, I can certainly put together a patch doing that (on top
> > of the one here, and probably not immediately).
> >
> > Jan
> 
> Exporting balloon_set_new_target can be grate feature to do own
> modules for balloon memory inside xen guest, becouse some times we
> need more realtime control from dom0 for memory growing speed inside
> guest.

Sounds like we have two use cases :-)

I am not seeing anything against it but I wonder if other virtualization
offerings would benefit from this as well? As in would it make sense to
export the virtio-ballon or the microsoft one as well? And perhaps have
an unified API? So that the ballloon drivers register and then have a
"virt_mem_set_new_target" exported?