Xen devel - Nov 2005 - [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.

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



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

> +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/


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

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>


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

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/


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

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

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

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


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

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/


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

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


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

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
> 
> 

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