Initial data structures and functionality for mbox_allocator. #133

bturrubiates · 2015-04-30T20:56:20Z

This is open for discussion and so @hppritcha has an interface to work with.

Thoughts?

Signed-off-by: Ben Turrubiates [email protected]

hppritcha · 2015-05-01T21:21:43Z

prov/gni/include/gnix_mbox_allocator.h

+ * @param alloc_handle	IN	Gnix_mbox_alloc_handle to use as allocator.
+ * @param ptr		IN/OUT	Pointer that contains allocated gnix_mbox.
+ */
+int mbox_alloc(struct gnix_mbox_alloc_handle *alloc_handle,


I think we'll want to feed a pointer to a pointer for mbox_alloc. struct gnix_mbox **ptr

Good catch. I'll fix that.

Shouldn't it be "struct gnix_mbox *ptr"? The interface fills out a descriptor allocated by the user.

I'm not sure, it could really go either way.... @hppritcha

jswaro · 2015-05-03T05:56:00Z

Looks fine to me. Do we have a need for more extended set of calls than just alloc and free? Would it be beneficial to provide a call for allocating blocks of mailbox space at once?

I have some implementation related questions that we can defer to another issue.

bturrubiates · 2015-05-04T15:22:59Z

@jswaro I think the initial plan was that a couple of blocks would be mmaped up front in mbox_allocator_create and mbox_alloc would just partition it up hand out the handles.

hppritcha · 2015-05-04T15:27:13Z

@jswaro for IAA did you ever allocate blocks of mailboxes at once?

jswaro · 2015-05-04T15:29:19Z

@bturrubiates
I understand that they will be allocated in chunks as a slab, but will there be occasions where you might do mbox_alloc repeatedly within the same function call, for which a calloc-esque call would be beneficial?

@hppritcha We allocated blocks of mailboxes internally, but never externally. This was done to save on MDD registrations. I'm assuming this would be analogous to allocating a new slab within the allocator.

hppritcha · 2015-05-04T15:56:58Z

2015-05-04 9:29 GMT-06:00 James Swaro [email protected]:

@hppritcha https://github.com/hppritcha We allocated blocks of
mailboxes internally, but never externally. This was done to save on MDD
registrations. I'm assuming this would be analogous to allocating a new
slab within the allocator.

yes that's the basic reason for allocation by slab.

—
Reply to this email directly or view it on GitHub
#133 (comment)
.

bturrubiates · 2015-05-04T16:05:37Z

@jswaro
I'm not sure I understand. What do you mean by a 'calloc-esque' call?

ztiffany · 2015-05-04T16:09:53Z

prov/gni/src/gnix_mbox_allocator.c

+	return 0;
+}
+
+int mbox_free(struct gnix_mbox *ptr)


I don't see anything in gnix_mbox that allows you to look up the correct slab to free.

If I understand you correctly, you could use the memory handle and the base pointer.

We'd have to have some global list of mbox allocators then, right? I'd recommend passing the mbox allocator handle to mbox_free().

Oh, I get what you're saying now. Yeah I guess that detail was overlooked.

Actually that requires keeping track of mailbox <-> allocator at a layer above this. Do we just want to shove a pointer to the allocator inside of each mailbox? It loses 8 bytes per mailbox, but is worth it to do that rather than rely on book keeping at the layer above?

@hppritcha @ztiffany

I think having a pointer to the allocator would be ideal. It makes the free call more simple and we could catch invalid frees by checking for a NULL allocator field.

jswaro · 2015-05-04T17:19:49Z

By calloc-esque, I'm referring to the possibility of allocating several mailboxes within a single call instead of multiple calls.

Do we ever see a case where we might do the following?

for (i = 0; i < num_ep; ++i) {
    mbox_alloc(alloc_handle, &ep->mbox);
}

bturrubiates · 2015-05-04T17:26:37Z

@jswaro I'm not sure. I had asked @hppritcha about usage and whether we want to have a count parameter, but I don't think it was in his use cases.

hppritcha · 2015-05-04T17:27:50Z

not at the moment, but maybe sometime in the future?

hppritcha · 2015-05-04T17:32:48Z

I don't think we need such a function right now though.

bturrubiates · 2015-05-07T00:22:59Z

@ztiffany @jswaro @hppritcha
Ready for review.

hppritcha · 2015-05-07T03:21:19Z

prov/gni/src/gnix_mbox_allocator.c

+
+	*filename = full_filename;
+
+	file_id++;


sounds a little crazy but could we make file_id an atomic counter?

Sure. I actually should probably go do a round of updates and use strtok_r, strerror_r, and update file_id to be an atomic counter.

bturrubiates · 2015-05-07T17:08:13Z

I think this is ready for a second look: @jswaro @hppritcha

jswaro · 2015-05-07T17:48:41Z

prov/gni/src/gnix_mbox_allocator.c

+	return ret;
+}
+
+static int __find_free(struct gnix_mbox_alloc_handle *handle,


Nice usage.

jswaro · 2015-05-07T18:02:45Z

prov/gni/test/allocator.c

+	ret = gnix_mbox_allocator_destroy(allocator);
+	assert_eq(ret, -FI_EINVAL,
+		  "gnix_mbox_allocator_destroy succeeded on NULL handle.");
+}


A few more tests would be prudent:

mbox_exhaustion -- can we test if a new slab has been allocated when resources are exhausted? mbox_allocation -- can we allocate multiple mboxes and ensure they are all different? mbox_free_err -- can we free a mailbox twice and get the proper return code ? mbox_free_array -- can we allocate multiple mailboxes, perhaps off different slabs, and free them all without error? so on and so forth...

