Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Xen>
  3. Devel
[PATCH 6/6] xen/xenbus-backend: Only register device if communication ring is local
waldi at debian
Dec 10, 2011, 10:29 AM
Post #1 of 5 (135 views)
Permalink
Signed-off-by: Bastian Blank <waldi@debian.org>
---
drivers/xen/xenbus/Makefile | 2 +-
drivers/xen/xenbus/xenbus_dev_backend.c | 13 ++-----------
drivers/xen/xenbus/xenbus_dev_backend.h | 2 ++
drivers/xen/xenbus/xenbus_probe.c | 18 ++++++++++++++++++
4 files changed, 23 insertions(+), 12 deletions(-)
create mode 100644 drivers/xen/xenbus/xenbus_dev_backend.h

diff --git a/drivers/xen/xenbus/Makefile b/drivers/xen/xenbus/Makefile
index 31e2e90..d751f45 100644
--- a/drivers/xen/xenbus/Makefile
+++ b/drivers/xen/xenbus/Makefile
@@ -7,8 +7,8 @@ xenbus-objs += xenbus_comms.o
xenbus-objs += xenbus_xs.o
xenbus-objs += xenbus_probe.o

+xenbus-be-objs-$(CONFIG_XEN_BACKEND) += xenbus_dev_backend.o
xenbus-be-objs-$(CONFIG_XEN_BACKEND) += xenbus_probe_backend.o
xenbus-objs += $(xenbus-be-objs-y)

-obj-$(CONFIG_XEN_BACKEND) += xenbus_dev_backend.o
obj-$(CONFIG_XEN_XENBUS_FRONTEND) += xenbus_probe_frontend.o
diff --git a/drivers/xen/xenbus/xenbus_dev_backend.c b/drivers/xen/xenbus/xenbus_dev_backend.c
index a2092bd..fb87f1b 100644
--- a/drivers/xen/xenbus/xenbus_dev_backend.c
+++ b/drivers/xen/xenbus/xenbus_dev_backend.c
@@ -3,7 +3,6 @@
#include <linux/mm.h>
#include <linux/fs.h>
#include <linux/miscdevice.h>
-#include <linux/module.h>
#include <linux/capability.h>

#include <xen/page.h>
@@ -11,8 +10,6 @@

#include "xenbus_comms.h"

-MODULE_LICENSE("GPL");
-
static int xenbus_backend_open(struct inode *inode, struct file *filp)
{
if (!capable(CAP_SYS_ADMIN))
@@ -67,23 +64,17 @@ static struct miscdevice xenbus_backend_dev = {
.fops = &xenbus_backend_fops,
};

-static int __init xenbus_backend_init(void)
+int __init xenbus_backend_init(void)
{
int err;

- if (!xen_initial_domain())
- return -ENODEV;
-
err = misc_register(&xenbus_backend_dev);
if (err)
printk(KERN_ERR "Could not register xenbus backend device\n");
return err;
}

-static void __exit xenbus_backend_exit(void)
+void __exit xenbus_backend_exit(void)
{
misc_deregister(&xenbus_backend_dev);
}
-
-module_init(xenbus_backend_init);
-module_exit(xenbus_backend_exit);
diff --git a/drivers/xen/xenbus/xenbus_dev_backend.h b/drivers/xen/xenbus/xenbus_dev_backend.h
new file mode 100644
index 0000000..d986853
--- /dev/null
+++ b/drivers/xen/xenbus/xenbus_dev_backend.h
@@ -0,0 +1,2 @@
+int xenbus_backend_init(void);
+void xenbus_backend_exit(void);
diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 1b178c6..4eff095 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -60,6 +60,7 @@
#include <xen/hvm.h>

#include "xenbus_comms.h"
+#include "xenbus_dev_backend.h"
#include "xenbus_probe.h"


@@ -641,6 +642,10 @@ EXPORT_SYMBOL_GPL(xenbus_dev_cancel);
/* A flag to determine if xenstored is 'ready' (i.e. has started) */
int xenstored_ready;

+/* A flag to determine if xenstored is 'local' */
+#ifdef CONFIG_XEN_BACKEND
+static int xenstored_local;
+#endif

int register_xenstore_notifier(struct notifier_block *nb)
{
@@ -675,6 +680,14 @@ static int __init xenbus_probe_initcall(void)
if (!xen_domain())
return -ENODEV;

+#ifdef CONFIG_XEN_BACKEND
+ if (xenstored_local) {
+ int ret = xenbus_backend_init();
+ if (ret)
+ return ret;
+ }
+#endif
+
if (xen_initial_domain() || xen_hvm_domain())
return 0;

@@ -747,9 +760,14 @@ static int __init xenbus_init(void)
if (xen_store_evtchn)
xenstored_ready = 1;
else {
+#ifdef CONFIG_XEN_BACKEND
+ xenstored_local = 1;
err = xenstored_local_init();
if (err)
goto out_error;
+#else
+ BUG();
+#endif
}
xen_store_interface = mfn_to_virt(xen_store_mfn);
}
--
1.7.7.3


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 6/6] xen/xenbus-backend: Only register device if communication ring is local [ In reply to ]
Ian.Campbell at citrix
Dec 12, 2011, 1:30 AM
Post #2 of 5 (117 views)
Permalink
On Sat, 2011-12-10 at 18:29 +0000, Bastian Blank wrote:
> +/* A flag to determine if xenstored is 'local' */
> +#ifdef CONFIG_XEN_BACKEND
> +static int xenstored_local;
> +#endif

