Skip to content

SocketAsyncEngine.Unix: Use eventfd(2) to signal shutdown #1538

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.

Sign up for GitHub

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

Closed
lpereira wants to merge 1 commit into from
Closed

SocketAsyncEngine.Unix: Use eventfd(2) to signal shutdown #1538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/libraries/Common/src/Interop/Unix/System.Native/Interop.Eventfd.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Runtime.InteropServices;

internal static partial class Interop
{
internal static partial class Sys
{
//
// Since eventfd is a Linux-only feature, there's no need to define our own
// flag values: pass these values through as the Linux system call expects.
//

@stephentoub stephentoub Jan 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We like to treat the shims as platform-agnostic, and the values we pass in here be for the PAL rather than for a particular platform. If the values we define happen to be the same as for that platform, then the PAL implementation can avoid doing any casting and just pass them through, but that's up to the PAL.

What that actually means for your changes:

  • Remove this comment
  • Define these values in pal_io.h
    _ Add static asserts that the PAL EFD_SEMAPHORE matches the target platform's EFD_SEMAPHORE

That will keep the PAL platform-agnostic from a managed code perspective while also not adding runtime overhead.

We do this elsewhere.

[Flags]
internal enum EventFdFlags
{
EFD_SEMAPHORE = 0x1,
EFD_CLOEXEC = 0x80000,
EFD_NONBLOCK = 0x800,
}

[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_EventFD", SetLastError = true)]
internal static extern unsafe int EventFD(uint initialVal, EventFdFlags flags = 0);
}
}
1 change: 1 addition & 0 deletions src/libraries/Native/Unix/Common/pal_config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@
#cmakedefine01 HAVE_TCP_H_TCP_KEEPALIVE
#cmakedefine01 HAVE_BUILTIN_MUL_OVERFLOW
#cmakedefine01 HAVE_DISCONNECTX
#cmakedefine01 HAVE_EVENTFD

// Mac OS X has stat64, but it is deprecated since plain stat now
// provides the same 64-bit aware struct when targeting OS X > 10.5
Expand Down
18 changes: 18 additions & 0 deletions src/libraries/Native/Unix/System.Native/pal_io.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@
#if HAVE_INOTIFY
#include <sys/inotify.h>
#endif
#if HAVE_EVENTFD
#include <sys/eventfd.h>
#endif

#ifdef _AIX
#include <alloca.h>
Expand Down Expand Up @@ -475,6 +478,21 @@ int32_t SystemNative_CloseDir(DIR* dir)
return closedir(dir);
}

#if HAVE_EVENTFD
int32_t SystemNative_EventFD(uint32_t initialVal, int32_t flags)
{
return eventfd(initialVal, flags);
}
#else
int32_t SystemNative_EventFD(uint32_t initialVal, int32_t flags)
{
(void)initialVal;
(void)flags;
errno = ENOTSUP;
return -1;
}
#endif

int32_t SystemNative_Pipe(int32_t pipeFds[2], int32_t flags)
{
switch (flags)
Expand Down
8 changes: 8 additions & 0 deletions src/libraries/Native/Unix/System.Native/pal_io.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,14 @@ DLLEXPORT int32_t SystemNative_CloseDir(DIR* dir);
DLLEXPORT int32_t SystemNative_Pipe(int32_t pipefd[2], // [out] pipefds[0] gets read end, pipefd[1] gets write end.
int32_t flags); // 0 for defaults or PAL_O_CLOEXEC for close-on-exec

/**
* Creates an eventfd on Linux. Returns error everywhere else.

@stephentoub stephentoub Jan 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
* Creates an eventfd on Linux. Returns error everywhere else.
* Creates an eventfd if available. Returns ENOTSUP error everywhere else.

*
* Returns 0 for success, -1 for failure. Sets errno on failure.
*/
DLLEXPORT int32_t SystemNative_EventFD(uint32_t initialValue,
int32_t flags); // Passes through to eventfd() without conversion

@stephentoub stephentoub Jan 10, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Going along with my previous comment:

Suggested change
int32_t flags); // Passes through to eventfd() without conversion
int32_t flags);


// NOTE: Rather than a general fcntl shim, we opt to export separate functions
// for each command. This allows use to have strongly typed arguments and saves
// complexity around converting command codes.
Expand Down
5 changes: 5 additions & 0 deletions src/libraries/Native/Unix/configure.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ check_symbol_exists(
ifaddrs.h
HAVE_GETIFADDRS)

check_symbol_exists(
eventfd
sys/eventfd.h
HAVE_EVENTFD)

check_symbol_exists(
lseek64
unistd.h
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Net.Sockets/src/System.Net.Sockets.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,9 @@
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Pipe.cs">
<Link>Common\Interop\Unix\System.Native\Interop.Pipe.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Eventfd.cs">
<Link>Common\Interop\Unix\System.Native\Interop.Eventfd.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Write.cs">
<Link>Common\Interop\Unix\System.Native\Interop.Write.cs</Link>
</Compile>
Expand Down
70 changes: 50 additions & 20 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,32 @@ private void FreeHandle(IntPtr handle)
}
}

private void CreateShutdownPipeOrEventFD(out int readFD, out int writeFD)
{
//
// If this system supports eventfd(2), use it to signal shutdown, and save us
// a file descriptor per engine. Otherwise, fall back to using a pipe.
//
int eventFD = Interop.Sys.EventFD(0, Interop.Sys.EventFdFlags.EFD_CLOEXEC | Interop.Sys.EventFdFlags.EFD_SEMAPHORE);

if (eventFD >= 0)
{
readFD = writeFD = eventFD;
}
else
{
int* pipeFds = stackalloc int[2];
int pipeResult = Interop.Sys.Pipe(pipeFds, Interop.Sys.PipeFlags.O_CLOEXEC);
if (pipeResult != 0)
{
throw new InternalException(pipeResult);
}

readFD = pipeFds[Interop.Sys.ReadEndOfPipe];
writeFD = pipeFds[Interop.Sys.WriteEndOfPipe];
}
}

