Re: [PATCH] skeleton frontend/backend examples and a deadlock

[Harry Butterworth <harry@hebutterworth.freeserve.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

On Wed, 2005-11-02 at 16:22 +1100, Rusty Russell wrote:
> Here are example frontend and backend driver skeletons.  They're
> *designed* to handle driver restart and module unloading.  However,
> device_unregister deadlocks.  I guess noone tried testing
> unregister_xenbus_watch when not called from a watch callback.

Thanks, that'll be useful for me to know when it comes to testing my
code.

For comparison, I've knocked up a skeleton FE/BE driver based on the
xenidc API that I'm using for my USB driver.

You'll see that my patch is smaller, simpler and provides significantly
more functionality: enough to send messages and transactions
bi-directionally between the FE and BE.  For a fair comparison, yours
would require interrupt handlers, use of the RING macros, correct memory
barriers etc.

Hopefully this helps to explain why I've split out the IDC functionality
from my USB driver into a generic service.

Harry.

[-- Attachment #2: xenidc-skeleton-patch --]
[-- Type: text/x-patch, Size: 18699 bytes --]

diff -r e376b14ac870 -r 4cc0fb2ce859 linux-2.6-xen-sparse/arch/xen/Kconfig
--- a/linux-2.6-xen-sparse/arch/xen/Kconfig	Wed Nov  2 10:25:12 2005
+++ b/linux-2.6-xen-sparse/arch/xen/Kconfig	Wed Nov  2 12:01:27 2005
@@ -210,6 +210,22 @@
 	  If security is not a concern then you may increase performance by
 	  saying N.
 
+config XEN_SKELETON_FE
+       tristate "Compile Xen skeleton example frontend driver code"
+       default m
+       help
+         There is an example skeleton driver frontend in drivers/xen/skeleton
+         which you can use as a basis for your own xenbus-aware
+         drivers.
+
+config XEN_SKELETON_BE
+       tristate "Compile Xen skeleton example backend driver code"
+       default m
+       help
+         There is an example skeleton driver backend in drivers/xen/skeleton
+         which you can use as a basis for your own xenbus-aware
+         drivers.
+
 choice
 	prompt "Processor Type"
 	default XEN_X86
diff -r e376b14ac870 -r 4cc0fb2ce859 linux-2.6-xen-sparse/drivers/xen/Makefile
--- a/linux-2.6-xen-sparse/drivers/xen/Makefile	Wed Nov  2 10:25:12 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/Makefile	Wed Nov  2 12:01:27 2005
@@ -6,6 +6,7 @@
 obj-y	+= privcmd/
 obj-y	+= xenbus/
 obj-y	+= xenidc/
+obj-y   += skeleton/
 
 obj-$(CONFIG_XEN_BLKDEV_BACKEND)	+= blkback/
 obj-$(CONFIG_XEN_NETDEV_BACKEND)	+= netback/
diff -r e376b14ac870 -r 4cc0fb2ce859 linux-2.6-xen-sparse/drivers/xen/skeleton/Makefile
--- /dev/null	Wed Nov  2 10:25:12 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/skeleton/Makefile	Wed Nov  2 12:01:27 2005
@@ -0,0 +1,2 @@
+obj-$(CONFIG_XEN_SKELETON_FE)  += skeleton_fe.o
+obj-$(CONFIG_XEN_SKELETON_BE)  += skeleton_be.o
diff -r e376b14ac870 -r 4cc0fb2ce859 linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_be.c
--- /dev/null	Wed Nov  2 10:25:12 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_be.c	Wed Nov  2 12:01:27 2005
@@ -0,0 +1,299 @@
+/*****************************************************************************/
+/* Copyright (c) 2005 Harry Butterworth IBM Corporation                      */
+/*                                                                           */
+/* This program is free software; you can redistribute it and/or modify it   */
+/* under the terms of the GNU General Public License as published by the     */
+/* Free Software Foundation; either version 2 of the License, or (at your    */
+/* option) any later version.                                                */
+/*                                                                           */
+/* This program is distributed in the hope that it will be useful, but       */
+/* WITHOUT ANY WARRANTY; without even the implied warranty of                */
+/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General */
+/* Public License for more details.                                          */
+/*                                                                           */
+/* You should have received a copy of the GNU General Public License along   */
+/* with this program; if not, write to the Free Software Foundation, Inc.,   */
+/* 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA                   */
+/*                                                                           */
+/*****************************************************************************/
+
+#include <linux/module.h>
+#include <asm-xen/xenbus.h>
+#include <asm-xen/xenidc.h>
+
+/* Skeleton driver doesn't actually send any messages or transactions. */
+
+#define SKELETON_BE_INITIATOR_QUOTA              0
+#define SKELETON_BE_INITIATOR_MAXIMUM_BYTE_COUNT 0
+#define SKELETON_BE_TARGET_QUOTA                 0
+#define SKELETON_BE_TARGET_MAXIMUM_BYTE_COUNT    0
+
+struct skeleton_be_device
+{
+    struct xenbus_device * dev;
+    xenidc_address         address;
+    long int               frontend_id;
+    char                 * frontend;
+    xenidc_endpoint        endpoint;
+    xenidc_callback      * endpoint_disconnect_callback;
+};
+
+static inline struct skeleton_be_device * skeleton_be_device_endpoint_to
+  ( xenidc_endpoint * endpoint )
+{
+    return container_of( endpoint, struct skeleton_be_device, endpoint );
+}
+
+static void skeleton_be_device_endpoint_connect( xenidc_endpoint * endpoint )
+{
+    /* Between connect and completion of the disconnect callback we are      */
+    /* allowed to issue messages and transactions.                           */
+}
+
+static void skeleton_be_device_endpoint_disconnect
+  ( xenidc_endpoint * endpoint, xenidc_callback * callback )
+{
+    /* We must stop issuing messages and transactions and complete the       */
+    /* callback once all of the messages and transactions we are issuing     */
+    /* have completed or failed back to us.                                  */
+
+    /* The skeleton driver doesn't issue any messages or transactions so can */
+    /* complete the callback immediately.                                    */
+
+    xenidc_callback_success( callback );
+}
+
+static void skeleton_be_device_endpoint_message
+  ( xenidc_endpoint * endpoint, xenidc_endpoint_message * message )
+{
+    /* Handle inbound message. */
+
+    xenidc_callback_success( xenidc_endpoint_message_to_callback( message ) );
+}
+
+static void skeleton_be_device_endpoint_transaction
+  ( xenidc_endpoint * endpoint, xenidc_endpoint_transaction * transaction )
+{
+    /* Handle inbound transaction. */
+
+    xenidc_callback_success
+      ( xenidc_endpoint_transaction_to_callback( transaction ) );
+}
+
+static int skeleton_be_device_suspend( struct xenbus_device * dev )
+{
+    struct skeleton_be_device * device = dev->data;
+
+    xenidc_endpoint_destroy( &device->endpoint );
+
+    kfree( device->frontend );
+
+    return 0;
+}
+
+static int skeleton_be_device_resume( struct xenbus_device * dev )
+{
+    struct skeleton_be_device * device = dev->data;
+
+    int return_value = xenbus_gather
+    (
+        NULL, /* FIXME? */
+        dev->nodename,
+        "frontend-id", "%li", &device->frontend_id,
+        "frontend",     NULL, &device->frontend,
+        NULL
+    );
+
+    if( XENBUS_EXIST_ERR( return_value ) )
+    {
+        goto EXIT_NO_FRONTEND;
+    }
+
+    if( return_value < 0 )
+    {
+        xenbus_dev_error
+        (
+            dev,
+            return_value,
+            "reading %s/frontend",
+            dev->nodename
+        );
+
+        goto EXIT_NO_FRONTEND;
+    }
+
+    xenidc_address_init
+    (
+        &device->address,
+        dev->nodename,
+        device->frontend,
+        device->frontend_id
+    );
+
+    xenidc_endpoint_create( &device->endpoint, device->address );
+
+    return 0;
+
+  EXIT_NO_FRONTEND:
+
+    return return_value;
+}
+
+static int skeleton_be_device_init_or_exit
+  ( struct skeleton_be_device * device, struct xenbus_device * dev, int exit )
+{
+    int return_value = 0;
+
+    if( exit )
+    {
+       goto EXIT;
+    }
+
+    memset( device, 0, sizeof( *device ) );
+
+    device->dev = dev;
+
+    return_value = xenidc_endpoint_init
+    (
+        &device->endpoint,
+        skeleton_be_device_endpoint_connect,
+        skeleton_be_device_endpoint_message,
+        skeleton_be_device_endpoint_transaction,
+        skeleton_be_device_endpoint_disconnect,
+        SKELETON_BE_INITIATOR_QUOTA,
+        SKELETON_BE_INITIATOR_MAXIMUM_BYTE_COUNT,
+        SKELETON_BE_TARGET_QUOTA,
+        SKELETON_BE_TARGET_MAXIMUM_BYTE_COUNT
+    );
+
+    if( return_value != 0 )
+    {
+        goto EXIT_NO_ENDPOINT;
+    }
+
+    return_value = skeleton_be_device_resume( dev );
+
+    if( return_value != 0 )
+    {
+        goto EXIT_NO_RESUME;
+    }
+
+    return 0;
+
+  EXIT:
+
+    skeleton_be_device_suspend( device->dev );
+
+  EXIT_NO_RESUME:
+
+    xenidc_endpoint_exit( &device->endpoint );
+
+  EXIT_NO_ENDPOINT:
+
+    return return_value;
+}
+
+static int skeleton_be_device_init
+  ( struct skeleton_be_device * device, struct xenbus_device * dev )
+{
+    return skeleton_be_device_init_or_exit( device, dev, 0 );
+}
+
+static void skeleton_be_device_exit( struct skeleton_be_device * device )
+{
+    (void)skeleton_be_device_init_or_exit( device, NULL, 1 );
+}
+
+static int skeleton_be_device_probe_or_remove
+  ( struct xenbus_device * dev, int remove )
+{
+    int return_value = 0;
+
+    struct skeleton_be_device * device;
+
+    if( remove )
+    {
+        goto REMOVE;
+    }
+
+    device = kmalloc( sizeof( *device ), GFP_KERNEL );
+
+    if( device == NULL )
+    {
+        xenbus_dev_error( dev, -ENOMEM, "allocating backend structure" );
+
+        return_value = -ENOMEM;
+
+        goto EXIT_NO_DEVICE;
+    }
+
+    dev->data = device;
+
+    return_value = skeleton_be_device_init( device, dev );
+
+    if( return_value != 0 )
+    {
+        goto EXIT_INIT_FAILED;
+    }
+
+    return 0;
+
+  REMOVE:
+
+    device = dev->data;
+
+    skeleton_be_device_exit( device );
+
+  EXIT_INIT_FAILED:
+
+    dev->data = NULL;
+
+    kfree( device );
+
+  EXIT_NO_DEVICE:
+
+    return return_value;
+}
+
+static int skeleton_be_device_probe
+  ( struct xenbus_device * dev, const struct xenbus_device_id * id )
+{
+    return skeleton_be_device_probe_or_remove( dev, 0 );
+}
+
+static int skeleton_be_device_remove( struct xenbus_device * dev )
+{
+    return skeleton_be_device_probe_or_remove( dev, 1 );
+}
+
+static struct xenbus_device_id skeleton_be_device_ids[] =
+{
+    { "skeleton" },
+    { "" }
+};
+
+static struct xenbus_driver skeleton_be_device_driver =
+{
+    .name    = "skeleton_be",
+    .owner   = THIS_MODULE,
+    .ids     = skeleton_be_device_ids,
+    .probe   = skeleton_be_device_probe,
+    .remove  = skeleton_be_device_remove,
+    .suspend = skeleton_be_device_suspend,
+    .resume  = skeleton_be_device_resume,
+};
+
+static int __init skeleton_be_module_init( void )
+{
+    return xenbus_register_driver( &skeleton_be_device_driver );
+}
+
+static void __exit skeleton_be_module_exit( void )
+{
+    xenbus_unregister_driver( &skeleton_be_device_driver );
+}
+
+module_init( skeleton_be_module_init );
+module_exit( skeleton_be_module_exit );
+
+MODULE_LICENSE( "GPL" );
diff -r e376b14ac870 -r 4cc0fb2ce859 linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_fe.c
--- /dev/null	Wed Nov  2 10:25:12 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/skeleton/skeleton_fe.c	Wed Nov  2 12:01:27 2005
@@ -0,0 +1,299 @@
+/*****************************************************************************/
+/* Copyright (c) 2005 Harry Butterworth IBM Corporation                      */
+/*                                                                           */
+/* This program is free software; you can redistribute it and/or modify it   */
+/* under the terms of the GNU General Public License as published by the     */
+/* Free Software Foundation; either version 2 of the License, or (at your    */
+/* option) any later version.                                                */
+/*                                                                           */
+/* This program is distributed in the hope that it will be useful, but       */
+/* WITHOUT ANY WARRANTY; without even the implied warranty of                */
+/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General */
+/* Public License for more details.                                          */
+/*                                                                           */
+/* You should have received a copy of the GNU General Public License along   */
+/* with this program; if not, write to the Free Software Foundation, Inc.,   */
+/* 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA                   */
+/*                                                                           */
+/*****************************************************************************/
+
+#include <linux/module.h>
+#include <asm-xen/xenbus.h>
+#include <asm-xen/xenidc.h>
+
+/* Skeleton driver doesn't actually send any messages or transactions. */
+
+#define SKELETON_FE_INITIATOR_QUOTA              0
+#define SKELETON_FE_INITIATOR_MAXIMUM_BYTE_COUNT 0
+#define SKELETON_FE_TARGET_QUOTA                 0
+#define SKELETON_FE_TARGET_MAXIMUM_BYTE_COUNT    0
+
+struct skeleton_fe_device
+{
+    struct xenbus_device     * dev;
+    xenidc_address             address;
+    long int                   backend_id;
+    char                     * backend;
+    xenidc_endpoint            endpoint;
+    xenidc_callback          * endpoint_disconnect_callback;
+};
+
+static inline struct skeleton_fe_device * skeleton_fe_device_endpoint_to
+  ( xenidc_endpoint * endpoint )
+{
+    return container_of( endpoint, struct skeleton_fe_device, endpoint );
+}
+
+static void skeleton_fe_device_endpoint_connect( xenidc_endpoint * endpoint )
+{
+    /* Between connect and completion of the disconnect callback we are      */
+    /* allowed to issue messages and transactions.                           */
+}
+
+static void skeleton_fe_device_endpoint_disconnect
+  ( xenidc_endpoint * endpoint, xenidc_callback * callback )
+{
+    /* We must stop issuing messages and transactions and complete the       */
+    /* callback once all of the messages and transactions we are issuing     */
+    /* have completed or failed back to us.                                  */
+
+    /* The skeleton driver doesn't issue any messages or transactions so can */
+    /* complete the callback immediately.                                    */
+
+    xenidc_callback_success( callback );
+}
+
+static void skeleton_fe_device_endpoint_message
+  ( xenidc_endpoint * endpoint, xenidc_endpoint_message * message )
+{
+    /* Handle inbound message. */
+
+    xenidc_callback_success( xenidc_endpoint_message_to_callback( message ) );
+}
+
+static void skeleton_fe_device_endpoint_transaction
+  ( xenidc_endpoint * endpoint, xenidc_endpoint_transaction * transaction )
+{
+    /* Handle inbound transaction. */
+
+    xenidc_callback_success
+      ( xenidc_endpoint_transaction_to_callback( transaction ) );
+}
+
+static int skeleton_fe_device_suspend( struct xenbus_device * dev )
+{
+    struct skeleton_fe_device * device = dev->data;
+
+    xenidc_endpoint_destroy( &device->endpoint );
+
+    kfree( device->backend );
+
+    return 0;
+}
+
+static int skeleton_fe_device_resume( struct xenbus_device * dev )
+{
+    struct skeleton_fe_device * device = dev->data;
+
+    int return_value = xenbus_gather
+    (
+        NULL, /* FIXME? */
+        dev->nodename,
+        "backend-id", "%li", &device->backend_id,
+        "backend",     NULL, &device->backend,
+        NULL
+    );
+
+    if( XENBUS_EXIST_ERR( return_value ) )
+    {
+        goto EXIT_NO_BACKEND;
+    }
+
+    if( return_value < 0 )
+    {
+        xenbus_dev_error
+        (
+            dev,
+            return_value,
+            "reading %s/backend",
+            dev->nodename
+        );
+
+        goto EXIT_NO_BACKEND;
+    }
+
+    xenidc_address_init
+    (
+        &device->address,
+        dev->nodename,
+        device->backend,
+        device->backend_id
+    );
+
+    xenidc_endpoint_create( &device->endpoint, device->address );
+
+    return 0;
+
+  EXIT_NO_BACKEND:
+
+    return return_value;
+}
+
+static int skeleton_fe_device_init_or_exit
+  ( struct skeleton_fe_device * device, struct xenbus_device * dev, int exit )
+{
+    int return_value = 0;
+
+    if( exit )
+    {
+       goto EXIT;
+    }
+
+    memset( device, 0, sizeof( *device ) );
+
+    device->dev = dev;
+
+    return_value = xenidc_endpoint_init
+    (
+        &device->endpoint,
+        skeleton_fe_device_endpoint_connect,
+        skeleton_fe_device_endpoint_message,
+        skeleton_fe_device_endpoint_transaction,
+        skeleton_fe_device_endpoint_disconnect,
+        SKELETON_FE_INITIATOR_QUOTA,
+        SKELETON_FE_INITIATOR_MAXIMUM_BYTE_COUNT,
+        SKELETON_FE_TARGET_QUOTA,
+        SKELETON_FE_TARGET_MAXIMUM_BYTE_COUNT
+    );
+
+    if( return_value != 0 )
+    {
+        goto EXIT_NO_ENDPOINT;
+    }
+
+    return_value = skeleton_fe_device_resume( dev );
+
+    if( return_value != 0 )
+    {
+        goto EXIT_NO_RESUME;
+    }
+
+    return 0;
+
+  EXIT:
+
+    skeleton_fe_device_suspend( device->dev );
+
+  EXIT_NO_RESUME:
+
+    xenidc_endpoint_exit( &device->endpoint );
+
+  EXIT_NO_ENDPOINT:
+
+    return return_value;
+}
+
+static int skeleton_fe_device_init
+  ( struct skeleton_fe_device * device, struct xenbus_device * dev )
+{
+    return skeleton_fe_device_init_or_exit( device, dev, 0 );
+}
+
+static void skeleton_fe_device_exit( struct skeleton_fe_device * device )
+{
+    (void)skeleton_fe_device_init_or_exit( device, NULL, 1 );
+}
+
+static int skeleton_fe_device_probe_or_remove
+  ( struct xenbus_device * dev, int remove )
+{
+    int return_value = 0;
+
+    struct skeleton_fe_device * device;
+
+    if( remove )
+    {
+        goto REMOVE;
+    }
+
+    device = kmalloc( sizeof( *device ), GFP_KERNEL );
+
+    if( device == NULL )
+    {
+        xenbus_dev_error( dev, -ENOMEM, "allocating frontend structure" );
+
+        return_value = -ENOMEM;
+
+        goto EXIT_NO_DEVICE;
+    }
+
+    dev->data = device;
+
+    return_value = skeleton_fe_device_init( device, dev );
+
+    if( return_value != 0 )
+    {
+        goto EXIT_INIT_FAILED;
+    }
+
+    return 0;
+
+  REMOVE:
+
+    device = dev->data;
+
+    skeleton_fe_device_exit( device );
+
+  EXIT_INIT_FAILED:
+
+    dev->data = NULL;
+
+    kfree( device );
+
+  EXIT_NO_DEVICE:
+
+    return return_value;
+}
+
+static int skeleton_fe_device_probe
+  ( struct xenbus_device * dev, const struct xenbus_device_id * id )
+{
+    return skeleton_fe_device_probe_or_remove( dev, 0 );
+}
+
+static int skeleton_fe_device_remove( struct xenbus_device * dev )
+{
+    return skeleton_fe_device_probe_or_remove( dev, 1 );
+}
+
+static struct xenbus_device_id skeleton_fe_device_ids[] =
+{
+    { "skeleton" },
+    { "" }
+};
+
+static struct xenbus_driver skeleton_fe_device_driver =
+{
+    .name    = "skeleton_fe",
+    .owner   = THIS_MODULE,
+    .ids     = skeleton_fe_device_ids,
+    .probe   = skeleton_fe_device_probe,
+    .remove  = skeleton_fe_device_remove,
+    .suspend = skeleton_fe_device_suspend,
+    .resume  = skeleton_fe_device_resume,
+};
+
+static int __init skeleton_fe_module_init( void )
+{
+    return xenbus_register_driver( &skeleton_fe_device_driver );
+}
+
+static void __exit skeleton_fe_module_exit( void )
+{
+    xenbus_unregister_driver( &skeleton_fe_device_driver );
+}
+
+module_init( skeleton_fe_module_init );
+module_exit( skeleton_fe_module_exit );
+
+MODULE_LICENSE( "GPL" );

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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