Add memory pool and register memory regions only once. #3

asqasq · 2017-11-28T09:38:25Z

This pull request contains changes to improve the number of IOPS:

Register memory regions only once with the RDMA NIC (save resources and time on the hardware)
Add a memory pool out of which send and receive buffers can be allocated
Allow allocations either from the regular heap or from hugepages

In addition

Send and receive pipeline length are now independent.
Version increased to 1.4, because client and server endpoint group has a different signature

Note: This PR and the corresponding Crail PR have to merged together

PepperJo

The mempool seems complicated. Can we find a simpler solution or some library to handle this?

PepperJo · 2017-11-28T09:53:27Z

src/main/java/com/ibm/darpc/DaRPCEndpoint.java


 		/* Receive memory region is the first half of the main buffer. */
 		dataBuffer.limit(dataBuffer.position() + sendBufferOffset);
 		receiveBuffer = dataBuffer.slice();

 		/* Send memory region is the second half of the main buffer. */
 		dataBuffer.position(sendBufferOffset);
-		dataBuffer.limit(dataBuffer.position() + sendBufferOffset);
+		dataBuffer.limit(dataBuffer.capacity());


Is dataBuffer.capacity always == position + sendBufferOffset? Or could getWRBuffer return a buffer with higher capacity and just limit set to something lower

Yes, it is the same.

PepperJo · 2017-11-28T09:56:07Z

src/main/java/com/ibm/darpc/DaRPCEndpoint.java

@@ -56,7 +56,8 @@
 	private ConcurrentHashMap<Integer, SVCPostSend> pendingPostSend;
 	private ArrayBlockingQueue<SVCPostSend> freePostSend;
 	private AtomicLong ticketCount;
-	private int pipelineLength;
+	private int sendPipelineLength;
+	private int recvPipelineLength;


It looks like most of the private variables are set in the constructor. Can we make them final?

PepperJo · 2017-11-28T09:58:00Z

src/main/java/com/ibm/darpc/DaRPCServerEndpoint.java

@@ -14,6 +14,7 @@
 	private static final Logger logger = LoggerFactory.getLogger("com.ibm.darpc");

 	private DaRPCServerGroup<R, T> group;
+	private int eventPoolSize;


Again, can we make these private variables final wherever applicable?

PepperJo · 2017-11-28T10:00:25Z

src/main/java/com/ibm/darpc/DaRPCMemPool.java

+	final int defaultAllocationSize = 16 * 1024 * 1024; // 16MB
+	final int defaultMinAllocationSize = 4 * 1024; // 4KB
+	final int defaultAlignmentSize = 4 * 1024; // 4KB
+	final int defaultHugePageLimit = 0; // no huge pages by default


We can make the default arguments static

PepperJo · 2017-11-28T10:01:02Z

src/main/java/com/ibm/darpc/DaRPCMemPool.java

+	protected int allocationSize;
+	protected int minAllocationSize;
+	protected int alignmentSize;
+	protected int hugePageLimit;


PepperJo · 2017-11-28T10:08:36Z

src/main/java/com/ibm/darpc/DaRPCMemPool.java