bturrubiates · 2015-05-07T22:11:53Z

@jswaro @ztiffany @hppritcha
Ready again

bturrubiates · 2015-05-07T23:06:29Z

Whenever this is reviewed, I'll squash into less commits.

jswaro · 2015-05-07T23:55:21Z

Looks good.

hppritcha · 2015-05-08T02:56:14Z

looks good to me too!

Implemented: - gnix_mbox_allocator_create - gnix_mbox_allocator_destroy - gnix_mbox_alloc - gnix_mbox_free - Add criterion tests to ensure that creation of allocator works with various page sizes. - Add a test to allocate multiple mailboxes and ensure they are different. - Add tests to make sure failure cases work properly. - Add tests to make sure that an extra slab is created when one has been exhausted. Signed-off-by: Ben Turrubiates <[email protected]>

bturrubiates · 2015-05-08T16:06:46Z

@hppritcha I'm seeing this:

[----] ../prov/gni/test/datagram.c:401: Assertion failed: cm_nic->control_progress == FI_PROGRESS_MANUAL
[====] dg_allocation::dgram_wc_post_exchg_manual: (0.00s)
[====] Synthesis: Tested: 130 | Passing: 129 | Failing: 1 | Crashing: 0

~~Is that normal?~~
Opened issue #152.

Initial data structures and functionality for mbox_allocator.

Break out option parsing for client-server tests into common code.

bturrubiates force-pushed the topic/mbox-allocator branch from 0f5b12a to 21f36ef Compare April 30, 2015 22:30

hppritcha reviewed May 1, 2015
View reviewed changes

bturrubiates force-pushed the topic/mbox-allocator branch from 21f36ef to 8cda423 Compare May 4, 2015 15:25

bturrubiates force-pushed the topic/mbox-allocator branch 2 times, most recently from 4c99bd0 to 0bfad1e Compare May 4, 2015 15:41

ztiffany reviewed May 4, 2015
View reviewed changes

bturrubiates force-pushed the topic/mbox-allocator branch 2 times, most recently from 18d2a9b to 3d52393 Compare May 4, 2015 16:21

bturrubiates force-pushed the topic/mbox-allocator branch from 3d52393 to cb6d9b3 Compare May 4, 2015 17:36

hppritcha reviewed May 7, 2015
View reviewed changes

jswaro reviewed May 7, 2015
View reviewed changes

prov/gni/src/gnix_mbox_allocator.c

return ret;

}

static int __find_free(struct gnix_mbox_alloc_handle *handle,

Copy link

Member

jswaro May 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice usage.

jswaro reviewed May 7, 2015
View reviewed changes

bturrubiates force-pushed the topic/mbox-allocator branch from 4fb6273 to cd01dfc Compare May 7, 2015 22:07

bturrubiates force-pushed the topic/mbox-allocator branch from c026454 to 9ef1375 Compare May 8, 2015 16:06

bturrubiates added a commit that referenced this pull request May 8, 2015

Merge pull request #133 from bturrubiates/topic/mbox-allocator

ab90252

Initial data structures and functionality for mbox_allocator.

bturrubiates merged commit ab90252 into ofi-cray:master May 8, 2015

bturrubiates mentioned this pull request May 8, 2015

need large page slab allocator that registers memory against gnix_nic's #98

Closed

bturrubiates deleted the topic/mbox-allocator branch May 29, 2015 18:27

jswaro pushed a commit to jswaro/libfabric that referenced this pull request Sep 11, 2018

Merge pull request ofi-cray#133 from pmmccorm/master

926344c

Break out option parsing for client-server tests into common code.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial data structures and functionality for mbox_allocator. #133

Initial data structures and functionality for mbox_allocator. #133

bturrubiates commented Apr 30, 2015

hppritcha May 1, 2015

bturrubiates May 1, 2015

ztiffany May 4, 2015

bturrubiates May 4, 2015

jswaro commented May 3, 2015

bturrubiates commented May 4, 2015

hppritcha commented May 4, 2015

jswaro commented May 4, 2015

hppritcha commented May 4, 2015

bturrubiates commented May 4, 2015

ztiffany May 4, 2015

bturrubiates May 4, 2015

ztiffany May 4, 2015

bturrubiates May 4, 2015

bturrubiates May 4, 2015

jswaro May 4, 2015

jswaro commented May 4, 2015

bturrubiates commented May 4, 2015

hppritcha commented May 4, 2015

hppritcha commented May 4, 2015

bturrubiates commented May 7, 2015

hppritcha May 7, 2015

bturrubiates May 7, 2015

bturrubiates commented May 7, 2015

jswaro May 7, 2015

jswaro May 7, 2015

bturrubiates commented May 7, 2015

bturrubiates commented May 7, 2015

jswaro commented May 7, 2015

hppritcha commented May 8, 2015

bturrubiates commented May 8, 2015

Initial data structures and functionality for mbox_allocator. #133

Initial data structures and functionality for mbox_allocator. #133

Conversation

bturrubiates commented Apr 30, 2015

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

jswaro commented May 3, 2015

bturrubiates commented May 4, 2015

hppritcha commented May 4, 2015

jswaro commented May 4, 2015

hppritcha commented May 4, 2015

bturrubiates commented May 4, 2015

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

jswaro commented May 4, 2015

bturrubiates commented May 4, 2015

hppritcha commented May 4, 2015

hppritcha commented May 4, 2015

bturrubiates commented May 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bturrubiates commented May 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bturrubiates commented May 7, 2015

bturrubiates commented May 7, 2015

jswaro commented May 7, 2015

hppritcha commented May 8, 2015

bturrubiates commented May 8, 2015