On Thu, 2006-06-08 at 01:17 -0700, Andrew Morton wrote: > On Wed, 07 Jun 2006 15:06:55 -0500 > Steve Wise <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html