support for linux scheduler and timer #64

neo5167 · 2018-01-27T18:14:10Z

Linux support for scheduler and timer with epoll, timerfd, eventfd, and other such sundry stuff. I am restructuring the code with less ifdefs. But I want to do that with guidance from you guys. Also, its a fork of an older version. I am working rebasing it to something more recent as well. In this PR, please comment on how to structure the code so that we can move platform dependent stuff into something more manageable.

I will move over to using std::invoke_result_t once I have confirmed that it is supported by clang/libc++ as well.

lewissbaker

@neo5167 Thanks very much for the PR.

I've had a read over the code and made some notes on some issues I found.

I think some of the functions with #ifdefs are a bit messy where we're trying to keep some common logic but then also having Linux-specific and Windows-specific parts. I wonder whether it would be more maintainable to put the #ifdefs outside of the function for some of these functions and cop a little bit of duplication between Windows/Linux versions in return for more readable code.

I'm also considering separating out the platform-specific bits into separate *_win32.cpp or *_linux.cpp files for some abstractions rather than sprinkling #ifdef around everywhere. It may require some refactoring to get there, though, so we can move towards that direction in time.

lewissbaker · 2018-02-05T11:55:26Z

lib/linux.cpp

+      {
+	m_mqdt = -1;
+	uuid_t unique_name;
+        const char* cppcoro_qname_prefix = "/cppcoro-";


It's a pity the Linux API doesn't provide a way to create anonymous message queues...

lewissbaker · 2018-02-05T11:59:05Z

lib/linux.cpp

+	for(;;)
+	  {
+	    uuid_generate(unique_name);
+	    uuid_unparse(unique_name, m_qname + sizeof(cppcoro_qname_prefix));


This sizeof(cppcoro_qname_prefix) is really just doing a sizeof(const char*) which will always be 4 or 8.
I assume you meant for this to be using strlen(cppcoro_qname_prefix)?
Or perhaps declare cppcoro_qname_prefix to have type const char[10]?

lewissbaker · 2018-02-05T12:05:55Z

lib/linux.cpp

+      {
+	close(m_epollfd);
+	assert(mq_close(m_mqdt) == 0);
+	assert(mq_unlink(m_qname) == 0);


Won't these calls to mq_close and mq_unlink be compiled out under optimised builds when NDEBUG is defined?

Can the calls be moved out of the assert() statements and just assert() the comparison of the result.

lewissbaker · 2018-02-05T12:55:13Z

lib/linux.cpp

+	  }
+
+	message qmsg;
+	ssize_t status = mq_receive(m_mqdt, (char*)&qmsg, sizeof(message), NULL);


I think there might be a possible race condition here if multiple threads concurrently call dequeue_message() that could cause one of the threads to block on the mq_receive call.
eg. consider if there is a single item in the queue and this signals the epoll handle. Both threads enter dequeue_message() and the call to epoll_wait() returns immediately with 1 for both threads since the message queue file descriptor is ready. The first thread to call mq_receive() dequeues the only message and the second thread to call mq_receive() blocks, waiting for another item to be queued to the message queue.

This may not be an issue in practice for calls that pass true to the wait parameter, but could lead to long delays for calls that pass false to concurrent calls.

lewissbaker · 2018-02-05T13:10:16Z

test/scheduling_operator_tests.cpp

+
+#if CPPCORO_OS_LINUX
+#define THREAD_ID unsigned long long
+#define GET_THIS_THREAD_ID get_thread_id()


Why are all of these macros necessary?

As far as I can see, all of the usages down below were previously just comparing std::thread::id values, so I don't quite follow why we need to try and convert the thread id into an unsigned long long.
I would have thought that std::this_thread::get_id() would work just fine.

Is it an issue with unresolved symbols for the operator<< for std::thread::id due to the use of the doctest CHECK() macros?

I'd really rather avoid use of macros like this here if possible.

If it's really still needed then perhaps just use a typdef and write a local get_thread_id() function for all platforms?

Sadly, on Linux thread id's are are not comparable directly. Have to convert to unsigned long long first to be able to compare. I will remove the macros and add functions instead.

lewissbaker · 2018-02-12T20:48:39Z

include/cppcoro/detail/linux.hpp

+#include <sys/epoll.h>
+#include <unistd.h>
+
+typedef int DWORD;


I'd like to avoid adding new global-scope typedefs and #defines like this in public headers.
I don't want to introduce any issues or potential conflicts with other libraries that may be used in conjunction with cppcoro.

It looks like this is only required for timeout values inside cppcoro::io_service::timer_thread_state::run(), so maybe its definition could be localised to that file?

Makes sense. Fixed it.

lewissbaker · 2018-02-12T20:51:02Z

include/cppcoro/detail/linux.hpp

@@ -0,0 +1,63 @@
+#pragma once


Can you change this to use #ifdef include guards to be consistent with other headers?

Can you also add a copyright/license header as per the existing source files?

Please attribute copyright to the appropriate entity.

Will have to circle back on this. Have to speak with our team here to get the appropriate rights entity name.

lewissbaker · 2018-02-13T05:11:42Z

include/cppcoro/io_service.hpp

@@ -76,8 +84,8 @@ namespace cppcoro
 		template<typename REP, typename PERIOD>
 		[[nodiscard]]
 		timed_schedule_operation schedule_after(
-			const std::chrono::duration<REP, PERIOD>& delay,


There are a number of unnecessary whitespace changes in these commits.

Have you tried running clang-format over the files?

Will circle back on this. I am fixing my editor to have the same whitespace language as yours.

Alright. Things are now "tab"-bed according to clang-format-6.0.el from my emacs. That fixed it.

lewissbaker · 2018-02-13T11:56:23Z

lib/linux.cpp

+				uuid_t unique_name;
+				const char* cppcoro_qname_prefix = "/cppcoro-";
+
+				if(NAME_MAX < UUID_STRING_SIZE + sizeof(cppcoro_qname_prefix) + 1)


The sizeof(cppcoro_qname_prefix) here will be sizeof(const char*) and not sizeof("/cppcoro-").
I assume that was not the intent here?

Yikes. Meant strlen. Good catch!

lewissbaker · 2018-02-13T12:00:32Z

lib/linux.cpp

+					attr.mq_msgsize = sizeof(cppcoro::detail::linux::message);
+					attr.mq_curmsgs = 0;
+
+					m_mqdt = mq_open((const char*)m_qname, O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, S_IRWXU, &attr);


I don't think the (const char*)m_qname cast is necessary here.
There should be an implicit decay cast from char[NAME_MAX] to const char* possible here.

The clang mainline has now been branched for clang-6 and mainline now uses clang-7 version.

neo5167 · 2018-02-20T18:30:39Z

@lewissbaker Cool. Thanks for the comments. Working on the easiest ones now. For the more complex ones, let's chat in the comments on how to proceed.

lewissbaker · 2018-03-05T05:08:40Z

@neo5167 How are you going with the changes? Any questions I can help you with?

neo5167 · 2018-03-06T18:24:50Z

@lewissbaker Sorry. Progress has been a little slow. I am hoping to send another commit in the next several days. Plan is to fix the comments by end of this month.

If the library is used as part of a bigger project some definitions can be provided via external build files.

Remove possible warnings with redefinitions of WIN32_LEAN_AND_MEAN.

Several tests are currently failing due to what seems to be compiler bugs. Ignore tests for now and revisit once MSVC 15.7 is released.

Update appveyor.yml to ignore x86 optimised failures

Correct file_write_operation's friend class

…ssbaker#75)

…rors under Linux (lewissbaker#78)

The AppVeyor 'Visual Studio 2017 Preview' image has now been updated with VS 2017.7 Preview 2

The msvcurt[d].lib is actually intended for C++/CLI usage and seems to no longer being bundled with the default VS install in more recent Visual Studio 2017 installs. Should fix lewissbaker#73

neo5167 · 2018-04-11T21:24:50Z

@lewissbaker take a look at the latest commit. Also, any ideas on how we should proceed with the whole ifdef thing? Is the main branch still ifdef based or have you already structured it more cleanly?

neo5167 · 2018-04-16T18:18:13Z

@lewissbaker I was thinking of working on file support at a later time but push this PR through first. What do you think? If that is ok then I will work on rebasing with the main... let me know...

lewissbaker and others added 4 commits January 20, 2018 15:12

Turn off warnings about std::result_of_t usage on latest msvc compiler.

4d1845b

I will move over to using std::invoke_result_t once I have confirmed that it is supported by clang/libc++ as well.

support for linux scheduler and timer

c549df8

restructuring the code to reduce ifdefs

af44da4

conforming to the white space and indentation rules.

26e6c72

lewissbaker requested changes Feb 13, 2018

View reviewed changes

lewissbaker added 2 commits February 16, 2018 11:53

Update .travis.yml to refer to clang-7

3ecdf4a

The clang mainline has now been branched for clang-6 and mainline now uses clang-7 version.

Try clang-7.0 instead of clang-7 package name

377deb8

lewissbaker and others added 3 commits February 25, 2018 09:10

Fix clang-7 apt package names again

48e1ef5

Try setting CLANG_VERSION=7 instead of 7.0 in Travis config

7d0f2df

[README] Add C++ highlighting for 'schedule_on' (lewissbaker#67)

862e99d

attila0x2A and others added 12 commits March 9, 2018 17:32

Remove possible warnings with redefinitions

936e6ee

If the library is used as part of a bigger project some definitions can be provided via external build files.

Merge pull request lewissbaker#71 from think-cell/redef_warnings

a19b13b

Remove possible warnings with redefinitions of WIN32_LEAN_AND_MEAN.

Update appveyor.yml to ignore x86 optimised failures

4963e33

Several tests are currently failing due to what seems to be compiler bugs. Ignore tests for now and revisit once MSVC 15.7 is released.

Merge pull request lewissbaker#76 from lewissbaker/appveyor-tweaks

53d4a06

Update appveyor.yml to ignore x86 optimised failures

Correct file_write_operation's friend class

a4d2c7b

Merge pull request lewissbaker#77 from think-cell/correct_file_write

98d98e2

Correct file_write_operation's friend class

Fix typo in async_generator state description

b0e5f76

Fix io_service_fixture_with_threads: create given threads count (lewi…

14ad4bd

…ssbaker#75)

Replace <iosfwd> with <ostream> in doctest.h to try and fix linker er…

bce2a72

…rors under Linux (lewissbaker#78)

[AppVeyor] Reenable VS 2017 Preview build variants

5aa1cd1

The AppVeyor 'Visual Studio 2017 Preview' image has now been updated with VS 2017.7 Preview 2

Link against vcruntime[d].lib instead of msvcurt[d].lib

2492c07

The msvcurt[d].lib is actually intended for C++/CLI usage and seems to no longer being bundled with the default VS install in more recent Visual Studio 2017 installs. Should fix lewissbaker#73

fixed the minor comments from lewis.

115aa39

neo5167 added 2 commits April 12, 2018 18:23

changes for white space based on clang-format.el

e5dc2fc

copyright block

d02cab7

lewissbaker mentioned this pull request Jun 12, 2018

error compilation with clang 6.0.1 : 4 errors generated #84

Closed

neo5167 and others added 15 commits June 20, 2018 12:02

support for linux scheduler and timer

b23f640

restructuring the code to reduce ifdefs

723c049

conforming to the white space and indentation rules.

b8db839

fixed the minor comments from lewis.

cc86c9b

changes for white space based on clang-format.el

343fe0a

copyright block

2565005

rebasing with upstream master and fixing some windows bugs.

b006b04

fixing merge conflicts.

7fec714

removing repeated files from cake.

87910b8

indentation fixes.

8007d4f

indentation fixes.

ab4b9b7

more indentation changes. Moving from cc mode to llvm style.

d38a213

reverting some changes since the functions are no longer needed.

1696936

windows tests passing.

751232b

All linux tests passing.

f870053

andreasbuhr mentioned this pull request Oct 12, 2020

Integrate all andreasbuhr/cppcoro#3

Closed

danlapid mentioned this pull request Nov 29, 2022

Added linux scheduler andreasbuhr/cppcoro#63

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for linux scheduler and timer #64

support for linux scheduler and timer #64

neo5167 commented Jan 27, 2018

lewissbaker left a comment •

edited

Loading

lewissbaker Feb 5, 2018

lewissbaker Feb 5, 2018

lewissbaker Feb 5, 2018

lewissbaker Feb 5, 2018

lewissbaker Feb 5, 2018

lewissbaker Feb 5, 2018

neo5167 Apr 11, 2018

lewissbaker Feb 12, 2018

lewissbaker Feb 19, 2018

neo5167 Apr 11, 2018

lewissbaker Feb 12, 2018

lewissbaker Feb 13, 2018

neo5167 Apr 11, 2018

neo5167 Apr 16, 2018

lewissbaker Feb 13, 2018

neo5167 Apr 11, 2018

neo5167 Apr 12, 2018

lewissbaker Feb 13, 2018

neo5167 Apr 11, 2018

lewissbaker Feb 13, 2018

neo5167 Apr 11, 2018

neo5167 commented Feb 20, 2018 •

edited

Loading

lewissbaker commented Mar 5, 2018

neo5167 commented Mar 6, 2018

neo5167 commented Apr 11, 2018

neo5167 commented Apr 16, 2018

support for linux scheduler and timer #64

Are you sure you want to change the base?

support for linux scheduler and timer #64

Conversation

neo5167 commented Jan 27, 2018

lewissbaker 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

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

neo5167 commented Feb 20, 2018 • edited Loading

lewissbaker commented Mar 5, 2018

neo5167 commented Mar 6, 2018

neo5167 commented Apr 11, 2018

neo5167 commented Apr 16, 2018

lewissbaker left a comment •

edited

Loading

neo5167 commented Feb 20, 2018 •

edited

Loading