private SocketAsyncEngine()
{
_port = (IntPtr)(-1);
Expand All @@ -258,18 +284,7 @@ private SocketAsyncEngine()
throw new InternalException(err);
}

//
// Create the pipe for signaling shutdown, and register for "read" events for the pipe. Now writing
// to the pipe will send an event to the event loop.
//
int* pipeFds = stackalloc int[2];
int pipeResult = Interop.Sys.Pipe(pipeFds, Interop.Sys.PipeFlags.O_CLOEXEC);
if (pipeResult != 0)
{
throw new InternalException(pipeResult);
}
_shutdownReadPipe = pipeFds[Interop.Sys.ReadEndOfPipe];
_shutdownWritePipe = pipeFds[Interop.Sys.WriteEndOfPipe];
CreateShutdownPipeOrEventFD(out _shutdownReadPipe, out _shutdownWritePipe);

err = Interop.Sys.TryChangeSocketEventRegistration(_port, (IntPtr)_shutdownReadPipe, Interop.Sys.SocketEvents.None, Interop.Sys.SocketEvents.Read, ShutdownHandle);
if (err != Interop.Error.SUCCESS)
Expand Down Expand Up @@ -351,25 +366,40 @@ private void EventLoop()
private void RequestEventLoopShutdown()
{
//
// Write to the pipe, which will wake up the event loop and cause it to exit.
// Write to the pipe or eventfd, which will wake up the event loop and cause it to exit.
// (Need to write 8 bytes in case we're on a system where eventfd is supported.)
//
byte b = 1;
int bytesWritten = Interop.Sys.Write(_shutdownWritePipe, &b, 1);
if (bytesWritten != 1)
byte[] wakeThreadUp = {0, 0, 0, 0, 0, 0, 0, 1};
fixed (byte *b = &wakeThreadUp[0])

@stephentoub stephentoub Jan 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This allocates an 8-byte array on the heap. That isn't necessary. You can instead make it:

byte* wakeThreadUp = stackalloc byte[8] { 0, 0, 0, 0, 0, 0, 0, 1 };

and not need the fixed at all.

@lpereira lpereira Jan 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Still working on the intuition to understand how C# code is generated/executed.

{
throw new InternalException(bytesWritten);
int bytesWritten = Interop.Sys.Write(_shutdownWritePipe, b, 8);
if (bytesWritten != 8)
{
throw new InternalException(bytesWritten);

@stephentoub stephentoub Jan 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks suspect. In general write isn't guaranteed to write everything you pass in, which is why it returns how many bytes it wrote. When we were writing 1 byte, this was fine, because it won't return 0 unless there was an error, but now that it's being called with 8, it could theoretically successfully return 1 through 7. Does something prevent that from being the case when the fd is a pipe or an eventfd, on all platforms we target now and in the future?

@lpereira lpereira Jan 10, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm aware of that. However, for eventfd, this is always fine (it's always ready to accept a 64-bit int). When using pipes, it's very unlikely that the buffer wouldn't have at least 8 bytes free, especially since these pipes are used only during shutdown and this is the only thing that's ever written to them (to make it more robust on Linux the pipe could be opened with O_DIRECT, so read/writes are atomic, but this doesn't work under WSL1 and BSDs AFAICT). So this is fine.

@stephentoub stephentoub Jan 10, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

very unlikely

I don't think "very unlikely" is sufficient for code that we need to be extremely robust.

I suggest instead doing:

Debug.Assert(bytesWritten == 8);
if (bytesWritten < 1)
{
    throw new InternalException(bytesWritten);
}

or something like that.

@lpereira lpereira Jan 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pipe buffers are at least a page in all systems we care about. It's not very unlikely, it's very unlikely.

But, yeah, the proposed change is fine. Even if the assert is removed in release builds, this would fine: the amount of bytes written here (on pipes, where short writes are possible) doesn't really matter as it's only used to wake the thread blocked on poll().

}
}
}

private void FreeNativeResources()
{
if (_shutdownReadPipe != -1)
if (_shutdownReadPipe == _shutdownWritePipe && _shutdownReadPipe != -1)
{
//
// When using eventfd, read/write ends of the "pipe" have the same file
// descriptor.
//
Interop.Sys.Close((IntPtr)_shutdownReadPipe);

@stephentoub stephentoub Jan 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This would be better as:

if (_shutdownReadPipe == _shutdownWritePipe)
{
    if (_shutdownReadPipe != -1)
    {
        ...
    }
}

}
if (_shutdownWritePipe != -1)
else
{
Interop.Sys.Close((IntPtr)_shutdownWritePipe);
if (_shutdownReadPipe != -1)
{
Interop.Sys.Close((IntPtr)_shutdownReadPipe);
}
if (_shutdownWritePipe != -1)
{
Interop.Sys.Close((IntPtr)_shutdownWritePipe);
}

@stephentoub stephentoub Jan 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or better yet:

if (_shutdownReadPipe != -1)
{
    Interop.Sys.Close((IntPtr)_shutdownReadPipe);
}
if (_shutdownReadPipe != _shutdownWritePipe && _shutdownWritePipe != -1)
{
    Interop.Sys.Close((IntPtr)_shutdownWritePipe);
}

}
if (_buffer != null)
{
Expand Down