I think this can be __initdata since all the users are __init.

> +#ifdef CONFIG_XEN_BACKEND
> + xenstored_local = 1;

I would push this down into xenstore_local_init() and only set it on
success.

> err = xenstored_local_init();

This is now only called if CONFIG_XEN_BACKEND. You should add a similar
#ifdef around the function definition.

Ian.

> if (err)
> goto out_error;
> +#else
> + BUG();
> +#endif
> }
> xen_store_interface = mfn_to_virt(xen_store_mfn);
> }



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 6/6] xen/xenbus-backend: Only register device if communication ring is local [ In reply to ]
konrad.wilk at oracle
Dec 12, 2011, 7:42 AM
Post #3 of 5 (114 views)
Permalink
On Sat, Dec 10, 2011 at 07:29:51PM +0100, Bastian Blank wrote:
> Signed-off-by: Bastian Blank <waldi@debian.org>
> ---
> drivers/xen/xenbus/Makefile | 2 +-
> drivers/xen/xenbus/xenbus_dev_backend.c | 13 ++-----------
> drivers/xen/xenbus/xenbus_dev_backend.h | 2 ++
> drivers/xen/xenbus/xenbus_probe.c | 18 ++++++++++++++++++
> 4 files changed, 23 insertions(+), 12 deletions(-)
> create mode 100644 drivers/xen/xenbus/xenbus_dev_backend.h
>
> diff --git a/drivers/xen/xenbus/Makefile b/drivers/xen/xenbus/Makefile
> index 31e2e90..d751f45 100644
> --- a/drivers/xen/xenbus/Makefile
> +++ b/drivers/xen/xenbus/Makefile
> @@ -7,8 +7,8 @@ xenbus-objs += xenbus_comms.o
> xenbus-objs += xenbus_xs.o
> xenbus-objs += xenbus_probe.o
>
> +xenbus-be-objs-$(CONFIG_XEN_BACKEND) += xenbus_dev_backend.o
> xenbus-be-objs-$(CONFIG_XEN_BACKEND) += xenbus_probe_backend.o
> xenbus-objs += $(xenbus-be-objs-y)
>
> -obj-$(CONFIG_XEN_BACKEND) += xenbus_dev_backend.o
> obj-$(CONFIG_XEN_XENBUS_FRONTEND) += xenbus_probe_frontend.o
> diff --git a/drivers/xen/xenbus/xenbus_dev_backend.c b/drivers/xen/xenbus/xenbus_dev_backend.c
> index a2092bd..fb87f1b 100644
> --- a/drivers/xen/xenbus/xenbus_dev_backend.c
> +++ b/drivers/xen/xenbus/xenbus_dev_backend.c
> @@ -3,7 +3,6 @@
> #include <linux/mm.h>
> #include <linux/fs.h>
> #include <linux/miscdevice.h>
> -#include <linux/module.h>
> #include <linux/capability.h>
>
> #include <xen/page.h>
> @@ -11,8 +10,6 @@
>
> #include "xenbus_comms.h"
>
> -MODULE_LICENSE("GPL");
> -
> static int xenbus_backend_open(struct inode *inode, struct file *filp)
> {
> if (!capable(CAP_SYS_ADMIN))
> @@ -67,23 +64,17 @@ static struct miscdevice xenbus_backend_dev = {
> .fops = &xenbus_backend_fops,
> };
>
> -static int __init xenbus_backend_init(void)
> +int __init xenbus_backend_init(void)
> {
> int err;
>
> - if (!xen_initial_domain())
> - return -ENODEV;
> -
> err = misc_register(&xenbus_backend_dev);
> if (err)
> printk(KERN_ERR "Could not register xenbus backend device\n");
> return err;
> }
>
> -static void __exit xenbus_backend_exit(void)
> +void __exit xenbus_backend_exit(void)
> {
> misc_deregister(&xenbus_backend_dev);
> }
> -
> -module_init(xenbus_backend_init);
> -module_exit(xenbus_backend_exit);

Why are we removing the module functionality?
Can't this [the purpose of this patch] be done while still maintaining the module functionality?

> diff --git a/drivers/xen/xenbus/xenbus_dev_backend.h b/drivers/xen/xenbus/xenbus_dev_backend.h
> new file mode 100644
> index 0000000..d986853
> --- /dev/null
> +++ b/drivers/xen/xenbus/xenbus_dev_backend.h
> @@ -0,0 +1,2 @@
> +int xenbus_backend_init(void);
> +void xenbus_backend_exit(void);
> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
> index 1b178c6..4eff095 100644
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -60,6 +60,7 @@
> #include <xen/hvm.h>
>
> #include "xenbus_comms.h"
> +#include "xenbus_dev_backend.h"
> #include "xenbus_probe.h"
>
>
> @@ -641,6 +642,10 @@ EXPORT_SYMBOL_GPL(xenbus_dev_cancel);
> /* A flag to determine if xenstored is 'ready' (i.e. has started) */
> int xenstored_ready;
>
> +/* A flag to determine if xenstored is 'local' */
> +#ifdef CONFIG_XEN_BACKEND
> +static int xenstored_local;

bool?

> +#endif
>
> int register_xenstore_notifier(struct notifier_block *nb)
> {
> @@ -675,6 +680,14 @@ static int __init xenbus_probe_initcall(void)
> if (!xen_domain())
> return -ENODEV;
>
> +#ifdef CONFIG_XEN_BACKEND
> + if (xenstored_local) {
> + int ret = xenbus_backend_init();
> + if (ret)
> + return ret;
> + }
> +#endif
> +
> if (xen_initial_domain() || xen_hvm_domain())
> return 0;
>
> @@ -747,9 +760,14 @@ static int __init xenbus_init(void)
> if (xen_store_evtchn)
> xenstored_ready = 1;
> else {
> +#ifdef CONFIG_XEN_BACKEND
> + xenstored_local = 1;
> err = xenstored_local_init();
> if (err)
> goto out_error;
> +#else
> + BUG();

No way. A WARN, sure - but BUG() is way too intense for this.

> +#endif
> }
> xen_store_interface = mfn_to_virt(xen_store_mfn);
> }
> --
> 1.7.7.3
>
> --
> 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/

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 6/6] xen/xenbus-backend: Only register device if communication ring is local [ In reply to ]
bastian at waldi
Dec 12, 2011, 10:28 AM
Post #4 of 5 (116 views)
Permalink
On Mon, Dec 12, 2011 at 10:42:27AM -0500, Konrad Rzeszutek Wilk wrote:
> On Sat, Dec 10, 2011 at 07:29:51PM +0100, Bastian Blank wrote:
> > -module_init(xenbus_backend_init);
> > -module_exit(xenbus_backend_exit);
>
> Why are we removing the module functionality?
> Can't this [the purpose of this patch] be done while still maintaining the module functionality?