+		if (isOpen) {
+			isOpen = false;
+			pdMemPool = null;
+			for (Iterator<IbvMr> it = mrs.iterator(); it.hasNext(); ) {


you can use for(IbvMr mr : mrs) { }

PepperJo · 2017-11-28T10:09:50Z

src/main/java/com/ibm/darpc/DaRPCMemPool.java

+		MappedByteBuffer mappedBuffer = null;
+		try {
+			mappedBuffer = channel.map(MapMode.READ_WRITE, 0,
+					allocationSize);


what about alignment here? We only know that it is at least hugepage aligned but what if the user wants a bigger alignment requirement?

PepperJo · 2017-11-28T10:12:37Z

src/main/java/com/ibm/darpc/DaRPCMemPool.java

+	final int defaultHugePageLimit = 0; // no huge pages by default
+
+	private HashMap<IbvPd, PdMemPool> pdMemPool; // One buddy allocator per protection domain
+	private LinkedList<IbvMr> mrs;


I prefer ArrayList over LinkedList and you can use the List interface here and choose your implementation in the constructor

PepperJo · 2017-11-28T10:17:59Z

src/main/java/com/ibm/darpc/DaRPCEndpoint.java

-		/* Only do one memory registration with the IB card. */
-		dataMr = registerMemory(dataBuffer).execute().free().getMr();
+		try {
+			dataBuffer = rpcGroup.getWRBuffer(this, sendPipelineLength * rawBufferSize + recvPipelineLength * rawBufferSize);


I don't understand this. Why make the mempool so complicated but than only get one big chunk out of it and slice them yourself?

Ok, I get that each connection might have different send/recv queue size, but at least in the group they always have the same so it might make sense to push the mempool instanciation to the group and simplify it. What do you think?

Right, the idea behind this implementation is that the memory pool registers chunks of memory with the card end every endpoint/every QP allocates a pieces of this already registered memory chunk.

The reason, why it is not instantiated in the group is to make the application (Crail) able to read properties, instantiate and "configure" the mempool and then pass to the DaRPC library.

I removed this allocator. Instead we use a simple one with fixed blocksize.

PepperJo · 2017-11-28T10:18:26Z

src/main/java/com/ibm/darpc/DaRPCMemPool.java

+		pdm.freeBuddies.get(size >> 1).add(bi2);
+
+		return true;
+	}


This all looks quite complicated. See comment above.

patrickstuedi · 2017-11-28T10:34:12Z

src/main/java/com/ibm/darpc/DaRPCEndpointGroup.java


 	public static int getVersion(){
 		return DARPC_VERSION;
 	}	

-	protected DaRPCEndpointGroup(DaRPCProtocol<R,T> protocol, int timeout, int maxinline, int recvQueue, int sendQueue) throws Exception {
+	protected DaRPCEndpointGroup(DaRPCProtocol<R,T> protocol, DaRPCMemPool memPool, int timeout, int maxinline, int recvQueue, int sendQueue) throws Exception {


The part I'm probably missing here, but why don't we have a simple interface defining a DaRPCMemPool, and then have applications pass any implementation of that interface while also providing a default implementation within DaRPC. Currently it looks like DaRPCMemPool is both the new MemPool type and the only implementation. Or I'm I missing something?

I agree. I changed it. There is an interface now and the application can instantiate a specific implementation of this interface.

patrickstuedi

I'm in favor of decoupling interface and implementation for the MemPool. Then, making the default implementation simple and tailored to the existing use cases also might make sense.

…ent send/receive queue lengths.

asqasq · 2017-12-13T21:31:57Z

DaRPC uses now a very simple memory pool, which implements a simple interface.

Most of the comments were about the memory allocator. I fixed most of them, but finally removed the memory allocator.

Please have a look at the newest version.

PepperJo

Some minor things otherwise looks good!

PepperJo · 2017-12-14T09:09:44Z

src/main/java/com/ibm/darpc/DaRPCMemPoolImpl.java

+		this.alignmentSize = 0;
+		this.hugePagePath = hugePagePath;
+
+		this.access = IbvMr.IBV_ACCESS_LOCAL_WRITE | IbvMr.IBV_ACCESS_REMOTE_WRITE | IbvMr.IBV_ACCESS_REMOTE_READ;


You can call the other constructor with this(hugePagePath, defaultAllocationSize, 0)

PepperJo · 2017-12-14T09:10:42Z

src/main/java/com/ibm/darpc/DaRPCMemPoolImpl.java

+		if (hugePagePath == null) {
+			System.out.println("Hugepage path must be set");
+			throw new IOException("Hugepage path must be set");
+		}


We should check this in the constructor and e.g. throw an IllegalArgumentException

PepperJo · 2017-12-14T09:15:52Z

src/main/java/com/ibm/darpc/DaRPCMemPoolImpl.java

+					throw new IOException("Requested size does not match block size managed by memory pool.");
+				}
+			}
+			r = freeList.removeFirst();


This throws a NoSuchElementException if the freeList is empty: https://docs.oracle.com/javase/7/docs/api/java/util/LinkedList.html#removeFirst()
This is not obvious from the interface description.

Yes, I agree. Changed interface showing that it throws a NoSuchElementException.

…onstructors and throw NoSuchElementException, if no more memory blocks are available.

asqasq · 2017-12-14T11:00:34Z

Addressed Jonas' comments and uploaded new version of the code to the PR. Please have a look. Thanks Adrian

PepperJo

Looks good!

patrickstuedi

Does MemPoolImpl write to stdout? All DaRPC code should only write to the logger, I think.

PepperJo

I think that we are moving in the right direction. I just have some minor comments about the design.

PepperJo · 2018-01-18T10:38:35Z

src/main/java/com/ibm/darpc/DaRPCMemPoolImpl.java

+	private ConcurrentHashMap<Long, IbvMr> memoryRegions;
+	private int access;
+	private DaRPCEndpointGroup<E,R,T> endpointGroup;
+	private ConcurrentHashMap<IbvPd, LinkedBlockingQueue<ByteBuffer>> pdMap;


IMO we should pull this out and have a per PD memory pool in the endpoint group. And use a factory to create the memory pools whenever we need to create a new PD. This also simplifies the interface since you do not need to pass the endpoint to the memory pool.

PepperJo · 2018-01-18T10:43:09Z

src/main/java/com/ibm/darpc/DaRPCMemPoolImpl.java

+	}
+
+	@Override
+	public ByteBuffer getBuffer(RdmaEndpoint endpoint) throws IOException, NoSuchElementException {


How about a new buffer type with the following methods added compared to the ByteBuffer: getRemoteKey(), getLocalKey() and a free() method which allows to return the buffer to the pool. This way we don't need to use hashmaps to find buffers when we free them and there is no uncertainty to which pool a buffer belongs to. I understand that this has some memory overhead but I believe that it outweighs the above.

PepperJo · 2018-01-18T10:44:59Z

src/main/java/com/ibm/darpc/DaRPCMemPoolImpl.java

+					+ ", alignmentSize = " + alignmentSize
+					+ ", currentAllocationSize = " + currentAllocationSize
+					+ ", allocationLimit = " + allocationLimit);
+			throw new IOException("Out of memory. Cannot allocate more buffers from hugepages."


Same string twice -> put it in variable

PepperJo · 2018-01-18T11:29:41Z

src/main/java/com/ibm/darpc/DaRPCMemPoolImpl.java

+		} catch (IOException e) {
+			logger.error("Could not set allocation length of mapped random access file on huge page directory.");
+			logger.error("allocaiton size = " + allocationSize + " , alignment size =  " + alignmentSize);
+			logger.error("allocation size and alignment must be a multiple of the hugepage size.");


multiple calls to the logger or append everything like above? I think we should be consistent.

PepperJo suggested changes Nov 28, 2017

View reviewed changes

patrickstuedi self-assigned this Nov 28, 2017

patrickstuedi reviewed Nov 28, 2017

View reviewed changes

patrickstuedi requested changes Nov 28, 2017

View reviewed changes

asqasq added 2 commits December 13, 2017 11:36

Add memory pool and register memory regions only once. Allow independ…

a7daf82

…ent send/receive queue lengths.

Added MemPool interface and added a simple version of the mempool.

cc896b6

asqasq force-pushed the iopsimprovements branch from 04e1ce7 to cc896b6 Compare December 13, 2017 11:52

Allocate always from hugepages.

7e06fc2

PepperJo suggested changes Dec 14, 2017

View reviewed changes

Check for hugepage path in constructor, remove duplicated code form c…

d88de1f

…onstructors and throw NoSuchElementException, if no more memory blocks are available.

PepperJo approved these changes Dec 14, 2017

View reviewed changes

patrickstuedi requested changes Dec 14, 2017

View reviewed changes

asqasq added 2 commits January 11, 2018 11:07

Simplified MemPoolImpl.java

e7a3c6d

Simplified mempool

bf52e02

patrickstuedi approved these changes Jan 18, 2018

View reviewed changes

PepperJo reviewed Jan 18, 2018

View reviewed changes

Add memory pool and register memory regions only once. #3

Are you sure you want to change the base?

Add memory pool and register memory regions only once. #3

Conversation

asqasq commented Nov 28, 2017

PepperJo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PepperJo Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickstuedi left a comment

Choose a reason for hiding this comment

asqasq commented Dec 13, 2017

PepperJo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asqasq commented Dec 14, 2017

PepperJo left a comment

Choose a reason for hiding this comment

patrickstuedi left a comment

Choose a reason for hiding this comment

PepperJo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PepperJo left a comment •

edited

Loading

PepperJo Nov 28, 2017 •

edited

Loading