USB virt 2.6 split driver---xenidc buffer resource provider
PATCH 2/17 USB virt 2.6 split driver---xenidc buffer resource provider
This patch implements a resource pool for buffer resources used by the xenidc transport which is in turn used by the USB driver.
linux-2.6-xen-sparse/drivers/xen/xenidc/Makefile +1 -0
@@ -3,3 +3,4 @@ xenidc-objs = xenidc-objs += xenidc_callback.o xenidc-objs += xenidc_work.o +xenidc-objs += xenidc_buffer_resource_provider.o
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +443 -0
443 lines not on this page · raw message
linux-2.6-xen-sparse/include/asm-xen/xenidc_buffer_resource_provider.h new file +138 -0
138 lines not on this page · raw message
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 69
Quoted 5 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +5
-0+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.
Review
Quoted 1 line
(quoted diff) +1
-0+ {
No internal blocks please.
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 122
Quoted 6 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +6
-0+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).
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 127
Quoted 4 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +4
-0+if (provider->page ==NULL ) { + return_value = -ENOMEM ; + + goto EXIT_NO_EMPTY_PAGE_RANGE;
common style is not to put the label in all uppercaps.
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 226
Quoted 4 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +4
-0+if (xenidc_buffer_resource_provider_init_or_exit + (provider,0 ) + !=0 ) { +vfree (provider);
using vmalloc/vfree is discouraged unless you must.
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 242
Quoted 7 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +7
-0+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?
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 261
Quoted 11 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +11
-0+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?
linux-2.6-xen-sparse/include/asm-xen/xenidc_buffer_resource_provider.h line 99
Quoted 2 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_buffer_resource_provider.h new file +2
-0+typedef struct xenidc_buffer_resource_provider_struct +xenidc_buffer_resource_provider ;
don't typedef structs.
Cheers,
Muli
Signature · 2 lines
Muli Ben-Yehuda http://www.mulix.org | http://mulix.livejournal.com/
Quoted 7 lines · Muli Ben-Yehuda
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 69
Quoted 5 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +5
-0+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.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +1
-0+ {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.
Quoted 9 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 122
Quoted 7 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +6
-0+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.
Quoted 8 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 127
Quoted 5 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +4
-0+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.
Quoted 6 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 226
Quoted 4 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +4
-0+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.
Quoted 10 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 242
Quoted 8 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +7
-0+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_exitfails won't you vfree an already freed pointer here?
No, init_or_exit never fails on the exit path so the code is correct.
Quoted 16 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 261
Quoted 12 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +11
-0+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.
Quoted 5 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_buffer_resource_provider.h line 99
Quoted 3 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_buffer_resource_provider.h new file +2
-0+typedef struct xenidc_buffer_resource_provider_struct +xenidc_buffer_resource_provider ;don't typedef structs.
OK.
Quoted 3 lines
Cheers,
Muli
Signature · 1 line
Harry Butterworth <[email protected]>
Quoted 10 lines · Harry Butterworth
Quoted 6 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 226
Quoted 4 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +4
-0+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.
Quoted 6 lines
Quoted 3 lines
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
Signature · 2 lines
Muli Ben-Yehuda http://www.mulix.org | http://mulix.livejournal.com/
Quoted 9 lines · Muli Ben-Yehuda
Quoted 7 lines · Harry Butterworth
Quoted 6 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c line 226
Quoted 4 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_buffer_resource_provider.c new file +4
-0+if (xenidc_buffer_resource_provider_init_or_exit + (provider,0 ) + !=0 ) { +vfree (provider);using vmalloc/vfree is discouraged unless you must.
Quoted 8 lines
Quoted 3 lines
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
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
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.
Quoted 3 lines
Cheers,
Muli
Quoted 8 lines · Muli Ben-Yehuda
Quoted 3 lines
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
Quoted 3 lines · Keir Fraser
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
Signature · 2 lines
Muli Ben-Yehuda http://www.mulix.org | http://mulix.livejournal.com/
Quoted 7 lines · Muli Ben-Yehuda
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
Quoted 10 lines · Keir Fraser
Quoted 7 lines · Muli Ben-Yehuda
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."
Quoted 5 lines
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.
Quoted 3 lines
-- Keir