Not really. The purpose is to register the device only if the ring is
local. This information is not available to modules.

However we could just move the whole xenbus ring setup into a "module".
Or is there any reason for this to be in a postcore_initcall, even if it
is only operational after a userspace process works?

> > int xenstored_ready;
> >
> > +/* A flag to determine if xenstored is 'local' */
> > +#ifdef CONFIG_XEN_BACKEND
> > +static int xenstored_local;
>
> bool?

Well. The other flags are int also.

> > + BUG();
> No way. A WARN, sure - but BUG() is way too intense for this.

Okay.

Bastian

--
The sight of death frightens them [Earthers].
-- Kras the Klingon, "Friday's Child", stardate 3497.2

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 6/6] xen/xenbus-backend: Only register device if communication ring is local [ In reply to ]
konrad.wilk at oracle
Dec 14, 2011, 11:22 AM
Post #5 of 5 (116 views)
Permalink
On Mon, Dec 12, 2011 at 07:28:07PM +0100, Bastian Blank wrote:
> On Mon, Dec 12, 2011 at 10:42:27AM -0500, Konrad Rzeszutek Wilk wrote:
> > On Sat, Dec 10, 2011 at 07:29:51PM +0100, Bastian Blank wrote:
> > > -module_init(xenbus_backend_init);
> > > -module_exit(xenbus_backend_exit);
> >
> > Why are we removing the module functionality?
> > Can't this [the purpose of this patch] be done while still maintaining the module functionality?
>
> Not really. The purpose is to register the device only if the ring is
> local. This information is not available to modules.
>
> However we could just move the whole xenbus ring setup into a "module".
> Or is there any reason for this to be in a postcore_initcall, even if it
> is only operational after a userspace process works?

My thinking is that we do not want to use memory space if the kernel
is booted as baremetal (as there is no point of having Xen pieces around).
Modules work great for that.

But then part of this code is __init so some does get evicted, but not sure
how much. Could you measure this on baremetal please?

>
> > > int xenstored_ready;
> > >
> > > +/* A flag to determine if xenstored is 'local' */
> > > +#ifdef CONFIG_XEN_BACKEND
> > > +static int xenstored_local;
> >
> > bool?
>
> Well. The other flags are int also.

Sure.. and they ought to be modified to bool too now that I think of it.

>
> > > + BUG();
> > No way. A WARN, sure - but BUG() is way too intense for this.
>
> Okay.
>
> Bastian
>
> --
> The sight of death frightens them [Earthers].
> -- Kras the Klingon, "Friday's Child", stardate 3497.2

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

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal