[openib-general] [resend][RFC][PATCH] adding call to madvise

[Gleb Natapov]

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.