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

[Roland Dreier]

    Gleb> Hello I've sent this mail a week ago and had no response. I
    Gleb> hope this is not because of lack of interest in user space
    Gleb> support :) Anyway I repost it one more time to get the
    Gleb> feedback and to find the way to include this patch to openib
    Gleb> ASAP and not wait till madvise defines will propogate to
    Gleb> libc.

Sorry, I missed it the first time around.

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.

 > --- 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?

 > --- 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?

 >  	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.

 - R.