Messages in this thread | | | Date | Thu, 8 Jun 2006 01:17:44 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH v2 4/7] AMSO1100 Memory Management. |
| |
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.
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.
> +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?
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.
> +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.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |