Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kqueue: add kqueue-based pollq implementation #204

Merged
merged 2 commits into from
Mar 2, 2018

Conversation

liamstask
Copy link
Contributor

@liamstask liamstask commented Jan 15, 2018

Initial cut at kqueue support for #35

This passes all tests on my machine, will see how it does on CI. Happy to rename/reorg as needed.

A couple thoughts:

  • since some of the poller API (add, remove, enable, disable) may involve syscalls, it would nice to return errors from those routines and handle them accordingly, possibly in a future PR
  • i wasn't totally clear on the expected handling of event flags - it looks like the existing poll implementation treats event flag manipulation as a cheap operation (it just manipulates userspace memory in the poll implementation), but since epoll and kqueue implementations might prefer to use the provided syscall interfaces to enable/disable events as distinct from adding/removing nodes, it could be nice to take that into consideration and minimize changes to enable/disable state.

@@ -103,7 +103,7 @@ nni_posix_epdesc_doconnect(nni_posix_epdesc *ed)
case 0:
// Success!
nni_posix_epdesc_finish(aio, 0, ed->node.fd);
ed->node.fd = -1;
// ed->node.fd = -1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not sure this is an appropriate change, but without it I was seeing the httpclient test fail during cleanup - because the fd was set to -1 but not closed, epdesc cleanup code doesn't close it (it's already -1) but the poller would deliver a callback after the epdesc had been cleaned up, causing an assert when accessing its mutex.

Please let me know if there's a better way to address this.

Relatedly, in nni_posix_epdesc_finish() I don't immediately see where the nni_posix_pipedesc that can be allocated there gets free'd - may be that i'm just not familiar with the ownership system there, but could possibly be leaking?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I may want to refactor some of this somewhat. Testing with helgrind is actually highlighting that the way we are tweaking the node.fd is actually a potential race.

So with respect to the pipedesc, if the pipedesc creation succeeds, then rv will be zero, and we will pass that out to the caller in the nni_aio_finish_pipe() logic. The aio callback then has the responsibility to "own" the pipe, and generally this causes the pipe to get associated with the socket. That logic is handled in the upper core code, endpt.c and pipe.c. I don't believe there are any leaks here, and valgrind at least agrees with me in that regard.

@codecov
Copy link

codecov bot commented Jan 15, 2018

Codecov Report

Merging #204 into master will increase coverage by 0.24%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #204      +/-   ##
=========================================
+ Coverage   77.75%     78%   +0.24%     
=========================================
  Files         155     155              
  Lines       13039   13041       +2     
=========================================
+ Hits        10138   10172      +34     
+ Misses       2901    2869      -32
Impacted Files Coverage Δ
src/transport/ipc/ipc.c 76.7% <33.33%> (+2.41%) ⬆️
src/protocol/pair0/pair.c 89.9% <0%> (-3.67%) ⬇️
src/transport/ws/websocket.c 61.11% <0%> (-2.34%) ⬇️
src/platform/posix/posix_pollq_poll.c 88.95% <0%> (-2.33%) ⬇️
src/platform/posix/posix_pipedesc.c 71.23% <0%> (-2.06%) ⬇️
src/transport/tcp/tcp.c 77.83% <0%> (-1.85%) ⬇️
src/supplemental/http/http_conn.c 75.42% <0%> (-0.29%) ⬇️
src/supplemental/http/http_server.c 66.66% <0%> (+0.27%) ⬆️
src/transport/inproc/inproc.c 82.6% <0%> (+0.54%) ⬆️
src/core/socket.c 88.73% <0%> (+0.7%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update feeec67...1a7ecdc. Read the comment docs.

CMakeLists.txt Outdated
@@ -324,6 +324,7 @@ macro (nng_check_sym SYM HDR DEF)
check_symbol_exists (${SYM} ${HDR} ${DEF})
if (${DEF})
add_definitions (-D${DEF}=1)
set (${DEF} ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be needed. CMake should already have set that with check_symbol_exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's needed in order to conditionalize other cmake inclusions, like the one in src/CMakeLists.txt - let me know if you'd prefer another approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me look at this in more detail -- I still think this is redundant unless I've misunderstood what check_symbol_exists does.

@gdamore
Copy link
Contributor

gdamore commented Jan 15, 2018

I need to review this, and probably won't have time to complete the review for a couple of days. At a high level, I suspect we will want to make some minor file reorganizations at least. (For example, I consider "bsd" as subset of posix, as it uses all the posix stuff for ~everything else, so I don't want to create a new subdirectory for it. Also you're already doing the right thing by guarding the code in the kqueue poller, so we might just add both files unconditionally, and assume that they got the guards set properly.)

More as I have time to read.

@liamstask
Copy link
Contributor Author

Sounds good. The reason i originally added files conditionally within cmake was the following (benign) warning:

[135/270] Linking C static library libnng_static.a
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libnng_static.a(posix_pollq_poll.c.o) has no symbols

Happy to use that approach if you prefer.

@gdamore
Copy link
Contributor

gdamore commented Jan 17, 2018

So the way I've solved that warning in the past is creating a bogus symbol, e.g. "int posix_pollq_kqueue_notused = 0;"

Now having said that, I am leaning more and more towards using CMake here like you've done, but I think I may want to move the "platform" logic into a new cmake tree. Stay tuned for that.

@gdamore
Copy link
Contributor

gdamore commented Jan 17, 2018

So the other thing is that I'm a bit uncomfortable with "ignoring" system calls, unless we are reasonably certain that they could not ever fail. Unfortunately, there are numerous reasons these calls can fail (ENOMEM, EACCES, EINTR) which the calling program may not have control over.

(I'm ok with ignoring failures from system calls that can only fail if we provide bogus input, like EFAULT due to passing a bad pointer -- since in theory we can reasonably be assured that our own code is not buggy in this regard.) Although in that case an assertion may be called for.

I need to think about the arm/disarm API, because those lack a way to pass event updates back. It would be better if we didn't need to pass such updates (having an API that is immune to failure is better!) but I'm not sure we can do that.

Stay tuned.

@liamstask
Copy link
Contributor Author

Agreed, same concern noted in the pr description above 😄

Also, I wonder if it would be worth reconsidering the current approach of disarming and re-arming nodes after each event - could save a bit of work by keeping them armed until they're explicitly requested to be disarmed, assuming that would be relatively infrequent (currently looks like it's just done during shutdown of an endpoint.

struct kevent event;

if ((rv = nni_plat_pipe_open(&pq->wakewfd, &pq->wakerfd)) != 0) {
return rv;
Copy link
Contributor

Choose a reason for hiding this comment

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

Stylistic issue here: I prefer return values always be surrounded by parens.

rv = kevent(pq->kq, kevents, nevents, NULL, 0, NULL);
if (rv < 0) {
// report error?
fprintf(stderr, "disarm err: %s (%d)\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

So it turns out that we currently only "disarm" events during "close". Apparently close() of a fd implicitly unregisters the associated kevents:

From kevent(2) on macOS:

Calling close() on a file descriptor will remove any kevents that reference the descriptor.

I'm looking at the other side next.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, what I mean here is that we should just ignore the return code, and inject a comment to this effect.

if (nevents > 0) {
int rv;
rv = kevent(pq->kq, kevents, nevents, NULL, 0, NULL);
if (rv < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this, I'm inclined to think what we ought to do is mark the event as if POLLERR was triggered. It would be even handier if we could stash the errno somewhere, or some indication that it was kevent that triggered the error. Stay tuned.

nni_mtx_lock(&pq->mtx);

if (nevents < 0) {
// report error?
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an unfortunate place to have this kind of error... I'm not sure how best to deal with it.... can this really fail? My read of the man page makes me thing any error other than EINTR here is impossible.

Copy link
Contributor

Choose a reason for hiding this comment

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

The more I think about this the more I think we need to have a new error flag or state on the node, and let the poller set this. If we have an error here, we would trigger that error on every item in the pollq.

This may help other poller implementations that have a need to report some other kind of weird failure scenario.

@liamstask
Copy link
Contributor Author

I started experimenting with a slightly different approach, using the following mapping:

  • EV_ADD -> nni_posix_pollq_add()
  • EV_DELETE -> nni_posix_pollq_remove()
  • EV_ENABLE -> nni_posix_pollq_arm()
  • EV_DISABLE -> nni_posix_pollq_disarm()

which arguably makes more sense. I think this would make a better case for ignoring errors on arm/disarm since those operations should essentially consist of setting a flag internally that should already be allocated.

one issue i quickly ran into - node->fd is 0 when nni_posix_pollq_add() is called, at least for tcp, such that the fd can't actually be added.

can special case this, of course, by checking for whether we've seen a valid fd by the time we're calling arm/disarm, but not ideal. if that's intended, do you have any suggestions for handling that?

@gdamore
Copy link
Contributor

gdamore commented Feb 15, 2018 via email

@gdamore
Copy link
Contributor

gdamore commented Feb 15, 2018

So I've gone through the FreeBSD code, and it appears that only EV_ADD can generate ENOMEM. The other documented errors seem to be impossible for correctly written code to cause ... (EBADF, EFAULT, EACCES, EINVAL, ENOENT, ESRCH) with the possible exception of EINTR. I need to go through your code and examine that more closely to see if it can happen.

The endpoint descriptors call posix_pollq_add to create the pollq node before the file descriptor is actually opened!

Probably this needs to be done as part of the posix_epdesc_listen or posix_epdesc_connect calls.
The alternative would be to open the underlying socket descriptor (unbound) during the epdesc initialization.

We don't have this problem with the pipedesc.

@gdamore
Copy link
Contributor

gdamore commented Feb 15, 2018

I'm going to try to change the code to ensure that we only call the _add() function when we have a corresponding file descriptor. Stay tuned.

@gdamore
Copy link
Contributor

gdamore commented Feb 15, 2018

Please have a look at the changes in the "epdesc" branch. This separates the pollq_add from initialization, so that pollq_add is only ever called when the node has a valid file descriptor. It also does some work to clean up the connect side, so that we do pollq_remove() when the connect logic finishes. (It will subsequently be re-added as part of a pipedesc rather than an endpoint desc.)

I think these changes, combined with your proposal to use EV_ENABLE and EV_DISABLE in the arm and disarm routines, should lead us to a nice solution.

@liamstask
Copy link
Contributor Author

just rebased on master & added a commit that implements the approach discussed above. (happy to squash/rebase as needed prior to merge.)

this passes all tests except ipc - i am still debugging an issue there, but thought i would push to continue the discussion and see if you had any pointers :)

issue is that i am seeing a sequence of calls for a given node:

  1. nni_posix_pollq_add()
  2. nni_posix_pollq_arm()
  3. [event is delivered]
  4. nni_posix_pollq_remove()
  5. nni_posix_pollq_arm() - this fails as the node has already been removed

all other tests are passing, so will continue to gather details on this and run it down.

@gdamore
Copy link
Contributor

gdamore commented Feb 21, 2018

I will have a look at the posix pollq_arm vs. not added flow... my guess there is that I botched something in the connection setup flow.

@@ -104,7 +104,7 @@ nni_posix_epdesc_doconnect(nni_posix_epdesc *ed)
// Success!
nni_posix_pollq_remove(&ed->node);
nni_posix_epdesc_finish(aio, 0, ed->node.fd);
ed->node.fd = -1;
// ed->node.fd = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets just remove it altogether, please.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I believe this code needs to stay the way it was. Now that we are removing the pollq, the value of the fd is -1, and this ensures that we do not attempt to reuse the same descriptor for connect, but instead allocate a new one. I'm wondering if this is the source of your grief about remove followed by arm...

// do getpeername().
// We could argue that knowing the remote peer address would
// be nice. But frankly if someone wants it, they can just
// do getpeername().
Copy link
Contributor

Choose a reason for hiding this comment

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

This goofy indentation (my old one I mean) may have been done automatically by clang-format. In which case it may wind up getting reverted later. I don't always agree 100% with the decisions it makes, but I find using it consistently means I don't have to get into debates about formatting style.

Still it isn't as aggressive about undoing changes as some other formatting tools, so its possible that this change won't get trashed. This is just a heads up in case it does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this more closely, we should remove this comment ... its no longer relevant due to other changes in the code.

#include "core/nng_impl.h"
#include "platform/posix/posix_pollq.h"

#if defined NNG_PLATFORM_NETBSD
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have this detected by a check, rather than using the platform. I'm not sure how you can test the type of a field member under cmake though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm yea - i just grabbed this from nanomsg & have not tested on net bsd specifically. i'll add a TODO comment and leave it as is for now.

nni_mtx_lock(&pq->mtx);
pq->close = 1;
pq->started = 0;
nni_plat_pipe_raise(pq->wakewfd);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can probably dispense with the wakewfd and wakerfd and the plat pipe logic. That logic exists because I don't have any other way to alert a pollq using poll to a change in the polled set of descriptors. I think kqueue doesn't suffer from this problem at all, since it uses explicit system calls for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

You do need to do something to cause the poll thread to notice and exit though, so maybe this pipe is still useful? Alternatively what happens if you just close the kqueue fd? Does the thread wake up? Is there some other convenient way to force an event via the kqueue system call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

closing the kqueue is enough to wake it up and exit - will remove wake fds for now, can always bring them back if needed.

// always add wake fd so we can be signaled
EV_SET(
&event, (uintptr_t) pq->wakerfd, EVFILT_READ, EV_ADD, 0, 0, NULL);
return kevent(pq->kq, &event, 1, NULL, 0, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: please use return (value) // note the parens

if ((rv = nni_plat_pipe_open(&pq->wakewfd, &pq->wakerfd)) != 0) {
return (rv);
}
if ((rv = nni_thr_init(&pq->thr, nni_posix_poll_thr, pq)) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should not be creating multiple of these threads, to increase scalability. I know with the posix poller I only create "one" at present, but I intended to scale that across multiple pollq instances. With kqueue it might make more sense to have just a single global pollq and run multiple thread instances. (Some assessment about lock contention may be desirable though -- having separate pollq and kqueue instances would eliminate any need for locking and increase locality I suppose.)

@gdamore
Copy link
Contributor

gdamore commented Feb 21, 2018

So I figured out the problem. I'll be pushing a fix, but the issue is that we sometimes close the descriptor from the callback. We therefore cannot automatically rearm, because it may have been closed (and removed). I'm also adding a check against events being 0, which is kind of redundant, but it can save some expensive operations (getting a lock) when we don't have anything new to arm.

@gdamore
Copy link
Contributor

gdamore commented Feb 21, 2018

Have a look at #242 -- I believe that this will address the problem you're seeing. I intend to merge that if CI comes back clean.

@gdamore
Copy link
Contributor

gdamore commented Feb 21, 2018

So, that worked, but now you'll have to rebase, sorry. Please also review my comments above -- you don't want to remove the reset of the node.fd for example.

The override of DEFS in CMake files should be removed -- I'm convinced it is redundant.

I'm ok with deferring any changes related to scaling to multiple threads, or the change to the wake pipe, until after this is merged. (I really hope we can ditch the wake pipes though -- I think they add complexity that its likely we do not need.)

We're really close I think.

@liamstask
Copy link
Contributor Author

alright - rebased on your latest changes and everything's looking 👌all tests are passing here on 10's of runs.

removed the wake fd handling and the cmake override - confirmed that cmake's check_symbol_exists creates the same variable already.

i also added a minor cleanup of ipc init that i stumbled across while debugging - not the source of the problem but seemed worth including.

@gdamore
Copy link
Contributor

gdamore commented Feb 23, 2018

This looks really quite good. I'm going to do some quick performance runs on my mac, but I expect to merge this shortly. Thanks for your hard work on this!

@gdamore
Copy link
Contributor

gdamore commented Feb 23, 2018

Well, my performance runs gave disappointing results. On the one hand I didn't really expect a boost because the test I have handy is single threaded, but I see a substantial performance regression in the throughput test (and a much smaller regression in the latency test.)

I think the problem is that the cost of the system calls is hurting us badly. I expect the code to scale better, but I want to eyeball this again to see if there is any place we can get some wins... one though that occurred to me is that if we use ONESHOT, maybe we can dispense with the calls to disable the node. Since we're doing this ourselves with a lock, we could easily keep track of the enabled vs. disabled state and simply not make the calls that we don't need.

The other thing to look at more closely is our use of the lock itself -- is it still needed? I doubt it has much affect (it shouldn't be contended), but every little bit counts.

If you get time to looks at this more closely, maybe you can give your thoughts.

One other idea might be to make the use of kqueue optional, so that users can fine tune for either single threaded/socket performance, or greater scalability, since that does seem to be the major tradeoff here.

@liamstask
Copy link
Contributor Author

hm! can you share any details on your perf test? if i can reproduce it here, i can try to profile a bit and see if there's any reasonably low hanging fruit.

at least according to comments in the header, EV_DISPATCH claims to disable the event after reporting (and subsequent calls to pollq_disable() should avoid the system call), but perhaps there are some other notification/filter options that might be advantageous? can explore a bit.

@liamstask
Copy link
Contributor Author

liamstask commented Feb 25, 2018

one problem i've seen so far is that there is sometimes nontrivial latency between the initial event notification (ie, event returned from kqueue) and when the actual io takes place (ie, readv()/writev() is called).

from running several of the tests, the worst instances i've seen are in the multistress test where 250 event notifications occur before the related io occurs.

this happens because the EV_DISPATCH flag, which we rely on to automatically disable the event, only disables the event after it's been ack'd by the user (ie, the fd has been read/written).

explicitly making a call to disable (via kevent()) prior to invoking the cb addresses this issue, at the cost of an additional syscall, but it may also suggest there's some other undesirable behavior (contention somewhere?) causing this delay in handling the reported events.

that said, not sure exactly what perf tests you are running, so difficult to say whether this could be related to the issues you've seen, but it certainly results in the system doing a lot of extra/wasted work.

let me know how you'd like to proceed - i can include the simple change that explicitly disables before callbacks and see if this improves the numbers that you see, or continue investigating.

@gdamore
Copy link
Contributor

gdamore commented Feb 26, 2018

So the performance test I ran was simply using the local_tput and remote_tput (and also the _lat) in the perf subdirectory. I compared a bunch of messages, with a message size of about 1000 bytes.

You can run those tests yourself and see what happens.

The long latency is very concerning. I think we should look hard at the EV_ONESHOT flag. That causes the event to only generate once.

@liamstask
Copy link
Contributor Author

liamstask commented Feb 26, 2018

I see similar behavior (ie, multiple notifications) with EV_ONESHOT. my reading of the man page on that is that it will behave similarly to EV_DISPATCH for the first event only, but instead of disabling the event after it's been consumed, it will delete it, such that it would need to be added again, rather than just re-enabling. maybe a closer look at the source is needed.

thanks for the pointer - i'll check out the perf tests and compare them to the poll() based implementation.

@liamstask
Copy link
Contributor Author

after some brief testing, it looks like this issue may indeed account for the drop in throughput. I did 2 runs each for the throughput and latency tests in the following configurations:

  • without the explicit disable after event notification (ie, the code as it existed at f9a81bd prior to the most recent 2 commits)
  • with the explicit disable (ie latest commit at 6854d8d)
  • with kqueue support disabled altogether, relying on poll()

obviously not statistically significant, but on first glance It looks like throughput of the explicit disable variant is more or less the same as poll(), and latency is slightly improved.

let me know if you're able to reproduce.

***************** kqueue() without explicit disable

total time: 0.052 [s]
message size: 1024 [B]
message count: 500
throughput: 9615 [msg/s]
throughput: 75.120 [Mb/s]

total time: 0.051 [s]
message size: 1024 [B]
message count: 500
throughput: 9804 [msg/s]
throughput: 76.593 [Mb/s]


total time: 0.115 [s]
message size: 1024 [B]
round trip count: 500
average latency: 115.000 [us]

total time: 0.115 [s]
message size: 1024 [B]
round trip count: 500
average latency: 115.000 [us]

***************** kqueue() with explicit disable

total time: 0.046 [s]
message size: 1024 [B]
message count: 500
throughput: 10870 [msg/s]
throughput: 84.918 [Mb/s]

total time: 0.046 [s]
message size: 1024 [B]
message count: 500
throughput: 10870 [msg/s]
throughput: 84.918 [Mb/s]


total time: 0.106 [s]
message size: 1024 [B]
round trip count: 500
average latency: 106.000 [us]

total time: 0.105 [s]
message size: 1024 [B]
round trip count: 500
average latency: 105.000 [us]

***************** poll()

total time: 0.045 [s]
message size: 1024 [B]
message count: 500
throughput: 11111 [msg/s]
throughput: 86.806 [Mb/s]

total time: 0.046 [s]
message size: 1024 [B]
message count: 500
throughput: 10870 [msg/s]
throughput: 84.918 [Mb/s]


total time: 0.107 [s]
message size: 1024 [B]
round trip count: 500
average latency: 107.000 [us]

total time: 0.117 [s]
message size: 1024 [B]
round trip count: 500
average latency: 117.000 [us]

@gdamore
Copy link
Contributor

gdamore commented Feb 27, 2018

Actually it looks like the latency with explicit disable dropped slightly? Anyway, as this is single threaded performance, as long as we are getting close to poll(), then I'm pretty happy. The purpose of kqueue() is not to make single threaded performance go faster (it doesn't), but to scale much better by elimination of expensive walks through long event lists that are required with poll().

I'll test this out myself and get back to you. If you rebase to HEAD, you can also eliminate your changes to the perf tool -- I have the same change in HEAD.

@liamstask
Copy link
Contributor Author

rebased on current master, squashed the explicit disable commit into the main kqueue commit & also added functionality to explicitly wake up poller (via EVFILT_USER) rather than the brute force approach of just close()ing the kqueue.

passing tests and perf appears to be similar to poll() - i believe it's ready to merge if perf looks ok on your end.

@gdamore
Copy link
Contributor

gdamore commented Mar 2, 2018

I'm happy with the test results. Thanks for doing this.

@gdamore gdamore merged commit 7f2b2f1 into nanomsg:master Mar 2, 2018
@gdamore
Copy link
Contributor

gdamore commented Mar 2, 2018

One thing I've belatedly noticed, is that you don't have a copyright notice on your work. I'd like to correct that, using the same license header, but your own copyright.

@gdamore
Copy link
Contributor

gdamore commented Mar 2, 2018

Its important that this work be licensed under MIT the same as the rest of the code.

I'd propose adding the following to the kqueue C file:

//
// Copyright 2018 Liam Staskawicz <[email protected]>
//
// This software is supplied under the terms of the MIT License, a
// copy of which should be located in the distribution where this
// file was obtained (LICENSE.txt).  A copy of the license may also be
// found online at https://opensource.org/licenses/MIT.
//

Additionally if you copy-pasted any code from the poll.c file, it should have Staysail and Capitar copyrights as well.

@liamstask liamstask deleted the kqueue branch March 3, 2018 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants