[PATCH][2/17] USB virt 2.6 split driver---xenidc buffer resource provider

[PATCH][2/17] USB virt 2.6 split driver---xenidc buffer resource provider

[harry]

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

This patch implements a resource pool for buffer resources used by the
xenidc transport which is in turn used by the USB driver.

Signed-off-by: Harry Butterworth <butterwo@uk.ibm.com>


[-- Attachment #2: p2-xenidc-buffer-resource-provider.patch --]
[-- Type: text/x-patch, Size: 19255 bytes --]

diff -r 7adcceaaf851 -r 1e2c0ff9c46a linux-2.6-xen-sparse/drivers/xen/xenidc/Makefile
--- a/linux-2.6-xen-sparse/drivers/xen/xenidc/Makefile	Sun Nov 20 14:53:27 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/xenidc/Makefile	Sun Nov 20 14:53:55 2005
@@ -3,3 +3,4 @@
 xenidc-objs =
 xenidc-objs += xenidc_callback.o
 xenidc-objs += xenidc_work.o
+xenidc-objs += xenidc_buffer_resource_provider.o
diff -r 7adcceaaf851 -r 1e2c0ff9c46a linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c
--- /dev/null	Sun Nov 20 14:53:27 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c	Sun Nov 20 14:53:55 2005
@@ -0,0 +1,443 @@
+/*****************************************************************************/
+/* Xen inter-domain communication buffer resource provider.                  */
+/*                                                                           */
+/* 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                   */
+/*                                                                           */
+/*****************************************************************************/
+/*****************************************************************************/
+/* Based on                                                                  */
+/*                                                                           */
+/* arch/xen/drivers/blkback/blkback.c                                        */
+/*                                                                           */
+/* original copyright notice follows...                                      */
+/*****************************************************************************/
+/******************************************************************************
+ * arch/xen/drivers/blkif/backend/main.c
+ * 
+ * Back-end of the driver for virtual block devices. This portion of the
+ * driver exports a 'unified' block-device interface that can be accessed
+ * by any operating system that implements a compatible front end. A 
+ * reference front-end implementation can be found in:
+ *  arch/xen/drivers/blkif/frontend
+ * 
+ * Copyright (c) 2003-2004, Keir Fraser & Steve Hand
+ * Copyright (c) 2005, Christopher Clark
+ */
+
+#include <asm/pgalloc.h>
+#include <linux/spinlock.h>
+#include <asm-xen/balloon.h>
+#include <asm-xen/driver_util.h>
+#include <asm-xen/xenidc_buffer_resource_provider.h>
+#include <linux/errno.h>
+#include <linux/gfp.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/vmalloc.h>
+#include "xenidc_trace.h"
+
+struct xenidc_buffer_resource_provider_struct {
+	spinlock_t lock;
+	xenidc_buffer_resource_list resource_allocation;
+	xenidc_buffer_resource_list free_resources;
+	struct list_head page_list;
+	grant_ref_t grant_ref_pool;
+	struct page *page;
+	unsigned long mmap_vstart;
+	struct list_head empty_page_range_list;
+	struct list_head *empty_page_range_link;
+};
+
+static int xenidc_buffer_resource_provider_init_or_exit
+    (xenidc_buffer_resource_provider * provider, int exit) {
+	trace1("%p", provider);
+
+	{
+		int return_value = 0;
+
+		if (exit) {
+			goto EXIT;
+		}
+
+		spin_lock_init(&provider->lock);
+
+		provider->free_resources = xenidc_buffer_resource_list_null();
+
+		INIT_LIST_HEAD(&provider->page_list);
+
+		{
+			int i;
+
+			for (i = 0; i < provider->resource_allocation.pages;
+			     i++) {
+				void *page =
+				    (void *)__get_free_page(GFP_KERNEL);
+
+				if (page == NULL) {
+					return_value = -ENOMEM;
+
+					goto EXIT_NO_PAGE;
+				}
+
+				xenidc_buffer_resource_provider_free_page
+				    (provider, page);
+			}
+		}
+
+		if (provider->resource_allocation.grant_references != 0) {
+			return_value = gnttab_alloc_grant_references
+			    (provider->resource_allocation.grant_references,
+			     &provider->grant_ref_pool);
+
+			if (return_value != 0) {
+				trace0
+				    ("failed to allocate grant reference pool");
+
+				goto EXIT_NO_GRANT_REF;
+			}
+
+			provider->free_resources.grant_references =
+			    provider->resource_allocation.grant_references;
+		}
+
+		if (provider->resource_allocation.empty_page_ranges != 0) {
+			provider->page = balloon_alloc_empty_page_range
+			    (provider->resource_allocation.empty_page_ranges
+			     *
+			     provider->resource_allocation.
+			     empty_page_range_page_count);
+
+			if (provider->page == NULL) {
+				return_value = -ENOMEM;
+
+				goto EXIT_NO_EMPTY_PAGE_RANGE;
+			}
+
+			provider->mmap_vstart =
+			    (unsigned long)
+			    pfn_to_kaddr(page_to_pfn(provider->page));
+
+			INIT_LIST_HEAD(&provider->empty_page_range_list);
+
+			provider->empty_page_range_link = vmalloc
+			    (sizeof(struct list_head)
+			     * provider->resource_allocation.empty_page_ranges);
+
+			if (provider->empty_page_range_link == NULL) {
+				return_value = -ENOMEM;
+
+				goto EXIT_NO_EMPTY_PAGE_RANGE_LINK;
+			}
+
+			{
+				int i;
+
+				for (i = 0;
+				     i <
+				     provider->resource_allocation.
+				     empty_page_ranges; i++) {
+					struct list_head *link =
+					    &provider->empty_page_range_link[i];
+
+					INIT_LIST_HEAD(link);
+
+					list_add(link,
+						 &provider->
+						 empty_page_range_list);
+				}
+			}
+
+			provider->free_resources.empty_page_ranges =
+			    provider->resource_allocation.empty_page_ranges;
+
+			provider->free_resources.empty_page_range_page_count =
+			    provider->resource_allocation.
+			    empty_page_range_page_count;
+		}
+
+		return 0;
+
+	      EXIT:
+
+		if (provider->resource_allocation.empty_page_ranges != 0) {
+			vfree(provider->empty_page_range_link);
+
+		      EXIT_NO_EMPTY_PAGE_RANGE_LINK:
+
+			balloon_dealloc_empty_page_range
+			    (provider->page,
+			     provider->resource_allocation.empty_page_ranges
+			     *
+			     provider->resource_allocation.
+			     empty_page_range_page_count);
+		}
+
+	      EXIT_NO_EMPTY_PAGE_RANGE:
+
+		if (provider->resource_allocation.grant_references != 0) {
+			gnttab_free_grant_references(provider->grant_ref_pool);
+		}
+
+	      EXIT_NO_GRANT_REF:
+
+	      EXIT_NO_PAGE:
+
+		while (!list_empty(&provider->page_list)) {
+			free_page
+			    ((unsigned long)
+			     xenidc_buffer_resource_provider_allocate_page
+			     (provider)
+			    );
+		}
+
+		return return_value;
+	}
+}
+
+xenidc_buffer_resource_provider *xenidc_allocate_buffer_resource_provider
+    (xenidc_buffer_resource_list resource_allocation) {
+	trace();
+
+	{
+		xenidc_buffer_resource_provider *provider =
+		    (xenidc_buffer_resource_provider *)
+		    vmalloc(sizeof(xenidc_buffer_resource_provider));
+
+		if (provider != NULL) {
+			provider->resource_allocation = resource_allocation;
+
+			if (xenidc_buffer_resource_provider_init_or_exit
+			    (provider, 0)
+			    != 0) {
+				vfree(provider);
+
+				provider = NULL;
+			}
+		}
+
+		return provider;
+	}
+}
+
+void xenidc_free_buffer_resource_provider
+    (xenidc_buffer_resource_provider * provider) {
+	trace();
+
+	(void)xenidc_buffer_resource_provider_init_or_exit(provider, 1);
+
+	vfree(provider);
+}
+
+xenidc_buffer_resource_list
+    xenidc_buffer_resource_provider_query_free_resources
+    (xenidc_buffer_resource_provider * provider) {
+	trace();
+
+	{
+		xenidc_buffer_resource_list list;
+
+		unsigned long flags;
+
+		spin_lock_irqsave(&provider->lock, flags);
+
+		list = provider->free_resources;
+
+		spin_unlock_irqrestore(&provider->lock, flags);
+
+		return list;
+	}
+}
+
+void *xenidc_buffer_resource_provider_allocate_page
+    (xenidc_buffer_resource_provider * provider) {
+	trace();
+
+	{
+		struct list_head *link;
+
+		unsigned long flags;
+
+		spin_lock_irqsave(&provider->lock, flags);
+
+		provider->free_resources.pages--;
+
+		link = provider->page_list.next;
+
+		trace1("%p", link);
+
+		list_del(link);
+
+		spin_unlock_irqrestore(&provider->lock, flags);
+
+		return link;
+	}
+}
+
+void xenidc_buffer_resource_provider_free_page
+    (xenidc_buffer_resource_provider * provider, void *page) {
+	trace1("%p", page);
+
+	{
+		struct list_head *link = page;
+
+		INIT_LIST_HEAD(link);
+
+		{
+			unsigned long flags;
+
+			spin_lock_irqsave(&provider->lock, flags);
+
+			provider->free_resources.pages++;
+
+			list_add(link, &provider->page_list);
+
+			spin_unlock_irqrestore(&provider->lock, flags);
+		}
+	}
+}
+
+grant_ref_t xenidc_buffer_resource_provider_allocate_grant_reference
+    (xenidc_buffer_resource_provider * provider) {
+	trace();
+
+	{
+		grant_ref_t reference;
+
+		{
+			unsigned long flags;
+
+			spin_lock_irqsave(&provider->lock, flags);
+
+			provider->free_resources.grant_references--;
+
+			reference =
+			    gnttab_claim_grant_reference(&provider->
+							 grant_ref_pool);
+
+			spin_unlock_irqrestore(&provider->lock, flags);
+		}
+
+		return reference;
+	}
+}
+
+void xenidc_buffer_resource_provider_free_grant_reference
+    (xenidc_buffer_resource_provider * provider, grant_ref_t reference) {
+	trace();
+
+	{
+		unsigned long flags;
+
+		spin_lock_irqsave(&provider->lock, flags);
+
+		gnttab_release_grant_reference(&provider->grant_ref_pool,
+					       reference);
+
+		provider->free_resources.grant_references++;
+
+		spin_unlock_irqrestore(&provider->lock, flags);
+	}
+}
+
+unsigned long xenidc_buffer_resource_provider_allocate_empty_page_range
+    (xenidc_buffer_resource_provider * provider, unsigned long page_count) {
+	trace();
+
+	{
+		unsigned long page_range;
+
+		{
+			unsigned long flags;
+
+			spin_lock_irqsave(&provider->lock, flags);
+
+			{
+				struct list_head *link =
+				    provider->empty_page_range_list.next;
+
+				list_del_init(link);
+
+				provider->free_resources.empty_page_ranges--;
+
+				page_range =
+				    (provider->mmap_vstart
+				     +
+				     (PAGE_SIZE
+				      *
+				      provider->resource_allocation.
+				      empty_page_range_page_count
+				      *
+				      (((unsigned long)link
+					-
+					(unsigned long)provider->
+					empty_page_range_link)
+				       / sizeof(struct list_head)
+				      )
+				     )
+				    );
+			}
+
+			spin_unlock_irqrestore(&provider->lock, flags);
+		}
+
+		return page_range;
+	}
+}
+
+void xenidc_buffer_resource_provider_free_empty_page_range
+    (xenidc_buffer_resource_provider * provider, unsigned long page_range) {
+	trace();
+
+	{
+		int i = ((page_range - provider->mmap_vstart)
+			 /
+			 (PAGE_SIZE
+			  *
+			  provider->resource_allocation.
+			  empty_page_range_page_count)
+		    );
+
+		unsigned long flags;
+
+		spin_lock_irqsave(&provider->lock, flags);
+
+		list_add
+		    (&provider->empty_page_range_link[i],
+		     &provider->empty_page_range_list);
+
+		provider->free_resources.empty_page_ranges++;
+
+		spin_unlock_irqrestore(&provider->lock, flags);
+	}
+}
+
+void xenidc_buffer_resource_list_trace(xenidc_buffer_resource_list list)
+{
+	trace();
+
+	printk
+	    (KERN_INFO
+	     "xenidc %s: "
+	     "pages:%d; "
+	     "grant_references:%d; "
+	     "empty_page_ranges:%d; "
+	     "empty_page_range_page_count:%d\n",
+	     __PRETTY_FUNCTION__,
+	     list.pages,
+	     list.grant_references,
+	     list.empty_page_ranges, list.empty_page_range_page_count);
+}
diff -r 7adcceaaf851 -r 1e2c0ff9c46a linux-2.6-xen-sparse/include/asm-xen/xenidc_buffer_resource_provider.h
--- /dev/null	Sun Nov 20 14:53:27 2005
+++ b/linux-2.6-xen-sparse/include/asm-xen/xenidc_buffer_resource_provider.h	Sun Nov 20 14:53:55 2005
@@ -0,0 +1,138 @@
+/*****************************************************************************/
+/* Xen inter-domain communication buffer resource provider.                  */
+/*                                                                           */
+/* 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                   */
+/*                                                                           */
+/*****************************************************************************/
+/*                                                                           */
+/* The main point of this buffer resource provider code is to make it        */
+/* possible to write some other generic code to manipulate various types of  */
+/* buffers and keep the other code independent of any buffer-type-specific   */
+/* resource management.                                                      */
+/*                                                                           */
+/* The initial implementation of this code provides sets of resources to     */
+/* clients in isolated per-client pools but this code provides a centralised */
+/* point of buffer resource management so it will be easy to implement a     */
+/* more efficient version which maintains isolation for a base set of        */
+/* resources to avoid deadlock and then shares additional resources between  */
+/* clients.                                                                  */
+
+#ifndef __ASM_XEN_XENIDC_BUFFER_RESOURCE_PROVIDER_H__
+#define __ASM_XEN_XENIDC_BUFFER_RESOURCE_PROVIDER_H__
+
+#include <asm/types.h>
+#include <asm-xen/gnttab.h>
+
+/* A xenidc_buffer_resource_list describes a set of resources, sometimes a   */
+/* set of resources required for an operation and sometimes a set of         */
+/* resources available to perform operations.                                */
+
+typedef struct xenidc_buffer_resource_list_struct xenidc_buffer_resource_list;
+
+struct xenidc_buffer_resource_list_struct {
+	u32 pages;
+	u32 grant_references;
+	u32 empty_page_ranges;
+	u32 empty_page_range_page_count;
+};
+
+/* xenidc_buffer_resource_list_null returns a list specifying no resources. */
+
+static inline xenidc_buffer_resource_list xenidc_buffer_resource_list_null(void)
+{
+	xenidc_buffer_resource_list list;
+
+	memset(&list, 0, sizeof(list));
+
+	return list;
+}
+
+/* xenidc_buffer_resource_list_subset_of returns true iff the resource        */
+/* requirements specified by list 'a' are met by the resources available in   */
+/* list 'b'.                                                                  */
+
+static inline int xenidc_buffer_resource_list_subset_of
+    (xenidc_buffer_resource_list * a, xenidc_buffer_resource_list * b) {
+	return ((a->pages <= b->pages)
+		&& (a->grant_references <= b->grant_references)
+		&& (a->empty_page_ranges <= b->empty_page_ranges)
+		&&
+		(a->empty_page_range_page_count <=
+		 b->empty_page_range_page_count)
+	    );
+}
+
+/* xenidc_buffer_resource_list_plus_equals adds the resource requirements     */
+/* specified by list 'b' to the resource requirements specified by list 'a'   */
+/* such that the resulting list 'a' specifies a set of resources sufficient   */
+/* to meet the sum of the requirements of the original 'a' list and 'b' lists.*/
+
+static inline void xenidc_buffer_resource_list_plus_equals
+    (xenidc_buffer_resource_list * a, xenidc_buffer_resource_list * b) {
+	a->pages += b->pages;
+	a->grant_references += b->grant_references;
+	a->empty_page_ranges += b->empty_page_ranges;
+
+	if (b->empty_page_range_page_count > a->empty_page_range_page_count) {
+		a->empty_page_range_page_count = b->empty_page_range_page_count;
+	}
+}
+
+/* Clients allocate a xenidc_buffer_resource_provider to provide themselves   */
+/* with a pool of resources.                                                  */
+
+typedef struct xenidc_buffer_resource_provider_struct
+    xenidc_buffer_resource_provider;
+
+extern xenidc_buffer_resource_provider *xenidc_allocate_buffer_resource_provider
+    (xenidc_buffer_resource_list resource_allocation);
+
+extern void xenidc_free_buffer_resource_provider
+    (xenidc_buffer_resource_provider * provider);
+
+/* Find out what resources are currently free for allocation. */
+
+extern xenidc_buffer_resource_list
+    xenidc_buffer_resource_provider_query_free_resources
+    (xenidc_buffer_resource_provider * provider);
+
+/* Resource-type specific allocate/free functions. */
+
+extern void *xenidc_buffer_resource_provider_allocate_page
+    (xenidc_buffer_resource_provider * provider);
+
+extern void xenidc_buffer_resource_provider_free_page
+    (xenidc_buffer_resource_provider * provider, void *page);
+
+extern grant_ref_t xenidc_buffer_resource_provider_allocate_grant_reference
+    (xenidc_buffer_resource_provider * provider);
+
+extern void xenidc_buffer_resource_provider_free_grant_reference
+    (xenidc_buffer_resource_provider * provider, grant_ref_t reference);
+
+extern unsigned long xenidc_buffer_resource_provider_allocate_empty_page_range
+    (xenidc_buffer_resource_provider * provider, unsigned long page_count);
+
+extern void xenidc_buffer_resource_provider_free_empty_page_range
+    (xenidc_buffer_resource_provider * provider, unsigned long page_range);
+
+/* xenidc_buffer_resource_list_trace is used for debugging and printks a dump */
+/* of the resource list.                                                      */
+
+extern void xenidc_buffer_resource_list_trace(xenidc_buffer_resource_list list);
+
+#endif

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

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

Re: [PATCH][2/17] USB virt 2.6 split driver---xenidc buffer resource provider

[Muli Ben-Yehuda]

> +static int xenidc_buffer_resource_provider_init_or_exit
> +    (xenidc_buffer_resource_provider * provider, int exit) {
> +	trace1("%p", provider);
> +
> +	{

This function is way way too long.

> +		{

No internal blocks please.

> +		if (provider->resource_allocation.empty_page_ranges != 0) {
> +			provider->page = balloon_alloc_empty_page_range
> +			    (provider->resource_allocation.empty_page_ranges
> +			     *
> +			     provider->resource_allocation.
> +			     empty_page_range_page_count);

lindent brain damage (putting the '*' on its own line).

> +			if (provider->page == NULL) {
> +				return_value = -ENOMEM;
> +
> +				goto EXIT_NO_EMPTY_PAGE_RANGE;

common style is not to put the label in all uppercaps.

> +			if (xenidc_buffer_resource_provider_init_or_exit
> +			    (provider, 0)
> +			    != 0) {
> +				vfree(provider);

using vmalloc/vfree is discouraged unless you must.

> +void xenidc_free_buffer_resource_provider
> +    (xenidc_buffer_resource_provider * provider) {
> +	trace();
> +
> +	(void)xenidc_buffer_resource_provider_init_or_exit(provider, 1);
> +
> +	vfree(provider);

If init_or_exit fails won't you vfree an already freed pointer here?

> +		xenidc_buffer_resource_list list;
> +
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&provider->lock, flags);
> +
> +		list = provider->free_resources;
> +
> +		spin_unlock_irqrestore(&provider->lock, flags);
> +
> +		return list;

You have a lot of empty lines. This function needs no empty lines, for
example, except maybe after the variable declarations. Also, does
'list' need to be reference counted here?

> +typedef struct xenidc_buffer_resource_provider_struct
> +    xenidc_buffer_resource_provider;

don't typedef structs.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: [PATCH][2/17] USB virt 2.6 split driver---xenidc buffer resource provider

[Harry Butterworth]

On Mon, 2005-11-21 at 22:18 +0200, Muli Ben-Yehuda wrote:
> > +static int xenidc_buffer_resource_provider_init_or_exit
> > +    (xenidc_buffer_resource_provider * provider, int exit) {
> > +	trace1("%p", provider);
> > +
> > +	{
> 
> This function is way way too long.

It's straight line code doing initialisation.  Breaking it up would be
artificial.  I suppose I could split it up to have a separate init
function for each kind of resource.

> 
> > +		{
> 
> No internal blocks please.

Well, I agree that they don't work well with 8 space tabs.  So, while we
have the big tabs I guess they'll have to go.

> 
> > +		if (provider->resource_allocation.empty_page_ranges != 0) {
> > +			provider->page = balloon_alloc_empty_page_range
> > +			    (provider->resource_allocation.empty_page_ranges
> > +			     *
> > +			     provider->resource_allocation.
> > +			     empty_page_range_page_count);
> 
> lindent brain damage (putting the '*' on its own line).
OK.
> 
> > +			if (provider->page == NULL) {
> > +				return_value = -ENOMEM;
> > +
> > +				goto EXIT_NO_EMPTY_PAGE_RANGE;
> 
> common style is not to put the label in all uppercaps.
> 
OK.
> > +			if (xenidc_buffer_resource_provider_init_or_exit
> > +			    (provider, 0)
> > +			    != 0) {
> > +				vfree(provider);
> 
> using vmalloc/vfree is discouraged unless you must.

I don't understand this.  I thought vmalloc was more likely to be
successful than kmalloc because the memory doesn't need to be contiguous
so I thought it was preferable to use vmalloc when possible.

> 
> > +void xenidc_free_buffer_resource_provider
> > +    (xenidc_buffer_resource_provider * provider) {
> > +	trace();
> > +
> > +	(void)xenidc_buffer_resource_provider_init_or_exit(provider, 1);
> > +
> > +	vfree(provider);
> 
> If init_or_exit fails won't you vfree an already freed pointer here?

No, init_or_exit never fails on the exit path so the code is correct.

> 
> > +		xenidc_buffer_resource_list list;
> > +
> > +		unsigned long flags;
> > +
> > +		spin_lock_irqsave(&provider->lock, flags);
> > +
> > +		list = provider->free_resources;
> > +
> > +		spin_unlock_irqrestore(&provider->lock, flags);
> > +
> > +		return list;
> 
> You have a lot of empty lines. This function needs no empty lines, for
> example, except maybe after the variable declarations. Also, does
> 'list' need to be reference counted here?

I'm used to a lot of empty lines.  I can get rid of them all if you
like.

No, 'list' doesn't need to be reference counted.

> 
> > +typedef struct xenidc_buffer_resource_provider_struct
> > +    xenidc_buffer_resource_provider;
> 
> don't typedef structs.

OK.

> 
> Cheers,
> Muli
-- 
Harry Butterworth <harry@hebutterworth.freeserve.co.uk>

Re: [PATCH][2/17] USB virt 2.6 split driver---xenidc buffer resource provider

[Muli Ben-Yehuda]

On Mon, Nov 21, 2005 at 09:30:00PM +0000, Harry Butterworth wrote:

> > > +			if (xenidc_buffer_resource_provider_init_or_exit
> > > +			    (provider, 0)
> > > +			    != 0) {
> > > +				vfree(provider);
> > 
> > using vmalloc/vfree is discouraged unless you must.
> 
> I don't understand this.  I thought vmalloc was more likely to be
> successful than kmalloc because the memory doesn't need to be contiguous
> so I thought it was preferable to use vmalloc when possible.

Nope, vmalloc has both a resource usage issue (we only have a limited
vmalloc space) and some small overhead that kmalloc doesn't. The only
time you should use vmalloc is if you know that kmalloc can't give you
a large enough buffer.

> > You have a lot of empty lines. This function needs no empty lines, for
> > example, except maybe after the variable declarations. Also, does
> > 'list' need to be reference counted here?
> 
> I'm used to a lot of empty lines.  I can get rid of them all if you
> like.

I would like it, but I'm not the one who will end up deciding whether
to commit it or not ;-)

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: [PATCH][2/17] USB virt 2.6 split driver---xenidc buffer resource provider

[harry]

On Tue, 2005-11-22 at 12:55 +0200, Muli Ben-Yehuda wrote:
> On Mon, Nov 21, 2005 at 09:30:00PM +0000, Harry Butterworth wrote:
> 
> > > > +			if (xenidc_buffer_resource_provider_init_or_exit
> > > > +			    (provider, 0)
> > > > +			    != 0) {
> > > > +				vfree(provider);
> > > 
> > > using vmalloc/vfree is discouraged unless you must.
> > 
> > I don't understand this.  I thought vmalloc was more likely to be
> > successful than kmalloc because the memory doesn't need to be contiguous
> > so I thought it was preferable to use vmalloc when possible.
> 
> Nope, vmalloc has both a resource usage issue (we only have a limited
> vmalloc space) and some small overhead that kmalloc doesn't. The only
> time you should use vmalloc is if you know that kmalloc can't give you
> a large enough buffer.
OK
> 
> > > You have a lot of empty lines. This function needs no empty lines, for
> > > example, except maybe after the variable declarations. Also, does
> > > 'list' need to be reference counted here?
> > 
> > I'm used to a lot of empty lines.  I can get rid of them all if you
> > like.
> 
> I would like it, but I'm not the one who will end up deciding whether
> to commit it or not ;-)
Well, If they give me some feedback, I can do what they want.
> 
> Cheers,
> Muli

Re: [PATCH][2/17] USB virt 2.6 split driver---xenidc buffer resource provider

[Keir Fraser]

On 22 Nov 2005, at 10:55, Muli Ben-Yehuda wrote:

>> I don't understand this.  I thought vmalloc was more likely to be
>> successful than kmalloc because the memory doesn't need to be 
>> contiguous
>> so I thought it was preferable to use vmalloc when possible.
>
> Nope, vmalloc has both a resource usage issue (we only have a limited
> vmalloc space) and some small overhead that kmalloc doesn't. The only
> time you should use vmalloc is if you know that kmalloc can't give you
> a large enough buffer.

If the buffer is > PAGE_SIZE, you should use vmalloc if at all 
possible. Order != 0 allocations can easily fail on a churned box with 
lots of memory fragmentation.

  -- Keir

Re: [PATCH][2/17] USB virt 2.6 split driver---xenidc buffer resource provider

[Muli Ben-Yehuda]

On Tue, Nov 22, 2005 at 11:12:29AM +0000, Keir Fraser wrote:

> If the buffer is > PAGE_SIZE, you should use vmalloc if at all 
> possible. Order != 0 allocations can easily fail on a churned box with 
> lots of memory fragmentation.

Quoth Linus[1[:

"vmalloc() is NOT SOMETHING YOU SHOULD EVER USE! It's only valid for
when you _need_ a big array, and you don't have any choice. It's slow,
and it's a very restricted resource: it's a global resource that is
literally restricted to a few tens of megabytes. It should be _very_
carefully used."

The main reason not to use vmalloc has a run-time overhead due to the
increased tlb footprint and is also a limited resource (128MB on x86,
IIRC). I've heard of systems running out of vmalloc space and
dying with plenty of memory otherwise available. There are other ways
to deal with high order allocations failing, e.g. pre-allocating, not
using GFP_ATOMIC where not absolutely necessary, etc.

[1] http://www.ussg.iu.edu/hypermail/linux/kernel/0311.1/0576.html

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: [PATCH][2/17] USB virt 2.6 split driver---xenidc buffer resource provider

[Keir Fraser]

On 22 Nov 2005, at 11:22, Muli Ben-Yehuda wrote:

> Quoth Linus[1[:
>
> "vmalloc() is NOT SOMETHING YOU SHOULD EVER USE! It's only valid for
> when you _need_ a big array, and you don't have any choice. It's slow,
> and it's a very restricted resource: it's a global resource that is
> literally restricted to a few tens of megabytes. It should be _very_
> carefully used."

He also says the correct fix is to not need a multi-page chunk in the 
first place. If you do, then you should pre-allocate. If that's 
impossible (e.g., you're a module) then you use vmalloc(). The page 
allocator does not attempt any compaction, so even if there is plenty 
of memory you may well not find a big contiguous extent.

  -- Keir

Re: *** SPAM *** Re: [PATCH][2/17] USB virt 2.6 split driver---xenidc buffer resource provider

[harry]

On Tue, 2005-11-22 at 12:06 +0000, Keir Fraser wrote:
> On 22 Nov 2005, at 11:22, Muli Ben-Yehuda wrote:
> 
> > Quoth Linus[1[:
> >
> > "vmalloc() is NOT SOMETHING YOU SHOULD EVER USE! It's only valid for
> > when you _need_ a big array, and you don't have any choice. It's slow,
> > and it's a very restricted resource: it's a global resource that is
> > literally restricted to a few tens of megabytes. It should be _very_
> > carefully used."
> 
> He also says the correct fix is to not need a multi-page chunk in the 
> first place. If you do, then you should pre-allocate. If that's 
> impossible (e.g., you're a module) then you use vmalloc(). The page 
> allocator does not attempt any compaction, so even if there is plenty 
> of memory you may well not find a big contiguous extent.

I have a couple of small vmallocs which will be OK as kmallocs and the others I'm just vmalloc-ing a chunk and then splitting it up into a number of smaller resources anyway so I could kmalloc them individually instead.

>   -- Keir
> 
>