Re: [PATCH v2 4/7] AMSO1100 Memory Management.

[Tom Tucker <tom@opengridcomputing.com>]

On Thu, 2006-06-08 at 01:17 -0700, Andrew Morton wrote:
> On Wed, 07 Jun 2006 15:06:55 -0500
> Steve Wise <swise@opengridcomputing.com> wrote:
> 
> > 
> > +void c2_free(struct c2_alloc *alloc, u32 obj)
> > +{
> > +	spin_lock(&alloc->lock);
> > +	clear_bit(obj, alloc->table);
> > +	spin_unlock(&alloc->lock);
> > +}
> 
> The spinlock is unneeded here.

Good point.

> 
> 
> What does all the code in this file do, anyway?  It looks totally generic
> (and hence inappropriate for drivers/infiniband/hw/amso1100/) and somewhat
> similar to idr trees, perhaps.
> 

We mimicked the mthca driver. It may be code that should be replaced
with Linux core services for new drivers. We'll investigate.

> > +int c2_array_set(struct c2_array *array, int index, void *value)
> > +{
> > +	int p = (index * sizeof(void *)) >> PAGE_SHIFT;
> > +
> > +	/* Allocate with GFP_ATOMIC because we'll be called with locks held. */
> > +	if (!array->page_list[p].page)
> > +		array->page_list[p].page =
> > +		    (void **) get_zeroed_page(GFP_ATOMIC);
> > +
> > +	if (!array->page_list[p].page)
> > +		return -ENOMEM;
> 
> This _will_ happen under load.  What will the result of that be, in the
> context of thise driver?

A higher level object allocation will fail. In this case, a kernel
application request will fail and the application must handle the error.
> 
> This function is incorrectly designed - it should receive a gfp_t argument.
> Because you don't *know* that the caller will always hold a spinlock.  And
> GFP_KERNEL is far, far stronger than GFP_ATOMIC.

This service is allocating a page that the adapter will DMA 2B message
indices into. 
> 
> > +static int c2_alloc_mqsp_chunk(gfp_t gfp_mask, struct sp_chunk **head)
> > +{
> > +	int i;
> > +	struct sp_chunk *new_head;
> > +
> > +	new_head = (struct sp_chunk *) __get_free_page(gfp_mask | GFP_DMA);
> 
> Why is __GFP_DMA in there?  Unless you've cornered the ISA bus infiniband
> market, it's likely to be wrong.
> 

Flag confusion about what GFP_DMA means. We'll revisit this whole
file ... 

> 
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html