[PATCH 0/6] MR78: NSI and GetAdaptersInfo() performance improvements
This is inspired by Street Fighter V game which performance has regressed in multiplayer mode since iphlpapi reimplementation on top of NSI. Speficically, the game calls GetAdapatersInfo() very often which turnaround time increased dramatically. While I was checking the effect against GetAdapatersInfo() only I hope the patchset might improve the performance of some other iphlpapi functions as well. Here is the patch I used as a top level measure of the effect: https://gist.github.com/gofman/c08e58f15105d60b4576c9ae25bcec9d The table below lists the times of GetAdaptersInfo() in ms measured with this test (this is the time for GetAdapatersInfo(NULL, ...) for estimating the buffer length plus actual data query call. The major difference between Machine 1 and Machine 2 is most likely in the number of ip routing table entries (8 on "Machine 2" and 4 on "Machine 1"). Windows has less interfaces and routes configured so it is probably not suitable for back to back performance comparison this way although the datapoint may still be interesting for very rough comparison. The baseline here is the performance before iphlpapi redesign (which I measured on Proton 6.3). There are maybe more opportunities for optimization but after this patchset GetAdapatersInfo() performance is slightly improved over baseline so I stopped here for now. ``` Machine 1 Machine 2 Windows 0.7 Proton 6.3 2.7 8.7 Current Wine 12.2 99.8 After patch 1 10.0 59.2 After patch 4 5.5 27.2 After patch 5 3.6 20.2 After patch 6 2.4 7.6 ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/nsiproxy.sys/ndis.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/dlls/nsiproxy.sys/ndis.c b/dlls/nsiproxy.sys/ndis.c index ba4933ff16b..a3a10148109 100644 --- a/dlls/nsiproxy.sys/ndis.c +++ b/dlls/nsiproxy.sys/ndis.c @@ -495,9 +495,11 @@ static NTSTATUS ifinfo_get_all_parameters( const void *key, UINT key_size, void pthread_mutex_lock( &if_list_lock ); - update_if_table(); - - entry = find_entry_from_luid( (const NET_LUID *)key ); + if (!(entry = find_entry_from_luid( (const NET_LUID *)key ))) + { + update_if_table(); + entry = find_entry_from_luid( (const NET_LUID *)key ); + } if (entry) { ifinfo_fill_entry( entry, NULL, rw_data, dynamic_data, static_data ); @@ -557,9 +559,11 @@ static NTSTATUS ifinfo_get_parameter( const void *key, UINT key_size, UINT param pthread_mutex_lock( &if_list_lock ); - update_if_table(); - - entry = find_entry_from_luid( (const NET_LUID *)key ); + if (!(entry = find_entry_from_luid( (const NET_LUID *)key ))) + { + update_if_table(); + entry = find_entry_from_luid( (const NET_LUID *)key ); + } if (entry) { switch (param_type) @@ -591,9 +595,11 @@ static NTSTATUS index_luid_get_parameter( const void *key, UINT key_size, UINT p pthread_mutex_lock( &if_list_lock ); - update_if_table(); - - entry = find_entry_from_index( *(UINT *)key ); + if (!(entry = find_entry_from_index( *(UINT *)key ))) + { + update_if_table(); + entry = find_entry_from_index( *(UINT *)key ); + } if (entry) { *(NET_LUID *)data = entry->if_luid; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/nsiproxy.sys/ndis.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/nsiproxy.sys/ndis.c b/dlls/nsiproxy.sys/ndis.c index a3a10148109..929bda9cd90 100644 --- a/dlls/nsiproxy.sys/ndis.c +++ b/dlls/nsiproxy.sys/ndis.c @@ -644,7 +644,7 @@ BOOL convert_luid_to_unix_name( const NET_LUID *luid, const char **unix_name ) { *unix_name = entry->if_unix_name; ret = TRUE; - + break; } pthread_mutex_unlock( &if_list_lock ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/nsiproxy.sys/ndis.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/dlls/nsiproxy.sys/ndis.c b/dlls/nsiproxy.sys/ndis.c index 929bda9cd90..4d21588fbe2 100644 --- a/dlls/nsiproxy.sys/ndis.c +++ b/dlls/nsiproxy.sys/ndis.c @@ -637,8 +637,6 @@ BOOL convert_luid_to_unix_name( const NET_LUID *luid, const char **unix_name ) pthread_mutex_lock( &if_list_lock ); - update_if_table(); - LIST_FOR_EACH_ENTRY( entry, &if_list, struct if_entry, entry ) if (entry->if_luid.Value == luid->Value) { @@ -646,6 +644,21 @@ BOOL convert_luid_to_unix_name( const NET_LUID *luid, const char **unix_name ) ret = TRUE; break; } + + if (!ret) + { + update_if_table(); + while (&entry->entry != &if_list) + { + if (entry->if_luid.Value == luid->Value) + { + *unix_name = entry->if_unix_name; + ret = TRUE; + break; + } + entry = LIST_ENTRY(entry->entry.next, struct if_entry, entry); + } + } pthread_mutex_unlock( &if_list_lock ); return ret; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/nsiproxy.sys/ndis.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/dlls/nsiproxy.sys/ndis.c b/dlls/nsiproxy.sys/ndis.c index 4d21588fbe2..10b68013f42 100644 --- a/dlls/nsiproxy.sys/ndis.c +++ b/dlls/nsiproxy.sys/ndis.c @@ -616,8 +616,6 @@ BOOL convert_unix_name_to_luid( const char *unix_name, NET_LUID *luid ) pthread_mutex_lock( &if_list_lock ); - update_if_table(); - LIST_FOR_EACH_ENTRY( entry, &if_list, struct if_entry, entry ) if (!strcmp( entry->if_unix_name, unix_name )) { @@ -625,6 +623,20 @@ BOOL convert_unix_name_to_luid( const char *unix_name, NET_LUID *luid ) ret = TRUE; break; } + if (!ret) + { + update_if_table(); + while (&entry->entry != &if_list) + { + if (!strcmp( entry->if_unix_name, unix_name )) + { + *luid = entry->if_luid; + ret = TRUE; + break; + } + entry = LIST_ENTRY(entry->entry.next, struct if_entry, entry); + } + } pthread_mutex_unlock( &if_list_lock ); return ret; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/nsi/nsi.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/dlls/nsi/nsi.c b/dlls/nsi/nsi.c index efb10c0f615..345dce6dfe6 100644 --- a/dlls/nsi/nsi.c +++ b/dlls/nsi/nsi.c @@ -39,7 +39,7 @@ DWORD WINAPI NsiAllocateAndGetTable( DWORD unk, const NPI_MODULEID *module, DWOR void **rw_data, DWORD rw_size, void **dynamic_data, DWORD dynamic_size, void **static_data, DWORD static_size, DWORD *count, DWORD unk2 ) { - DWORD err, num = 0; + DWORD err, num = 64; void *data[4] = { NULL }; DWORD sizes[4] = { key_size, rw_size, dynamic_size, static_size }; int i, attempt; @@ -49,9 +49,6 @@ DWORD WINAPI NsiAllocateAndGetTable( DWORD unk, const NPI_MODULEID *module, DWOR for (attempt = 0; attempt < 5; attempt++) { - err = NsiEnumerateObjectsAllParameters( unk, 0, module, table, NULL, 0, NULL, 0, NULL, 0, NULL, 0, &num ); - if (err) return err; - for (i = 0; i < ARRAY_SIZE(data); i++) { if (sizes[i]) @@ -68,9 +65,11 @@ DWORD WINAPI NsiAllocateAndGetTable( DWORD unk, const NPI_MODULEID *module, DWOR err = NsiEnumerateObjectsAllParameters( unk, 0, module, table, data[0], sizes[0], data[1], sizes[1], data[2], sizes[2], data[3], sizes[3], &num ); if (err != ERROR_MORE_DATA) break; - + TRACE( "Short buffer, attempt %d.\n", attempt ); NsiFreeTable( data[0], data[1], data[2], data[3] ); memset( data, 0, sizeof(data) ); + err = NsiEnumerateObjectsAllParameters( unk, 0, module, table, NULL, 0, NULL, 0, NULL, 0, NULL, 0, &num ); + if (err) return err; } if (!err) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/iphlpapi/iphlpapi_main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/iphlpapi/iphlpapi_main.c b/dlls/iphlpapi/iphlpapi_main.c index 0bd7cdc06c8..5a4acdb9c5a 100644 --- a/dlls/iphlpapi/iphlpapi_main.c +++ b/dlls/iphlpapi/iphlpapi_main.c @@ -598,7 +598,6 @@ DWORD WINAPI GetAdaptersInfo( IP_ADAPTER_INFO *info, ULONG *size ) DWORD len, i, uni, fwd; NET_LUID *if_keys = NULL; struct nsi_ndis_ifinfo_rw *if_rw = NULL; - struct nsi_ndis_ifinfo_dynamic *if_dyn = NULL; struct nsi_ndis_ifinfo_static *if_stat = NULL; struct nsi_ipv4_unicast_key *uni_keys = NULL; struct nsi_ip_unicast_rw *uni_rw = NULL; @@ -612,7 +611,8 @@ DWORD WINAPI GetAdaptersInfo( IP_ADAPTER_INFO *info, ULONG *size ) err = NsiAllocateAndGetTable( 1, &NPI_MS_NDIS_MODULEID, NSI_NDIS_IFINFO_TABLE, (void **)&if_keys, sizeof(*if_keys), (void **)&if_rw, sizeof(*if_rw), - (void **)&if_dyn, sizeof(*if_dyn), (void **)&if_stat, sizeof(*if_stat), &if_count, 0 ); + NULL, 0, (void **)&if_stat, sizeof(*if_stat), &if_count, 0 ); + if (err) return err; for (i = 0; i < if_count; i++) { @@ -726,7 +726,7 @@ err: heap_free( wins_servers ); NsiFreeTable( fwd_keys, NULL, NULL, NULL ); NsiFreeTable( uni_keys, uni_rw, NULL, NULL ); - NsiFreeTable( if_keys, if_rw, if_dyn, if_stat ); + NsiFreeTable( if_keys, if_rw, NULL, if_stat ); return err; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
These changes, or at least the first few, won't pick up interface removals which we might care about. It would probably be ok to cache the results for a second or so (and force an update if the interface isn't found). A better approcach, which is on my todo list, would be a create a thread that listens on a netlink socket for interface changes (and figure out if there's an equivalent we could do on macOS). There used to be code in iphlpapi that I'd written to parse the netlink messages for Android that might be useful. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/78#note_930
On Tue May 17 08:19:21 2022 +0000, Huw Davies wrote:
These changes, or at least the first few, won't pick up interface removals which we might care about. It would probably be ok to cache the results for a second or so (and force an update if the interface isn't found). A better approcach, which is on my todo list, would be a create a thread that listens on a netlink socket for interface changes (and figure out if there's an equivalent we could do on macOS). There used to be code in iphlpapi that I'd written to parse the netlink messages for Android that might be useful. Actually, we don't de-populate the list on interface removals, so this should be ok. I'll take another look.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/78#note_934
Huw Davies (@huw) commented about dlls/nsiproxy.sys/ndis.c:
} + + if (!ret) + { + update_if_table(); + while (&entry->entry != &if_list) + { + if (entry->if_luid.Value == luid->Value) + { + *unix_name = entry->if_unix_name; + ret = TRUE; + break; + } + entry = LIST_ENTRY(entry->entry.next, struct if_entry, entry); + } + }
This is pretty fragile and assumes `update_if_table()` appends to the list - simply restart the loop again. Better yet, enclose the first loop in something like this: ```c do {} while(!updated++); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/78#note_935
participants (3)
-
Huw Davies (＠huw) -
Paul Gofman
-
Paul Gofman (＠gofman)