issue: 2935075 Fix Windows support #193

EldarShalev · 2022-07-20T11:39:14Z

Adding Windows support for sockperf including AF_UNIX.
Most of the code is added with #ifdef WIN32, so no impact is expected on linux.

Major changes include:

AF_UNIX which in windows is only supported by SOCK_STREAM mode so no binding is needed from client side (thus no path_pid_num binding as provided in linux)
Using new API s.a 'WSAStartup', 'DeleteFileA' (instead of unlink), 'closesocket()' instead of close(), etc..
Working with <winsock2.h>

More about AF_UNIX in windows:
https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/

swx-jenkins3 · 2022-07-20T11:39:18Z

Can one of the admins verify this patch?

igor-ivanov · 2022-07-27T16:24:26Z

src/client.cpp

                if (setitimer(ITIMER_REAL, &timer, NULL)) {
                    log_err("ERROR: setitimer() failed when disarming");
                }
+#endif


please consider moving os dependencies into os_abstarct.[h|cpp] if possible.
For example:

int os_settimer(timer) { #ifdef WIN32 #else #endif }

Pay attention that Linux and Windows return different values but it can be unified to linux style.

igor-ivanov · 2022-07-27T16:28:00Z

src/ip_address.cpp

        err = gai_strerror(res);
+#endif


please consider moving os dependencies into os_abstarct.[h|cpp] if possible.

src/sockperf.cpp

igor-ivanov · 2022-07-27T16:37:53Z

src/sockperf.cpp

@@ -2337,7 +2352,12 @@ void cleanup() {
    if (g_fds_array) {
        for (ifd = 0; ifd <= s_fd_max; ifd++) {
            if (g_fds_array[ifd]) {
+#ifdef WIN32
+                closesocket(ifd);
+                WSACleanup();


It seems as WSACleanup() call is wrong. It impacts the process and in fact called later as os_sock_cleanup

close() and closesocket() should be in os_abstract.h

WSACleanup() must be called if WSAStartup() was called, So I do need to call it.
doc- https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-wsastartup

The WSAStartup function must be the first Windows Sockets function called by an application or DLL. It allows an application or DLL to specify the version of Windows Sockets required and retrieve details of the specific Windows Sockets implementation. The application or DLL can only issue further Windows Sockets functions after successfully calling WSAStartup.
If the wVersion member of the WSADATA structure is unacceptable to the caller, the application or DLL should call WSACleanup to release the Winsock DLL resources and fail to initialize the Winsock application.

I can move close and closesocket , just to clarify there is a different-
close() is a unix function work on any file descriptor,
closesocket() is a Windows-specific function, which works specifically with sockets.

WSACleanup() must be called if WSAStartup() was called, So I do need to call it.
doc- https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-wsastartup

The WSAStartup function must be the first Windows Sockets function called by an application or DLL. It allows an application or DLL to specify the version of Windows Sockets required and retrieve details of the specific Windows Sockets implementation. The application or DLL can only issue further Windows Sockets functions after successfully calling WSAStartup. If the wVersion member of the WSADATA structure is unacceptable to the caller, the application or DLL should call WSACleanup to release the Winsock DLL resources and fail to initialize the Winsock application.

I can move close and closesocket , just to clarify there is a different-
close() is a unix function work on any file descriptor,
closesocket() is a Windows-specific function, which works specifically with sockets.

All above is correct but I said that WSACleanup() is called inside os_sock_cleanup() and no need to call WSACleanup() many times.

Sorry, missed that!
ok no problem

igor-ivanov · 2022-07-27T16:44:38Z

src/sockperf.cpp

                        unlink(g_fds_array[ifd]->server_addr.addr_un.sun_path);
+#endif


There are two ways here:
1 - wrapping if (s_user_params.addr.ss_family == AF_UNIX) { } by #ifndef WIN32
2 - do nothing if code under if (s_user_params.addr.ss_family == AF_UNIX) { } is compiled under windows. Code should not allow set unix socket under windows. In this case any condition as == AF_UNIX is FALSE

I prefer the option 2 (Less #ifdef constructions in the code)

In windows AF_UNIX is only supported in sock_stream, therefor the if statement for not using DGRAM.
I can do nothing, but then if someone use AF_UNIX in windows without --tcp option than the behavior is not defined.

this is the only place I left with #ifdef

src/sockperf.cpp

igor-ivanov · 2022-07-27T16:47:25Z

src/sockperf.cpp

        errno = EPERM;
        exit_with_err("Please compile with VMA Extra API to use these options", SOCKPERF_ERR_FATAL);
    }
 #endif
+#endif


There are two ways here:
1 - wrapping if (s_user_params.fd_handler_type == SOCKETXTREME) { } by #ifndef WIN32
2 - do nothing if code under if (s_user_params.fd_handler_type == SOCKETXTREME) { } is compiled under windows. Code should not allow set SOCKETXTREME mode under windows. In this case any condition as == SOCKETXTREME is FALSE

I prefer the option 2 (Less #ifdef constructions in the code)

win/project/sockperf.vcxproj

src/ticks.cpp

src/client.cpp

src/common.h

src/ip_address.cpp

src/sockperf.cpp

win/project/sockperf.vcxproj

src/sockperf.cpp

igor-ivanov · 2022-08-08T09:26:22Z

win/project/sockperf.vcxproj

@@ -133,7 +133,7 @@
      <Optimization>MaxSpeed</Optimization>
      <FunctionLevelLinking>true</FunctionLevelLinking>
      <IntrinsicFunctions>true</IntrinsicFunctions>
-      <PreprocessorDefinitions>WIN32;NDEBUG;_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>__windows__;WIN32;NDEBUG;_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>


Please remove WIN32

igor-ivanov · 2022-08-08T09:26:32Z

win/project/sockperf.vcxproj

@@ -115,7 +115,7 @@
      <Optimization>MaxSpeed</Optimization>
      <FunctionLevelLinking>true</FunctionLevelLinking>
      <IntrinsicFunctions>true</IntrinsicFunctions>
-      <PreprocessorDefinitions>WIN32;NDEBUG;_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>__windows__;WIN32;NDEBUG;_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>


Please remove WIN32

igor-ivanov · 2022-08-08T09:26:36Z

win/project/sockperf.vcxproj

@@ -99,7 +99,7 @@
      </PrecompiledHeader>
      <WarningLevel>Level3</WarningLevel>
      <Optimization>Disabled</Optimization>
-      <PreprocessorDefinitions>WIN32;_DEBUG;_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>__windows__;WIN32;_DEBUG;_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>


Please remove WIN32

igor-ivanov · 2022-08-08T09:26:40Z

win/project/sockperf.vcxproj

@@ -85,7 +85,7 @@
      </PrecompiledHeader>
      <WarningLevel>Level3</WarningLevel>
      <Optimization>Disabled</Optimization>
-      <PreprocessorDefinitions>WIN32;_DEBUG;_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>__windows__;WIN32;_DEBUG;_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>


Please remove WIN32

igor-ivanov · 2022-08-08T10:25:09Z

src/common.cpp

@@ -64,7 +64,7 @@ std::string sockaddr_to_hostport(const struct sockaddr *addr)
    if (addr->sa_family == AF_INET6) {
        return "[" + std::string(hbuf) + "]:" + std::string(pbuf);
    } else if (addr->sa_family == AF_UNIX) {
-        return std::string(pbuf) + " [UNIX]";
+        return std::string(addr->sa_data) + " [AF_UNIX]";


can you show output

in linux it's enough to give the pbuf because getnameinfo knows how to fill it with the address and the output is good.

In windows, I changed to more common way that works both for linux and windows with addr->sa_data which is the address directly.
Also changed UNIX to AF_UNIX - more generic
BTW, now I see another place to change..

I think that [AF_UNIX] is not needed
I prefer [fd=232] Binding to: \tmp\test ...

Can you please adapt https://github.com/Mellanox/sockperf/blob/sockperf_v2/src/iohandlers.cpp#L41
to
printf("[%2d] ADDR = %s # %s\n", list_count++, pbuf, PRINT_PROTOCOL(data->sock_type));
to get output as
[ 0] ADDR = \tmp\test # TCP

just one thing, it's SOCK_STREAM / SOCK_DGRAM rather then TCP / UDP
so output like
[ 0] ADDR = \tmp\test # SOCK_STREAM

just one thing, it's SOCK_STREAM / SOCK_DGRAM rather then TCP / UDP so output like [ 0] ADDR = \tmp\test # SOCK_STREAM

TCP/UDP is clear for end user and exist. I do not see benefits in SOCK_STREAM / SOCK_DGRAM

it's just a semantic we agreed in UDS patch, that AF_UNIX is not a TCP/UDP protocol and it might be confusing.
AF_INET + SOCK_STREAM = TCP
AF_UNIX + SOCK_STREAM !=TCP

That's why in the print I use PRINT_SOCKET_TYPE and not PRINT_PROTOCOL

The behavior or AF_UNIX + SOCK_STREAM sockets is comparable to TCP.
So seeing
[ 0] ADDR = \tmp\test # TCP
in output points to AF_UNIX + SOCK_STREAM
and
[ 0] ADDR = 1.1.1.1 # TCP
points to AF_INET + SOCK_STREAM
Let keep identical output format for inet and unix

igor-ivanov · 2022-08-08T10:26:40Z

src/ip_address.h

+#include <afunix.h>
+typedef unsigned short int sa_family_t;
+
+#else


please consider #elif __linux__

igor-ivanov · 2022-08-08T10:34:34Z

src/sockperf.cpp

                    if (s_user_params.mode == MODE_SERVER)
+#ifdef WIN32
+                        DeleteFileA(g_fds_array[ifd]->server_addr.addr_un.sun_path);
+#else
                        unlink(g_fds_array[ifd]->server_addr.addr_un.sun_path);


move DeleteFileA / unlink into os_abstract.
consider using remove() instead of DeleteFileA

igor-ivanov · 2022-08-08T10:35:53Z

src/sockperf.cpp

@@ -3332,6 +3354,7 @@ int bringup(const int *p_daemonize) {
    /* Setup VMA */
    int _vma_pkts_desc_size = 0;

+


remove empty line

igor-ivanov · 2022-08-08T10:37:34Z

insert issue: 2935075 in the second commit title

igor-ivanov

Fix commit title "issue 2935075: RM Adding Windows support" to "issue: 2935075 Fix Windows support"
and the second as "issue: 2935075 Add preprocessor definition for windows"

EldarShalev · 2022-08-10T13:30:10Z

Feedfile example:

src/sockperf.cpp

src/defs.h

agalanin-at-nvidia · 2022-08-15T07:56:57Z

bot:retest

src/defs.h

agalanin-at-nvidia · 2022-08-16T12:44:15Z

bot:retest

vasily-v-ryabov · 2022-08-16T14:34:20Z

src/sockperf.cpp

    bool success = vma_xlio_try_set_func_pointers();
    if (!success) {
        log_dbg("Failed to set function pointers for system functions.");
        log_dbg("Check vma-xlio-redirect.cpp for functions which your OS implementation is missing. "
                "Re-compile sockperf without them.");
    }
 #endif
+    int rc = 0;
+    if (os_sock_startup() == false) { // Only relevant for Windows


Should we replace #endif above with #else and add #endif below this condition?

Yes agree since os_sock_startup() is only relevant for windows

agalanin-at-nvidia · 2022-08-17T07:01:07Z

bot:retest

agalanin-at-nvidia · 2022-08-17T11:46:47Z

bot:retest

src/sockperf.cpp

agalanin-at-nvidia · 2022-08-18T07:07:51Z

bot:retest

win/project/sockperf.vcxproj

agalanin-at-nvidia · 2022-08-22T06:36:15Z

bot:retest

agalanin-at-nvidia · 2022-08-22T13:47:08Z

bot:retest

src/defs.h

doc/main.dox

Signed-off-by: Eldar Shalev <eldarsh@nvidia.com>

EldarShalev force-pushed the windows branch from 95c17e2 to 1ba0d48 Compare July 20, 2022 11:47

igor-ivanov self-assigned this Jul 27, 2022

igor-ivanov added the enhancement label Jul 27, 2022

igor-ivanov requested review from igor-ivanov, agalanin-at-nvidia and pasis July 27, 2022 11:42

igor-ivanov reviewed Jul 27, 2022

View reviewed changes

igor-ivanov changed the title ~~Adding Windows support~~ issue: 2935075 Adding Windows support Jul 27, 2022

igor-ivanov changed the title ~~issue: 2935075 Adding Windows support~~ issue: 2935075 Add Windows support Jul 27, 2022

igor-ivanov changed the title ~~issue: 2935075 Add Windows support~~ issue: 2935075 Fix Windows support Jul 27, 2022

EldarShalev force-pushed the windows branch 2 times, most recently from 80a4c16 to 5a98f5c Compare August 4, 2022 12:56

agalanin-at-nvidia reviewed Aug 5, 2022

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

src/common.h Show resolved Hide resolved

src/ip_address.cpp Outdated Show resolved Hide resolved

src/sockperf.cpp Outdated Show resolved Hide resolved

agalanin-at-nvidia reviewed Aug 5, 2022

View reviewed changes

win/project/sockperf.vcxproj Outdated Show resolved Hide resolved

igor-ivanov reviewed Aug 8, 2022

View reviewed changes

EldarShalev force-pushed the windows branch from 5a98f5c to 0e174ac Compare August 10, 2022 09:23

igor-ivanov reviewed Aug 10, 2022

View reviewed changes

EldarShalev force-pushed the windows branch 2 times, most recently from 4f45cc2 to a6c2e1a Compare August 10, 2022 13:24

EldarShalev force-pushed the windows branch from a6c2e1a to 931a6b2 Compare August 10, 2022 13:41

agalanin-at-nvidia reviewed Aug 11, 2022

View reviewed changes

src/sockperf.cpp Outdated Show resolved Hide resolved

EldarShalev force-pushed the windows branch 2 times, most recently from 17f5099 to 6bb155b Compare August 14, 2022 14:00

agalanin-at-nvidia reviewed Aug 15, 2022

View reviewed changes

src/defs.h Outdated Show resolved Hide resolved

src/defs.h Outdated Show resolved Hide resolved

EldarShalev force-pushed the windows branch from 6bb155b to 65be809 Compare August 15, 2022 08:54

agalanin-at-nvidia reviewed Aug 15, 2022

View reviewed changes

src/defs.h Outdated Show resolved Hide resolved

EldarShalev force-pushed the windows branch from 65be809 to 2dbf83e Compare August 15, 2022 10:00

agalanin-at-nvidia reviewed Aug 15, 2022

View reviewed changes

src/defs.h Outdated Show resolved Hide resolved

EldarShalev force-pushed the windows branch 2 times, most recently from 90f5085 to 84cddfd Compare August 16, 2022 12:42

vasily-v-ryabov reviewed Aug 16, 2022

View reviewed changes

igor-ivanov mentioned this pull request Aug 16, 2022

how to install sockperf onwindows 10 and window 11 machines #197

Closed

EldarShalev force-pushed the windows branch from 84cddfd to 173cf04 Compare August 17, 2022 06:59

igor-ivanov approved these changes Aug 17, 2022

View reviewed changes

EldarShalev force-pushed the windows branch from 173cf04 to c9ebd80 Compare August 17, 2022 10:56

EldarShalev force-pushed the windows branch from c9ebd80 to 60a90ce Compare August 17, 2022 14:19

agalanin-at-nvidia reviewed Aug 17, 2022

View reviewed changes

src/sockperf.cpp Outdated Show resolved Hide resolved

EldarShalev force-pushed the windows branch from 60a90ce to 1e938e0 Compare August 17, 2022 14:59

vasily-v-ryabov reviewed Aug 19, 2022

View reviewed changes

win/project/sockperf.vcxproj Outdated Show resolved Hide resolved

EldarShalev force-pushed the windows branch from 1e938e0 to f65e56e Compare August 21, 2022 07:42

EldarShalev force-pushed the windows branch from f65e56e to 7f6e22c Compare August 22, 2022 13:36

agalanin-at-nvidia reviewed Aug 23, 2022

View reviewed changes

src/defs.h Show resolved Hide resolved

agalanin-at-nvidia approved these changes Aug 23, 2022

View reviewed changes

agalanin-at-nvidia reviewed Aug 23, 2022

View reviewed changes

doc/main.dox Outdated Show resolved Hide resolved

Eldar Shalev added 2 commits August 23, 2022 11:52

issue 2935075: Fix Windows support

382e4b1

Signed-off-by: Eldar Shalev <eldarsh@nvidia.com>

issue 2935075: Add preprocessor definition for windows

b73cc2b

EldarShalev force-pushed the windows branch from 7f6e22c to b73cc2b Compare August 23, 2022 08:52

igor-ivanov merged commit 2ce4123 into Mellanox:sockperf_v2 Aug 23, 2022

EldarShalev deleted the windows branch August 23, 2022 09:51

		unlink(g_fds_array[ifd]->server_addr.addr_un.sun_path);
		#endif

		@@ -3332,6 +3354,7 @@ int bringup(const int *p_daemonize) {
		/* Setup VMA */
		int _vma_pkts_desc_size = 0;

issue: 2935075 Fix Windows support #193

issue: 2935075 Fix Windows support #193

Conversation

EldarShalev commented Jul 20, 2022 • edited Loading

swx-jenkins3 commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-ivanov Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EldarShalev Jul 31, 2022 • 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

EldarShalev Jul 31, 2022 • 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

igor-ivanov Aug 10, 2022 • 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

igor-ivanov commented Aug 8, 2022

igor-ivanov left a comment • edited Loading

Choose a reason for hiding this comment

EldarShalev commented Aug 10, 2022

agalanin-at-nvidia commented Aug 15, 2022

agalanin-at-nvidia commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agalanin-at-nvidia commented Aug 17, 2022

agalanin-at-nvidia commented Aug 17, 2022

agalanin-at-nvidia commented Aug 18, 2022

agalanin-at-nvidia commented Aug 22, 2022

agalanin-at-nvidia commented Aug 22, 2022

EldarShalev commented Jul 20, 2022 •

edited

Loading

igor-ivanov Jul 27, 2022 •

edited

Loading

EldarShalev Jul 31, 2022 •

edited

Loading

EldarShalev Jul 31, 2022 •

edited

Loading

igor-ivanov Aug 10, 2022 •

edited

Loading

igor-ivanov left a comment •

edited

Loading