<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">Hi all,</div><div class=""><br class=""></div><div class="">Here’s another attempt, with all of Ken’s suggestions in place.</div><div class="">-Matt</div><div class=""><br class=""></div><div class="">---</div><div class=""> dlls/ws2_32/socket.c | 116 +++++++++++++++++++++++++++++++++++++++++++++--</div><div class=""> dlls/ws2_32/tests/sock.c | 2 -</div><div class=""> 2 files changed, 113 insertions(+), 5 deletions(-)</div><div class=""><br class=""></div><div class="">diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c</div><div class="">index ca82ec9..465be2e 100644</div><div class="">--- a/dlls/ws2_32/socket.c</div><div class="">+++ b/dlls/ws2_32/socket.c</div><div class="">@@ -1429,6 +1429,15 @@ static int set_ipx_packettype(int sock, int ptype)</div><div class=""> #endif</div><div class=""> }</div><div class=""> </div><div class="">+#define TABLE_SIZE 256</div><div class="">+#define BUCKET_DEPTH 16</div><div class="">+static SOCKET socket_table[TABLE_SIZE][BUCKET_DEPTH];</div><div class="">+</div><div class="">+/* Cache support */</div><div class="">+static void add_to_table(SOCKET s);</div><div class="">+static BOOL remove_from_table(SOCKET s);</div><div class="">+static BOOL socket_in_table(SOCKET s);</div><div class="">+</div><div class=""> /* ----------------------------------- API -----</div><div class=""> *</div><div class=""> * Init / cleanup / error checking.</div><div class="">@@ -1470,8 +1479,24 @@ int WINAPI WSAStartup(WORD wVersionRequested, LPWSADATA lpWSAData)</div><div class=""> INT WINAPI WSACleanup(void)</div><div class=""> {</div><div class=""> if (num_startup) {</div><div class="">- num_startup--;</div><div class="">- TRACE("pending cleanups: %d\n", num_startup);</div><div class="">+ /* WS_closesocket needs num_startup to be non-zero, so decrement afterwards */</div><div class="">+ if (num_startup == 1) {</div><div class="">+ TRACE("cleaning up sockets");</div><div class="">+ int i, j;</div><div class="">+ for (i = 0; i < TABLE_SIZE; ++i) {</div><div class="">+ for (j = 0; j < BUCKET_DEPTH; ++j) {</div><div class="">+ SOCKET s = socket_table[i][j];</div><div class="">+ if (s) {</div><div class="">+ WS_closesocket(s);</div><div class="">+ }</div><div class="">+ }</div><div class="">+ }</div><div class="">+ num_startup = 0;</div><div class="">+ } else {</div><div class="">+ num_startup--;</div><div class="">+ TRACE("pending cleanups: %d\n", num_startup);</div><div class="">+ }</div><div class="">+</div><div class=""> return 0;</div><div class=""> }</div><div class=""> SetLastError(WSANOTINITIALISED);</div><div class="">@@ -2595,6 +2620,11 @@ SOCKET WINAPI WS_accept(SOCKET s, struct WS_sockaddr *addr, int *addrlen32)</div><div class=""> WS_closesocket(as);</div><div class=""> return SOCKET_ERROR;</div><div class=""> }</div><div class="">+ else </div><div class="">+ {</div><div class="">+ add_to_table(as);</div><div class="">+ }</div><div class="">+</div><div class=""> TRACE("\taccepted %04lx\n", as);</div><div class=""> return as;</div><div class=""> }</div><div class="">@@ -2965,15 +2995,21 @@ int WINAPI WS_closesocket(SOCKET s)</div><div class=""> int res = SOCKET_ERROR, fd;</div><div class=""> if (num_startup)</div><div class=""> {</div><div class="">+ BOOL success = FALSE;</div><div class=""> fd = get_sock_fd(s, FILE_READ_DATA, NULL);</div><div class=""> if (fd >= 0)</div><div class=""> {</div><div class=""> release_sock_fd(s, fd);</div><div class=""> if (CloseHandle(SOCKET2HANDLE(s)))</div><div class=""> res = 0;</div><div class="">+</div><div class="">+ success = remove_from_table(s);</div><div class=""> }</div><div class="">- else</div><div class="">+</div><div class="">+ if (!success) {</div><div class=""> SetLastError(WSAENOTSOCK);</div><div class="">+ res = SOCKET_ERROR;</div><div class="">+ }</div><div class=""> }</div><div class=""> else</div><div class=""> SetLastError(WSANOTINITIALISED);</div><div class="">@@ -4987,6 +5023,11 @@ static int WS2_sendto( SOCKET s, LPWSABUF lpBuffers, DWORD dwBufferCount,</div><div class=""> DWORD bytes_sent;</div><div class=""> BOOL is_blocking;</div><div class=""> </div><div class="">+ if (!socket_in_table(s)) {</div><div class="">+ SetLastError(WSAENOTSOCK);</div><div class="">+ return SOCKET_ERROR;;</div><div class="">+ }</div><div class="">+</div><div class=""> TRACE("socket %04lx, wsabuf %p, nbufs %d, flags %d, to %p, tolen %d, ovl %p, func %p\n",</div><div class=""> s, lpBuffers, dwBufferCount, dwFlags,</div><div class=""> to, tolen, lpOverlapped, lpCompletionRoutine);</div><div class="">@@ -6706,6 +6747,7 @@ SOCKET WINAPI WSASocketW(int af, int type, int protocol,</div><div class=""> SERVER_END_REQ;</div><div class=""> if (ret)</div><div class=""> {</div><div class="">+ add_to_table(ret);</div><div class=""> TRACE("\tcreated %04lx\n", ret );</div><div class=""> if (ipxptype > 0)</div><div class=""> set_ipx_packettype(ret, ipxptype);</div><div class="">@@ -8107,3 +8149,71 @@ INT WINAPI WSCEnumProtocols( LPINT protocols, LPWSAPROTOCOL_INFOW buffer, LPDWOR</div><div class=""> </div><div class=""> return ret;</div><div class=""> }</div><div class="">+</div><div class="">+/*****************/</div><div class="">+/* Cache support */</div><div class="">+</div><div class="">+static inline DWORD socket_to_index(SOCKET s)</div><div class="">+{</div><div class="">+ /* Hash to entry using Bernstein function */</div><div class="">+ DWORD h = 52812;</div><div class="">+ BYTE *b = (BYTE*)&s;</div><div class="">+ h = ((h << 5) + h) ^ b[0];</div><div class="">+ h = ((h << 5) + h) ^ b[1];</div><div class="">+ h = ((h << 5) + h) ^ b[2];</div><div class="">+ h = ((h << 5) + h) ^ b[3];</div><div class="">+ return h % TABLE_SIZE;</div><div class="">+}</div><div class="">+</div><div class="">+static void cache_update(SOCKET new_entry, SOCKET old_entry)</div><div class="">+{</div><div class="">+ int index, depth, tries;</div><div class="">+ LONG *bucket;</div><div class="">+</div><div class="">+ index = socket_to_index(new_entry == 0 ? old_entry : new_entry); </div><div class="">+ depth = 0;</div><div class="">+ bucket = (LONG*)&socket_table[index];</div><div class="">+ tries = 0;</div><div class="">+ do </div><div class="">+ {</div><div class="">+ LONG *slot = (LONG*)&bucket[depth++];</div><div class="">+ if (InterlockedCompareExchange(slot, new_entry, old_entry) == old_entry)</div><div class="">+ break;</div><div class="">+ if (depth == BUCKET_DEPTH)</div><div class="">+ {</div><div class="">+ ++tries;</div><div class="">+ depth = 0;</div><div class="">+ }</div><div class="">+</div><div class="">+ } while (tries < 3);</div><div class="">+ if (tries == 3) </div><div class="">+ {</div><div class="">+ ERR("Socket hash table bucket overflow. Resize buckets");</div><div class="">+ }</div><div class="">+}</div><div class="">+</div><div class="">+static void add_to_table(SOCKET s)</div><div class="">+{</div><div class="">+ cache_update(s, 0);</div><div class="">+}</div><div class="">+</div><div class="">+static BOOL remove_from_table(SOCKET s)</div><div class="">+{</div><div class="">+ if (!socket_in_table(s))</div><div class="">+ return FALSE;</div><div class="">+</div><div class="">+ cache_update(0, s);</div><div class="">+ return TRUE;</div><div class="">+}</div><div class="">+</div><div class="">+static inline BOOL socket_in_table(SOCKET s)</div><div class="">+{</div><div class="">+ int bucket, depth;</div><div class="">+ bucket = socket_to_index(s);</div><div class="">+ for (depth = 0; depth < TABLE_SIZE; ++depth) {</div><div class="">+ if (socket_table[bucket][depth] == s)</div><div class="">+ return TRUE;</div><div class="">+ }</div><div class="">+</div><div class="">+ return FALSE; </div><div class="">+}</div><div class="">diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c</div><div class="">index 2d14496..afa4063 100644</div><div class="">--- a/dlls/ws2_32/tests/sock.c</div><div class="">+++ b/dlls/ws2_32/tests/sock.c</div><div class="">@@ -1119,7 +1119,6 @@ static void test_WithWSAStartup(void)</div><div class=""> ok(res == 0, "WSAStartup() failed unexpectedly: %d\n", res);</div><div class=""> </div><div class=""> /* show that sockets are destroyed automatically after WSACleanup */</div><div class="">- todo_wine {</div><div class=""> SetLastError(0xdeadbeef);</div><div class=""> res = send(src, "TEST", 4, 0);</div><div class=""> error = WSAGetLastError();</div><div class="">@@ -1131,7 +1130,6 @@ static void test_WithWSAStartup(void)</div><div class=""> error = WSAGetLastError();</div><div class=""> ok(res == SOCKET_ERROR, "closesocket should have failed\n");</div><div class=""> ok(error == WSAENOTSOCK, "expected 10038, got %d\n", error);</div><div class="">- }</div><div class=""> </div><div class=""> closesocket(src);</div><div class=""> closesocket(dst);</div><div class="">-- </div><div class="">2.3.2 (Apple Git-55)</div><div class=""><br class=""></div><div><blockquote type="cite" class=""><div class="">On Aug 25, 2015, at 1:44 PM, Matt Durgavich <<a href="mailto:mattdurgavich@gmail.com" class="">mattdurgavich@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div id="mbx-wrapper" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div id="mbx-content" class="content" applecontenteditable="true">Responses inline. </div><div id="mbx-old-email" class="" applecontenteditable="true"><div class="mailbox_signature" style="display: block;"><br class="">Best,<div class="">-Matt</div></div><span id="orc-full-body-initial-text" style="display: inline;" class=""><br class="">On Tuesday, Aug 25, 2015 at 12:46 AM, Ken Thomases <<a href="mailto:ken@codeweavers.com" target="_blank" class="">ken@codeweavers.com</a>>, wrote:<br class=""></span><blockquote class="gmail_quote">Thanks for continuing to work on this. By no means a thorough review:<span class="Apple-converted-space"> </span><br class=""><br class="">> On Aug 24, 2015, at 10:49 PM, Matt Durgavich <<a href="mailto:mattdurgavich@gmail.com" class="">mattdurgavich@gmail.com</a>> wrote:<span class="Apple-converted-space"> </span><br class=""><br class="">> +#define CACHE_SIZE 256<span class="Apple-converted-space"> </span><br class="">> +#define CACHE_DEPTH 16<span class="Apple-converted-space"> </span><br class="">> +static SOCKET socket_cache[CACHE_SIZE][CACHE_DEPTH];<span class="Apple-converted-space"> </span><br class="">> +<span class="Apple-converted-space"> </span><br class="">> +/* Cache support */<span class="Apple-converted-space"> </span><br class="">> +static void add_to_cache(SOCKET s);<span class="Apple-converted-space"> </span><br class="">> +static BOOL remove_from_cache(SOCKET s);<span class="Apple-converted-space"> </span><br class="">> +static BOOL socket_in_cache(SOCKET s);<span class="Apple-converted-space"> </span><br class=""><br class="">I'm not sure this is a "cache", per se. It's a hash table, but naming issues are fairly minor.<span class="Apple-converted-space"> </span><br class=""><br class=""></blockquote><span class="mailbox-inline-edit"></span><div class=""></div><span class="mailbox-inline-edit">Let's go with socket_table</span><div class=""><br class=""><blockquote class="gmail_quote"><br class="">> @@ -1470,8 +1479,24 @@ int WINAPI WSAStartup(WORD wVersionRequested, LPWSADATA lpWSAData)<span class="Apple-converted-space"> </span><br class="">> INT WINAPI WSACleanup(void)<span class="Apple-converted-space"> </span><br class="">> {<span class="Apple-converted-space"> </span><br class="">> if (num_startup) {<span class="Apple-converted-space"> </span><br class="">> - num_startup--;<span class="Apple-converted-space"> </span><br class="">> - TRACE("pending cleanups: %d\n", num_startup);<span class="Apple-converted-space"> </span><br class="">> + /* WS_closesocket needs num_startup to be non-zero, so decrement afterwards */<span class="Apple-converted-space"> </span><br class="">> + if (num_startup - 1 == 0) {<span class="Apple-converted-space"> </span><br class=""><br class="">Why not "if (num_startup == 1)"?<span class="Apple-converted-space"> </span><br class=""><br class="">> +static void add_to_cache(SOCKET s)<span class="Apple-converted-space"> </span><br class="">> +{<span class="Apple-converted-space"> </span><br class="">> + int index, depth;<span class="Apple-converted-space"> </span><br class="">> + SOCKET old;<span class="Apple-converted-space"> </span><br class="">> + LONG *dest;<span class="Apple-converted-space"> </span><br class="">> + index = socket_to_index(s) % CACHE_SIZE;<span class="Apple-converted-space"> </span><br class=""><br class="">If you always mod the result of socket_to_index() by CACHE_SIZE, why not do that in that function?<span class="Apple-converted-space"> </span><br class=""><br class=""></blockquote></div><span class="mailbox-inline-edit"></span><div class=""></div><span class="mailbox-inline-edit">Point taken. </span><div class=""><br class=""><div class=""><blockquote class="gmail_quote"><br class="">> + for (depth = 0; depth < CACHE_DEPTH; ++depth) {<span class="Apple-converted-space"> </span><br class="">> + if (socket_cache[index][depth] == 0)<span class="Apple-converted-space"> </span><br class="">> + break;<span class="Apple-converted-space"> </span><br class="">> + }<span class="Apple-converted-space"> </span><br class="">> +<span class="Apple-converted-space"> </span><br class="">> + if (depth == CACHE_DEPTH) {<span class="Apple-converted-space"> </span><br class="">> + ERR("Socket hash table collision\n");<span class="Apple-converted-space"> </span><br class=""><br class="">This should exit here or you access beyond the end of socket_cache[index] just below.<span class="Apple-converted-space"> </span><br class=""><br class=""></blockquote></div></div><span class="mailbox-inline-edit"></span><div class=""></div><span class="mailbox-inline-edit">Good catch. </span><div class=""><br class=""><div class=""><div class=""><blockquote class="gmail_quote"><br class="">> + }<span class="Apple-converted-space"> </span><br class="">> +<span class="Apple-converted-space"> </span><br class="">> + dest = (PLONG)&socket_cache[index][depth];<span class="Apple-converted-space"> </span><br class=""><br class="">Don't use PLONG, just use LONG*.<span class="Apple-converted-space"> </span><br class=""><br class=""></blockquote></div></div></div><span class="mailbox-inline-edit"></span><div class=""></div><span class="mailbox-inline-edit">Ok</span><div class=""><br class=""><div class=""><div class=""><div class=""><blockquote class="gmail_quote"><br class="">> + old = InterlockedExchange(dest, s);<span class="Apple-converted-space"> </span><br class="">> +<span class="Apple-converted-space"> </span><br class="">> + if (old != 0) {<span class="Apple-converted-space"> </span><br class="">> + ERR("Socket hash table internal corruption");<span class="Apple-converted-space"> </span><br class=""><br class="">Reporting corruption isn't right. You should use InterlockedCompareExchange() with 0 as the comparand to _avoid_ corruption. If it fails, loop back to the depth loop and try again. The only error possible should be filling the hash table bucket.<span class="Apple-converted-space"> </span><br class=""><br class=""></blockquote></div></div></div></div><span class="mailbox-inline-edit"></span><div class=""></div><span class="mailbox-inline-edit">Right. Makes sense. Will fix. </span><div class=""><br class=""><div class=""><div class=""><div class=""><div class=""><blockquote class="gmail_quote"><br class="">> + }<span class="Apple-converted-space"> </span><br class="">> +}<span class="Apple-converted-space"> </span><br class="">> +<span class="Apple-converted-space"> </span><br class="">> +static BOOL remove_from_cache(SOCKET s)<span class="Apple-converted-space"> </span><br class="">> +{<span class="Apple-converted-space"> </span><br class="">> + int index,depth;<span class="Apple-converted-space"> </span><br class="">> + SOCKET old;<span class="Apple-converted-space"> </span><br class="">> + LONG *dest;<span class="Apple-converted-space"> </span><br class="">> + index = socket_to_index(s) % CACHE_SIZE;<span class="Apple-converted-space"> </span><br class="">> + for (depth = 0; depth < CACHE_DEPTH; ++depth) {<span class="Apple-converted-space"> </span><br class="">> + if (socket_cache[index][depth] == s)<span class="Apple-converted-space"> </span><br class="">> + break;<span class="Apple-converted-space"> </span><br class="">> + }<span class="Apple-converted-space"> </span><br class="">> +<span class="Apple-converted-space"> </span><br class="">> + if (depth == CACHE_DEPTH) {<span class="Apple-converted-space"> </span><br class="">> + return FALSE;<span class="Apple-converted-space"> </span><br class="">> + }<span class="Apple-converted-space"> </span><br class="">> +<span class="Apple-converted-space"> </span><br class="">> + dest = (PLONG)&socket_cache[index][depth];<span class="Apple-converted-space"> </span><br class="">> + old = InterlockedExchange(dest, 0);<span class="Apple-converted-space"> </span><br class="">> + return (old == s);<span class="Apple-converted-space"> </span><br class=""><br class="">Under what circumstances could old not equal s? If that can happen, then this should also use InterlockedCompareExchange(), otherwise you've removed some other socket.<span class="Apple-converted-space"> </span><br class=""><br class=""></blockquote></div></div></div></div></div><span class="mailbox-inline-edit"></span><div class=""></div><span class="mailbox-inline-edit">If I fork a child, I'll inherit their sockets right? So some paranoia in the remove is needed. Unless wine doesn't work that way. I'll write it as the analog to add. </span><div class=""><br class=""><div class=""><div class=""><div class=""><div class=""><div class=""><blockquote class="gmail_quote"><br class="">-Ken<span class="Apple-converted-space"> </span><br class=""><br class=""></blockquote><blockquote class="gmail_quote"></blockquote></div></div></div></div></div></div><span class="mailbox-inline-edit"></span><div class=""></div><span class="mailbox-inline-edit">Thanks! I appreciate the feedback. </span></div></div></div></blockquote></div><br class=""></body></html>