On Thu, May 11, 2006 at 09:19:13AM -0700, Roland Dreier wrote:
> In general this seems good, but I have a few quick comments:
>
> > +#ifndef MADV_DONTFORK
> > +#define MADV_DONTFORK 10
> > +#endif
> > +#ifndef MADV_DOFORK
> > +#define MADV_DOFORK 11
> > +#endif
>
> This should probably be in the only file that uses it, memory.c. And
> I think it's cleanest to use autoconf to check if MADV_DONTFORK and
> MADV_DOFORK are available.
I'll move this to memory.c. What about libmthca? Leave it in the header
there?
>
> > --- libibverbs/include/infiniband/verbs.h (revision 7112)
> > +++ libibverbs/include/infiniband/verbs.h (working copy)
> > @@ -289,6 +289,8 @@
> > uint32_t handle;
> > uint32_t lkey;
> > uint32_t rkey;
> > + void *addr;
> > + size_t length;
> > };
>
> This breaks ABI, right?
>
Yes. Absolutely. AFAIK you are going to remove libsysfs dependency and
break ABI with this change, I think we can piggyback this one then.
> > --- libmthca/src/verbs.c (revision 7112)
> > +++ libmthca/src/verbs.c (working copy)
> > @@ -134,6 +134,9 @@
> > return NULL;
> > }
> >
> > + mr->addr = addr;
> > + mr->length = length;
> > +
> > return mr;
> > }
>
> What's the reason to set addr and length here? Doesn't libibverbs
> already do it?
>
libmthca uses __mthca_reg_mr() to do internal registrations (qp, cq).
If every hw driver will set them we can remove this from libibverbs.
> > if (!cq->buf)
> > goto err;
> >
> > + madvise(cq->buf, cqe * MTHCA_CQ_ENTRY_SIZE, MADV_DONTFORK);
> > cq->mr = __mthca_reg_mr(to_mctx(context)->pd, cq->buf,
> > cqe * MTHCA_CQ_ENTRY_SIZE,
> > 0, IBV_ACCESS_LOCAL_WRITE);
> > @@ -247,6 +251,7 @@
> > mthca_dereg_mr(cq->mr);
> >
> > err_buf:
> > + madvise(cq->buf, cqe * MTHCA_CQ_ENTRY_SIZE, MADV_DOFORK);
> > free(cq->buf);
>
> It seems it would be better to put the DONTFORK call into
> mthca_alloc_cq_buf(), and the DOFORK into a new mthca_free_cq_buf() call.
>
> Actually, to handle the QP and SRQ cases too it's probably better to
> have wrappers for posix_memalign() and free() to keep this
> encapsulated in one place.
>
Will do.
--
Gleb.
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
