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 linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
