[PATCH v21 0/9] MR3103: server: Create and use a desktop shared mapping for GetCursorPos.
First part of Proton shared memory series. The full branch can be seen at https://gitlab.winehq.org/rbernon/wine/-/commits/mr/shared-memories.
-- v21: win32u: Use the desktop shared data for GetCursorPos. server: Move the last cursor time to the desktop session object. server: Move the cursor position to the desktop session object. win32u: Open desktop shared objects from session mapping. win32u: Add a weak reference count to the shared session. server: Allocate shared session object for desktops. win32u: Open the global session shared mapping. include: Add ReadNoFence64 inline helpers. server: Create a global session shared mapping.
From: Rémi Bernon rbernon@codeweavers.com
--- server/directory.c | 3 +++ server/file.h | 17 +++++++++++++++++ server/mapping.c | 37 +++++++++++++++++++++++++++++++++++++ server/protocol.def | 24 ++++++++++++++++++++++++ tools/make_requests | 1 + 5 files changed, 82 insertions(+)
diff --git a/server/directory.c b/server/directory.c index 23d7eb0a2b7..20a0d1d49e8 100644 --- a/server/directory.c +++ b/server/directory.c @@ -439,8 +439,10 @@ void init_directories( struct fd *intl_fd ) /* mappings */ static const WCHAR intlW[] = {'N','l','s','S','e','c','t','i','o','n','L','A','N','G','_','I','N','T','L'}; static const WCHAR user_dataW[] = {'_','_','w','i','n','e','_','u','s','e','r','_','s','h','a','r','e','d','_','d','a','t','a'}; + static const WCHAR sessionW[] = {'_','_','w','i','n','e','_','s','e','s','s','i','o','n'}; static const struct unicode_str intl_str = {intlW, sizeof(intlW)}; static const struct unicode_str user_data_str = {user_dataW, sizeof(user_dataW)}; + static const struct unicode_str session_str = {sessionW, sizeof(sessionW)};
struct directory *dir_driver, *dir_device, *dir_global, *dir_kernel, *dir_nls; struct object *named_pipe_device, *mailslot_device, *null_device; @@ -489,6 +491,7 @@ void init_directories( struct fd *intl_fd ) /* mappings */ release_object( create_fd_mapping( &dir_nls->obj, &intl_str, intl_fd, OBJ_PERMANENT, NULL )); release_object( create_user_data_mapping( &dir_kernel->obj, &user_data_str, OBJ_PERMANENT, NULL )); + release_object( create_session_mapping( &dir_kernel->obj, &session_str, OBJ_PERMANENT, NULL )); release_object( intl_fd );
release_object( named_pipe_device ); diff --git a/server/file.h b/server/file.h index 7f2d1637863..85aed168966 100644 --- a/server/file.h +++ b/server/file.h @@ -188,6 +188,23 @@ extern struct mapping *create_fd_mapping( struct object *root, const struct unic unsigned int attr, const struct security_descriptor *sd ); extern struct object *create_user_data_mapping( struct object *root, const struct unicode_str *name, unsigned int attr, const struct security_descriptor *sd ); +extern struct object *create_session_mapping( struct object *root, const struct unicode_str *name, + unsigned int attr, const struct security_descriptor *sd ); + +#define SHARED_WRITE_BEGIN( object, type ) \ + do { \ + const type *__shared = (object); \ + type *shared = (type *)__shared; \ + LONG64 __seq = shared->obj.seq + 1, __end = __seq + 1; \ + assert( (__seq & 1) != 0 ); \ + __WINE_ATOMIC_STORE_RELEASE( &shared->obj.seq, &__seq ); \ + do + +#define SHARED_WRITE_END \ + while(0); \ + assert( __seq == shared->obj.seq ); \ + __WINE_ATOMIC_STORE_RELEASE( &shared->obj.seq, &__end ); \ + } while(0)
/* device functions */
diff --git a/server/mapping.c b/server/mapping.c index 6b0de1b8b94..972b0ffb073 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -225,6 +225,15 @@ static const mem_size_t granularity_mask = 0xffff; static struct addr_range ranges32; static struct addr_range ranges64;
+struct session +{ + const session_shm_t *shared; + unsigned int object_count; + unsigned int object_capacity; +}; +static struct mapping *session_mapping; +static struct session session; + #define ROUND_SIZE(size) (((size) + page_mask) & ~page_mask)
void init_memory(void) @@ -1256,6 +1265,34 @@ int get_page_size(void) return page_mask + 1; }
+struct object *create_session_mapping( struct object *root, const struct unicode_str *name, + unsigned int attr, const struct security_descriptor *sd ) +{ + static const unsigned int access = FILE_READ_DATA | FILE_WRITE_DATA; + struct mapping *mapping; + void *tmp; + + if (!(mapping = create_mapping( root, name, attr, 0x10000, SEC_COMMIT, 0, access, sd ))) return NULL; + if ((tmp = mmap( NULL, mapping->size, PROT_READ | PROT_WRITE, MAP_SHARED, get_unix_fd( mapping->fd ), 0 )) == MAP_FAILED) + { + release_object( &mapping->obj ); + return NULL; + } + + session_mapping = mapping; + session.object_capacity = (mapping->size - offsetof(session_shm_t, objects[0])) / sizeof(session_obj_t); + session.shared = tmp; + + SHARED_WRITE_BEGIN( session.shared, session_shm_t ) + { + shared->obj.id++; + shared->size = mapping->size; + } + SHARED_WRITE_END; + + return &mapping->obj; +} + struct object *create_user_data_mapping( struct object *root, const struct unicode_str *name, unsigned int attr, const struct security_descriptor *sd ) { diff --git a/server/protocol.def b/server/protocol.def index fa78e0487f8..a677c5aad47 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -44,6 +44,7 @@ typedef unsigned __int64 mem_size_t; typedef unsigned __int64 file_pos_t; typedef unsigned __int64 client_ptr_t; typedef unsigned __int64 affinity_t; +typedef unsigned __int64 object_id_t; typedef client_ptr_t mod_handle_t;
struct request_header @@ -879,6 +880,29 @@ typedef struct lparam_t info; } cursor_pos_t;
+/****************************************************************/ +/* shared session mapping structures */ + +typedef volatile struct +{ + LONG64 seq; /* sequence number - server updating if (seq & 1) != 0 */ + object_id_t id; /* object unique id, object data is valid if != 0 */ +} object_shm_t; + +typedef volatile union +{ + object_shm_t obj; +} session_obj_t; + +typedef volatile struct +{ + object_shm_t obj; + mem_size_t size; /* session shared mapping size */ + session_obj_t objects[]; +} session_shm_t; + +C_ASSERT(sizeof(session_shm_t) == offsetof(session_shm_t, objects[0])); + /****************************************************************/ /* Request declarations */
diff --git a/tools/make_requests b/tools/make_requests index e3eaaf45b6f..f22ad279b61 100755 --- a/tools/make_requests +++ b/tools/make_requests @@ -42,6 +42,7 @@ my %formats = "file_pos_t" => [ 8, 8, "&dump_uint64" ], "mem_size_t" => [ 8, 8, "&dump_uint64" ], "affinity_t" => [ 8, 8, "&dump_uint64" ], + "object_id_t" => [ 8, 8, "&dump_uint64" ], "timeout_t" => [ 8, 8, "&dump_timeout" ], "abstime_t" => [ 8, 8, "&dump_abstime" ], "rectangle_t" => [ 16, 4, "&dump_rectangle" ],
From: Rémi Bernon rbernon@codeweavers.com
--- include/winnt.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/include/winnt.h b/include/winnt.h index 93d0a78e344..42aef90de84 100644 --- a/include/winnt.h +++ b/include/winnt.h @@ -7082,11 +7082,14 @@ static FORCEINLINE void MemoryBarrier(void) */ #if _MSC_VER >= 1700 #pragma intrinsic(__iso_volatile_load32) +#pragma intrinsic(__iso_volatile_load64) #pragma intrinsic(__iso_volatile_store32) #define __WINE_LOAD32_NO_FENCE(src) (__iso_volatile_load32(src)) +#define __WINE_LOAD64_NO_FENCE(src) (__iso_volatile_load64(src)) #define __WINE_STORE32_NO_FENCE(dest, value) (__iso_volatile_store32(dest, value)) #else /* _MSC_VER >= 1700 */ #define __WINE_LOAD32_NO_FENCE(src) (*(src)) +#define __WINE_LOAD64_NO_FENCE(src) (*(src)) #define __WINE_STORE32_NO_FENCE(dest, value) ((void)(*(dest) = (value))) #endif /* _MSC_VER >= 1700 */
@@ -7120,6 +7123,12 @@ static FORCEINLINE LONG ReadNoFence( LONG const volatile *src ) return value; }
+static FORCEINLINE LONG64 ReadNoFence64( LONG64 const volatile *src ) +{ + LONG64 value = __WINE_LOAD64_NO_FENCE( (__int64 const volatile *)src ); + return value; +} + static FORCEINLINE void WriteRelease( LONG volatile *dest, LONG value ) { __wine_memory_barrier_acq_rel(); @@ -7306,6 +7315,13 @@ static FORCEINLINE LONG ReadNoFence( LONG const volatile *src ) return value; }
+static FORCEINLINE LONG64 ReadNoFence64( LONG64 const volatile *src ) +{ + LONG64 value; + __WINE_ATOMIC_LOAD_RELAXED( src, &value ); + return value; +} + static FORCEINLINE void WriteRelease( LONG volatile *dest, LONG value ) { __WINE_ATOMIC_STORE_RELEASE( dest, &value );
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/win32u/win32u_private.h | 23 +++++++ dlls/win32u/winstation.c | 114 ++++++++++++++++++++++++++++++++++- 2 files changed, 136 insertions(+), 1 deletion(-)
diff --git a/dlls/win32u/win32u_private.h b/dlls/win32u/win32u_private.h index d5f010a8249..f7d25ffc6f3 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -362,4 +362,27 @@ static inline BOOL intersect_rect( RECT *dst, const RECT *src1, const RECT *src2 return !IsRectEmpty( dst ); }
+#if defined(__i386__) || defined(__x86_64__) +/* this prevents compilers from incorrectly reordering non-volatile reads (e.g., memcpy) from shared memory */ +#define __SHARED_READ_FENCE do { __asm__ __volatile__( "" ::: "memory" ); } while (0) +#else +#define __SHARED_READ_FENCE __atomic_thread_fence( __ATOMIC_ACQUIRE ) +#endif + +#define SHARED_READ_BEGIN( ptr, type ) \ + do { \ + const type *shared = (ptr); \ + LONG64 __seq; \ + do { \ + while ((__seq = ReadNoFence64( &shared->obj.seq )) & 1) \ + YieldProcessor(); \ + __SHARED_READ_FENCE; \ + do + +#define SHARED_READ_END \ + while (0); \ + __SHARED_READ_FENCE; \ + } while (ReadNoFence64( &shared->obj.seq ) != __seq); \ + } while(0) + #endif /* __WINE_WIN32U_PRIVATE */ diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index 6ddd5411f94..e5fe36ff566 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -22,9 +22,12 @@ #pragma makedep unix #endif
+#include <stdarg.h> +#include <stddef.h> +#include <pthread.h> + #include "ntstatus.h" #define WIN32_NO_STATUS -#include <stdarg.h> #include "windef.h" #include "winbase.h" #include "ntuser.h" @@ -40,6 +43,113 @@ WINE_DECLARE_DEBUG_CHANNEL(win);
#define DESKTOP_ALL_ACCESS 0x01ff
+static pthread_mutex_t session_lock = PTHREAD_MUTEX_INITIALIZER; +static struct shared_session *shared_session; + +struct shared_session +{ + LONG ref; + UINT64 id; + SIZE_T size; + UINT object_capacity; + const session_shm_t *shared; +}; + +static struct shared_session *shared_session_acquire( struct shared_session *session ) +{ + int ref = InterlockedIncrement( &session->ref ); + TRACE( "session %p incrementing ref to %d\n", session, ref ); + return session; +} + +static void shared_session_release( struct shared_session *session ) +{ + int ref = InterlockedDecrement( &session->ref ); + TRACE( "session %p decrementing ref to %d\n", session, ref ); + if (!ref) + { + NtUnmapViewOfSection( GetCurrentProcess(), (void *)session->shared ); + free( session ); + } +} + +static NTSTATUS map_shared_session_section( struct shared_session *session, HANDLE handle ) +{ + NTSTATUS status; + + while (!(status = NtMapViewOfSection( handle, GetCurrentProcess(), (void **)&session->shared, 0, 0, + NULL, &session->size, ViewUnmap, 0, PAGE_READONLY ))) + { + BOOL valid; + + SHARED_READ_BEGIN( session->shared, session_shm_t ) + { + if ((valid = session->size == shared->size)) + session->id = shared->obj.id; + } + SHARED_READ_END; + if (valid) break; + + NtUnmapViewOfSection( GetCurrentProcess(), (void *)session->shared ); + session->shared = NULL; + session->size = 0; + } + + session->object_capacity = (session->size - offsetof(session_shm_t, objects[0])) / sizeof(session_obj_t); + return status; +} + +static struct shared_session *get_shared_session( BOOL force ) +{ + struct shared_session *session; + + pthread_mutex_lock( &session_lock ); + + if (force || !shared_session) + { + static const WCHAR nameW[] = + { + '\','K','e','r','n','e','l','O','b','j','e','c','t','s','\', + '_','_','w','i','n','e','_','s','e','s','s','i','o','n',0 + }; + UNICODE_STRING name = RTL_CONSTANT_STRING( nameW ); + OBJECT_ATTRIBUTES attr; + unsigned int status; + HANDLE handle; + + if (!(session = calloc( 1, sizeof(*session) ))) + { + pthread_mutex_unlock( &session_lock ); + return NULL; + } + session->ref = 1; + + InitializeObjectAttributes( &attr, &name, 0, NULL, NULL ); + if (!(status = NtOpenSection( &handle, SECTION_MAP_READ | SECTION_QUERY, &attr ))) + { + status = map_shared_session_section( session, handle ); + NtClose( handle ); + } + + if (status) + { + ERR( "Failed to map session mapping, status %#x\n", status ); + free( session ); + } + else + { + if (shared_session) shared_session_release( shared_session ); + shared_session = session; + } + } + + session = shared_session_acquire( shared_session ); + + pthread_mutex_unlock( &session_lock ); + + return session; +} + BOOL is_virtual_desktop(void) { HANDLE desktop = NtUserGetThreadDesktop( GetCurrentThreadId() ); @@ -636,6 +746,8 @@ void winstation_init(void)
static const WCHAR winsta0[] = {'W','i','n','S','t','a','0',0};
+ shared_session = get_shared_session( FALSE ); + if (params->Desktop.Length) { buffer = malloc( params->Desktop.Length + sizeof(WCHAR) );
From: Rémi Bernon rbernon@codeweavers.com
--- server/file.h | 4 +++ server/mapping.c | 74 +++++++++++++++++++++++++++++++++++++++++++++ server/protocol.def | 8 +++++ server/user.h | 1 + server/winstation.c | 19 ++++++++++++ 5 files changed, 106 insertions(+)
diff --git a/server/file.h b/server/file.h index 85aed168966..51fc6eb1858 100644 --- a/server/file.h +++ b/server/file.h @@ -191,6 +191,10 @@ extern struct object *create_user_data_mapping( struct object *root, const struc extern struct object *create_session_mapping( struct object *root, const struct unicode_str *name, unsigned int attr, const struct security_descriptor *sd );
+extern int alloc_shared_object(void); +extern void free_shared_object( int index ); +extern const desktop_shm_t *get_shared_desktop( int index ); + #define SHARED_WRITE_BEGIN( object, type ) \ do { \ const type *__shared = (object); \ diff --git a/server/mapping.c b/server/mapping.c index 972b0ffb073..b14c7952636 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -230,6 +230,7 @@ struct session const session_shm_t *shared; unsigned int object_count; unsigned int object_capacity; + object_id_t next_object_id; }; static struct mapping *session_mapping; static struct session session; @@ -1293,6 +1294,79 @@ struct object *create_session_mapping( struct object *root, const struct unicode return &mapping->obj; }
+static int grow_session_mapping(void) +{ + unsigned int capacity; + mem_size_t size; + int unix_fd; + void *tmp; + + capacity = session.object_capacity * 3 / 2; + size = offsetof(session_shm_t, objects[capacity]); + size = (size + page_mask) & ~((mem_size_t)page_mask); + + unix_fd = get_unix_fd( session_mapping->fd ); + if (!grow_file( unix_fd, size )) return -1; + session_mapping->size = size; + + if ((tmp = mmap( NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, unix_fd, 0 )) == MAP_FAILED) return -1; + munmap( (void *)session.shared, session.shared->size ); + session.shared = tmp; + + SHARED_WRITE_BEGIN( session.shared, session_shm_t ) + { + shared->obj.id++; + shared->size = size; + } + SHARED_WRITE_END; + + return 0; +} + +int alloc_shared_object(void) +{ + int index; + + if (session.object_count < session.object_capacity) + index = session.object_count++; + else + { + for (index = 0; index < session.object_count; index++) + if (!session.shared->objects[index].obj.id) + break; + if (index == session.object_count) + { + if (grow_session_mapping()) return -1; + index = session.object_count++; + } + } + + SHARED_WRITE_BEGIN( &session.shared->objects[index], session_obj_t ) + { + shared->obj.id = ++session.next_object_id; + } + SHARED_WRITE_END; + + return index; +} + +void free_shared_object( int index ) +{ + if (index < 0) return; + + SHARED_WRITE_BEGIN( &session.shared->objects[index], session_obj_t ) + { + shared->obj.id = 0; + } + SHARED_WRITE_END; +} + +const desktop_shm_t *get_shared_desktop( int index ) +{ + if (index < 0) return NULL; + return &session.shared->objects[index].desktop; +} + struct object *create_user_data_mapping( struct object *root, const struct unicode_str *name, unsigned int attr, const struct security_descriptor *sd ) { diff --git a/server/protocol.def b/server/protocol.def index a677c5aad47..34b382ac2d9 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -889,9 +889,15 @@ typedef volatile struct object_id_t id; /* object unique id, object data is valid if != 0 */ } object_shm_t;
+typedef volatile struct +{ + object_shm_t obj; +} desktop_shm_t; + typedef volatile union { object_shm_t obj; + desktop_shm_t desktop; } session_obj_t;
typedef volatile struct @@ -2810,6 +2816,8 @@ enum coords_relative thread_id_t tid; /* thread id */ @REPLY obj_handle_t handle; /* handle to the desktop */ + int index; /* index of desktop object in session shared memory */ + object_id_t object_id; /* id of the session object */ @END
diff --git a/server/user.h b/server/user.h index b4cf618f87e..599ffd68c19 100644 --- a/server/user.h +++ b/server/user.h @@ -80,6 +80,7 @@ struct desktop unsigned int users; /* processes and threads using this desktop */ struct global_cursor cursor; /* global cursor information */ unsigned char keystate[256]; /* asynchronous key state */ + int session_index; /* desktop index in session shared memory */ };
/* user handles functions */ diff --git a/server/winstation.c b/server/winstation.c index 373a1f7ba92..4de5dd69039 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -296,6 +296,13 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned memset( desktop->keystate, 0, sizeof(desktop->keystate) ); list_add_tail( &winstation->desktops, &desktop->entry ); list_init( &desktop->hotkeys ); + desktop->session_index = -1; + + if ((desktop->session_index = alloc_shared_object()) == -1) + { + release_object( desktop ); + return NULL; + } } else { @@ -363,6 +370,7 @@ static void desktop_destroy( struct object *obj ) if (desktop->close_timeout) remove_timeout_user( desktop->close_timeout ); list_remove( &desktop->entry ); release_object( desktop->winstation ); + free_shared_object( desktop->session_index ); }
/* retrieve the thread desktop, checking the handle access rights */ @@ -726,10 +734,21 @@ DECL_HANDLER(close_desktop) /* get the thread current desktop */ DECL_HANDLER(get_thread_desktop) { + struct desktop *desktop; struct thread *thread;
if (!(thread = get_thread_from_id( req->tid ))) return; reply->handle = thread->desktop; + reply->index = -1; + + if ((desktop = get_thread_desktop( thread, 0 ))) + { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); + reply->index = desktop->session_index; + reply->object_id = desktop_shm->obj.id; + release_object( desktop ); + } + release_object( thread ); }
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/win32u/winstation.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index e5fe36ff566..deb4cd0bf02 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -49,27 +49,45 @@ static struct shared_session *shared_session; struct shared_session { LONG ref; + LONG weak_ref; UINT64 id; SIZE_T size; UINT object_capacity; const session_shm_t *shared; };
+/* try acquiring a strong reference from a possibly weak referenced session */ static struct shared_session *shared_session_acquire( struct shared_session *session ) { - int ref = InterlockedIncrement( &session->ref ); - TRACE( "session %p incrementing ref to %d\n", session, ref ); - return session; + if (InterlockedOr( &session->ref, 0 )) + { + int ref = InterlockedIncrement( &session->ref ); + TRACE( "session %p incrementing ref to %d\n", session, ref ); + return session; + } + + TRACE( "session %p failed to upgrade from weak ref\n", session ); + return NULL; +} + +/* release a weak reference on a session */ +static void shared_session_release_weak( struct shared_session *session ) +{ + int ref = InterlockedDecrement( &session->weak_ref ); + TRACE( "session %p decrementing weak_ref to %d\n", session, ref ); + if (!ref) free( session ); }
+/* release a strong reference on a session */ static void shared_session_release( struct shared_session *session ) { int ref = InterlockedDecrement( &session->ref ); TRACE( "session %p decrementing ref to %d\n", session, ref ); if (!ref) { + /* global shared_session has a strong reference, unmap when strong refcount reaches 0. */ NtUnmapViewOfSection( GetCurrentProcess(), (void *)session->shared ); - free( session ); + shared_session_release_weak( session ); } }
@@ -99,6 +117,7 @@ static NTSTATUS map_shared_session_section( struct shared_session *session, HAND return status; }
+/* return a strong reference on the last known valid session */ static struct shared_session *get_shared_session( BOOL force ) { struct shared_session *session; @@ -123,6 +142,7 @@ static struct shared_session *get_shared_session( BOOL force ) return NULL; } session->ref = 1; + session->weak_ref = 1;
InitializeObjectAttributes( &attr, &name, 0, NULL, NULL ); if (!(status = NtOpenSection( &handle, SECTION_MAP_READ | SECTION_QUERY, &attr )))
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/win32u/ntuser_private.h | 1 + dlls/win32u/sysparams.c | 1 + dlls/win32u/win32u_private.h | 14 ++++ dlls/win32u/winstation.c | 129 +++++++++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+)
diff --git a/dlls/win32u/ntuser_private.h b/dlls/win32u/ntuser_private.h index 3b6cab5bdc9..524195941f3 100644 --- a/dlls/win32u/ntuser_private.h +++ b/dlls/win32u/ntuser_private.h @@ -127,6 +127,7 @@ struct user_thread_info UINT spy_indent; /* Current spy indent */ BOOL clipping_cursor; /* thread is currently clipping */ DWORD clipping_reset; /* time when clipping was last reset */ + struct session_object *shared_desktop; /* thread desktop shared session object */ };
C_ASSERT( sizeof(struct user_thread_info) <= sizeof(((TEB *)0)->Win32ClientInfo) ); diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index a31d586a5b6..7ccdd544633 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -6262,6 +6262,7 @@ static void thread_detach(void) destroy_thread_windows(); cleanup_imm_thread(); NtClose( thread_info->server_queue ); + if (thread_info->shared_desktop) session_object_release_weak( thread_info->shared_desktop );
exiting_thread_id = 0; } diff --git a/dlls/win32u/win32u_private.h b/dlls/win32u/win32u_private.h index f7d25ffc6f3..1d3626be3f5 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -194,6 +194,20 @@ extern void user_unlock(void); extern void user_check_not_lock(void);
/* winstation.c */ + +struct shared_session; +struct session_object +{ + LONG ref; + UINT64 id; + const session_obj_t *shared; + struct shared_session *session; +}; + +extern void session_object_release( struct session_object *desktop ); +extern void session_object_release_weak( struct session_object *desktop ); +extern struct session_object *get_shared_desktop( BOOL force ); + extern BOOL is_virtual_desktop(void);
/* window.c */ diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index deb4cd0bf02..647204e396a 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -56,6 +56,13 @@ struct shared_session const session_shm_t *shared; };
+/* acquire a weak reference on a session */ +static void shared_session_acquire_weak( struct shared_session *session ) +{ + int ref = InterlockedIncrement( &session->weak_ref ); + TRACE( "session %p incrementing weak_ref to %d\n", session, ref ); +} + /* try acquiring a strong reference from a possibly weak referenced session */ static struct shared_session *shared_session_acquire( struct shared_session *session ) { @@ -170,6 +177,125 @@ static struct shared_session *get_shared_session( BOOL force ) return session; }
+/* acquire an object with a weak reference on its session */ +static struct session_object *session_object_acquire_weak( struct session_object *object ) +{ + int ref = InterlockedIncrement( &object->ref ); + TRACE( "object %p incrementing ref to %d\n", object, ref ); + return object; +} + +/* try acquiring an object with a strong reference on its session */ +static struct session_object *session_object_acquire( struct session_object *object ) +{ + /* try upgrading the session reference, which fails if the session has been invalidated */ + if (object && shared_session_acquire( object->session )) + return session_object_acquire_weak( object ); + + TRACE( "object %p failed to upgrade from weak ref\n", object ); + return NULL; +} + +/* release an object with a weak session reference */ +void session_object_release_weak( struct session_object *object ) +{ + int ref = InterlockedDecrement( &object->ref ); + TRACE( "object %p decrementing ref to %d\n", object, ref ); + if (!ref) + { + shared_session_release_weak( object->session ); + free( object ); + } +} + +/* release an object with a strong session reference */ +void session_object_release( struct session_object *object ) +{ + shared_session_release( object->session ); + session_object_release_weak( object ); +} + +enum object_type +{ + OBJECT_TYPE_DESKTOP = 1, +}; + +static int get_thread_session_object_index( UINT tid, enum object_type type, UINT64 *id ) +{ + switch (type) + { + case OBJECT_TYPE_DESKTOP: + SERVER_START_REQ( get_thread_desktop ) + { + req->tid = tid; + if (wine_server_call_err( req )) return -1; + *id = reply->object_id; + return reply->index; + } + SERVER_END_REQ; + break; + } + + return -1; +} + +/* return a strong reference on the last known session object for a thread id and type */ +static struct session_object *get_thread_session_object( UINT tid, enum object_type type ) +{ + struct shared_session *session; + struct session_object *object; + BOOL valid = TRUE; + int index; + + TRACE( "tid %04x, type %u\n", tid, type ); + + if (!(object = calloc( 1, sizeof(*object) ))) return NULL; + object->ref = 1; + + while ((session = get_shared_session( !valid ))) + { + if ((index = get_thread_session_object_index( tid, type, &object->id )) < 0) break; + if ((valid = index < session->object_capacity)) + { + object->shared = &session->shared->objects[index]; + break; + } + shared_session_release( session ); + } + + if (!object->shared) + { + WARN( "Failed to find object type %u for thread %04x\n", type, tid ); + if (session) shared_session_release( session ); + free( object ); + return NULL; + } + + /* every object holds an additional weak ref on their session */ + shared_session_acquire_weak( session ); + object->session = session; + + TRACE( "returning session %p, object %p\n", session, object ); + return object; +} + +struct session_object *get_shared_desktop( BOOL force ) +{ + struct user_thread_info *thread_info = get_user_thread_info(); + struct session_object *desktop; + + TRACE( "force %u\n", force ); + + if (force || !(desktop = session_object_acquire( thread_info->shared_desktop ))) + { + if (!(desktop = get_thread_session_object( GetCurrentThreadId(), OBJECT_TYPE_DESKTOP ))) return NULL; + if (thread_info->shared_desktop) session_object_release_weak( thread_info->shared_desktop ); + thread_info->shared_desktop = session_object_acquire_weak( desktop ); + } + + return desktop; +} + BOOL is_virtual_desktop(void) { HANDLE desktop = NtUserGetThreadDesktop( GetCurrentThreadId() ); @@ -394,6 +520,9 @@ BOOL WINAPI NtUserSetThreadDesktop( HDESK handle ) thread_info->client_info.top_window = 0; thread_info->client_info.msg_window = 0; if (key_state_info) key_state_info->time = 0; + if (thread_info->shared_desktop) session_object_release_weak( thread_info->shared_desktop ); + thread_info->shared_desktop = NULL; + get_shared_desktop( FALSE ); if (was_virtual_desktop != is_virtual_desktop()) update_display_cache( TRUE ); } return ret;
From: Rémi Bernon rbernon@codeweavers.com
Based on a patch by Huw Davies huw@codeweavers.com. --- server/protocol.def | 7 +++++ server/queue.c | 73 ++++++++++++++++++++++++++++----------------- server/user.h | 2 -- 3 files changed, 52 insertions(+), 30 deletions(-)
diff --git a/server/protocol.def b/server/protocol.def index 34b382ac2d9..577ff2a520a 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -883,6 +883,12 @@ typedef struct /****************************************************************/ /* shared session mapping structures */
+struct shared_cursor +{ + int x; /* cursor position */ + int y; +}; + typedef volatile struct { LONG64 seq; /* sequence number - server updating if (seq & 1) != 0 */ @@ -892,6 +898,7 @@ typedef volatile struct typedef volatile struct { object_shm_t obj; + struct shared_cursor cursor; /* global cursor information */ } desktop_shm_t;
typedef volatile union diff --git a/server/queue.c b/server/queue.c index 4373bd71254..cf23628e77d 100644 --- a/server/queue.c +++ b/server/queue.c @@ -418,6 +418,7 @@ static void queue_cursor_message( struct desktop *desktop, user_handle_t win, un lparam_t wparam, lparam_t lparam ) { static const struct hw_msg_source source = { IMDT_UNAVAILABLE, IMO_SYSTEM }; + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); struct thread_input *input; struct message *msg;
@@ -426,8 +427,8 @@ static void queue_cursor_message( struct desktop *desktop, user_handle_t win, un msg->msg = message; msg->wparam = wparam; msg->lparam = lparam; - msg->x = desktop->cursor.x; - msg->y = desktop->cursor.y; + msg->x = desktop_shm->cursor.x; + msg->y = desktop_shm->cursor.y; if (!(msg->win = win) && (input = desktop->foreground_input)) msg->win = input->active; queue_hardware_message( desktop, msg, 1 ); } @@ -465,13 +466,20 @@ static int update_desktop_cursor_window( struct desktop *desktop, user_handle_t
static int update_desktop_cursor_pos( struct desktop *desktop, user_handle_t win, int x, int y ) { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); int updated;
x = max( min( x, desktop->cursor.clip.right - 1 ), desktop->cursor.clip.left ); y = max( min( y, desktop->cursor.clip.bottom - 1 ), desktop->cursor.clip.top ); - updated = (desktop->cursor.x != x || desktop->cursor.y != y); - desktop->cursor.x = x; - desktop->cursor.y = y; + + SHARED_WRITE_BEGIN( desktop_shm, desktop_shm_t ) + { + updated = shared->cursor.x != x || shared->cursor.y != y; + shared->cursor.x = x; + shared->cursor.y = y; + } + SHARED_WRITE_END; + desktop->cursor.last_change = get_tick_count();
if (!win || !is_window_visible( win ) || is_window_transparent( win )) @@ -517,15 +525,17 @@ static void set_cursor_pos( struct desktop *desktop, int x, int y ) static void get_message_defaults( struct msg_queue *queue, int *x, int *y, unsigned int *time ) { struct desktop *desktop = queue->input->desktop; + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index );
- *x = desktop->cursor.x; - *y = desktop->cursor.y; + *x = desktop_shm->cursor.x; + *y = desktop_shm->cursor.y; *time = get_tick_count(); }
/* set the cursor clip rectangle */ void set_clip_rectangle( struct desktop *desktop, const rectangle_t *rect, unsigned int flags, int reset ) { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); rectangle_t top_rect; int x, y;
@@ -543,9 +553,9 @@ void set_clip_rectangle( struct desktop *desktop, const rectangle_t *rect, unsig else desktop->cursor.clip = top_rect;
/* warp the mouse to be inside the clip rect */ - x = max( min( desktop->cursor.x, desktop->cursor.clip.right - 1 ), desktop->cursor.clip.left ); - y = max( min( desktop->cursor.y, desktop->cursor.clip.bottom - 1 ), desktop->cursor.clip.top ); - if (x != desktop->cursor.x || y != desktop->cursor.y) set_cursor_pos( desktop, x, y ); + x = max( min( desktop_shm->cursor.x, desktop->cursor.clip.right - 1 ), desktop->cursor.clip.left ); + y = max( min( desktop_shm->cursor.y, desktop->cursor.clip.bottom - 1 ), desktop->cursor.clip.top ); + if (x != desktop_shm->cursor.x || y != desktop_shm->cursor.y) set_cursor_pos( desktop, x, y );
/* request clip cursor rectangle reset to the desktop thread */ if (reset) post_desktop_message( desktop, WM_WINE_CLIPCURSOR, flags, FALSE ); @@ -1666,6 +1676,7 @@ static void prepend_cursor_history( int x, int y, unsigned int time, lparam_t in /* queue a hardware message into a given thread input */ static void queue_hardware_message( struct desktop *desktop, struct message *msg, int always_queue ) { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); user_handle_t win; struct thread *thread; struct thread_input *input; @@ -1698,8 +1709,8 @@ static void queue_hardware_message( struct desktop *desktop, struct message *msg if (desktop->keystate[VK_XBUTTON2] & 0x80) msg->wparam |= MK_XBUTTON2; break; } - msg->x = desktop->cursor.x; - msg->y = desktop->cursor.y; + msg->x = desktop_shm->cursor.x; + msg->y = desktop_shm->cursor.y;
if (msg->win && (thread = get_window_thread( msg->win ))) { @@ -1981,6 +1992,7 @@ static void dispatch_rawinput_message( struct desktop *desktop, struct rawinput_ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, const hw_input_t *input, unsigned int origin, struct msg_queue *sender ) { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); const struct rawinput_device *device; struct hardware_msg_data *msg_data; struct rawinput_message raw_msg; @@ -2019,19 +2031,19 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons x = input->mouse.x; y = input->mouse.y; if (flags & ~(MOUSEEVENTF_MOVE | MOUSEEVENTF_ABSOLUTE) && - x == desktop->cursor.x && y == desktop->cursor.y) + x == desktop_shm->cursor.x && y == desktop_shm->cursor.y) flags &= ~MOUSEEVENTF_MOVE; } else { - x = desktop->cursor.x + input->mouse.x; - y = desktop->cursor.y + input->mouse.y; + x = desktop_shm->cursor.x + input->mouse.x; + y = desktop_shm->cursor.y + input->mouse.y; } } else { - x = desktop->cursor.x; - y = desktop->cursor.y; + x = desktop_shm->cursor.x; + y = desktop_shm->cursor.y; }
if ((foreground = get_foreground_thread( desktop, win ))) @@ -2042,7 +2054,7 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons raw_msg.time = time; raw_msg.message = WM_INPUT; raw_msg.flags = flags; - rawmouse_init( &raw_msg.rawinput, &raw_msg.data.mouse, x - desktop->cursor.x, y - desktop->cursor.y, + rawmouse_init( &raw_msg.rawinput, &raw_msg.data.mouse, x - desktop_shm->cursor.x, y - desktop_shm->cursor.y, raw_msg.flags, input->mouse.data, input->mouse.info );
dispatch_rawinput_message( desktop, &raw_msg ); @@ -2220,6 +2232,7 @@ static int queue_keyboard_message( struct desktop *desktop, user_handle_t win, c static void queue_custom_hardware_message( struct desktop *desktop, user_handle_t win, unsigned int origin, const hw_input_t *input ) { + const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); struct hw_msg_source source = { IMDT_UNAVAILABLE, origin }; struct thread *foreground; struct message *msg; @@ -2252,8 +2265,8 @@ static void queue_custom_hardware_message( struct desktop *desktop, user_handle_ msg->msg = input->hw.msg; msg->wparam = 0; msg->lparam = input->hw.lparam; - msg->x = desktop->cursor.x; - msg->y = desktop->cursor.y; + msg->x = desktop_shm->cursor.x; + msg->y = desktop_shm->cursor.y;
queue_hardware_message( desktop, msg, 1 ); } @@ -2789,6 +2802,7 @@ DECL_HANDLER(send_hardware_message) struct desktop *desktop; unsigned int origin = (req->flags & SEND_HWMSG_INJECTED ? IMO_INJECTED : IMO_HARDWARE); struct msg_queue *sender = get_current_queue(); + const desktop_shm_t *desktop_shm;
if (!(desktop = get_hardware_input_desktop( req->win ))) return; if ((origin == IMO_INJECTED && desktop != current->queue->input->desktop) || @@ -2798,9 +2812,10 @@ DECL_HANDLER(send_hardware_message) set_error( STATUS_ACCESS_DENIED ); return; } + desktop_shm = get_shared_desktop( desktop->session_index );
- reply->prev_x = desktop->cursor.x; - reply->prev_y = desktop->cursor.y; + reply->prev_x = desktop_shm->cursor.x; + reply->prev_y = desktop_shm->cursor.y;
switch (req->input.type) { @@ -2817,8 +2832,8 @@ DECL_HANDLER(send_hardware_message) set_error( STATUS_INVALID_PARAMETER ); }
- reply->new_x = desktop->cursor.x; - reply->new_y = desktop->cursor.y; + reply->new_x = desktop_shm->cursor.x; + reply->new_y = desktop_shm->cursor.y; release_object( desktop ); }
@@ -3503,15 +3518,17 @@ DECL_HANDLER(set_cursor) struct msg_queue *queue = get_current_queue(); struct thread_input *input; struct desktop *desktop; + const desktop_shm_t *desktop_shm;
if (!queue) return; input = queue->input; desktop = input->desktop; + desktop_shm = get_shared_desktop( desktop->session_index );
reply->prev_handle = input->cursor; reply->prev_count = input->cursor_count; - reply->prev_x = desktop->cursor.x; - reply->prev_y = desktop->cursor.y; + reply->prev_x = desktop_shm->cursor.x; + reply->prev_y = desktop_shm->cursor.y;
if (req->flags & SET_CURSOR_HANDLE) { @@ -3534,8 +3551,8 @@ DECL_HANDLER(set_cursor) if (req->flags & (SET_CURSOR_HANDLE | SET_CURSOR_COUNT)) update_desktop_cursor_handle( desktop, input );
- reply->new_x = desktop->cursor.x; - reply->new_y = desktop->cursor.y; + reply->new_x = desktop_shm->cursor.x; + reply->new_y = desktop_shm->cursor.y; reply->new_clip = desktop->cursor.clip; reply->last_change = desktop->cursor.last_change; } diff --git a/server/user.h b/server/user.h index 599ffd68c19..dd29b1b1be5 100644 --- a/server/user.h +++ b/server/user.h @@ -56,8 +56,6 @@ struct winstation
struct global_cursor { - int x; /* cursor position */ - int y; rectangle_t clip; /* cursor clip rectangle */ unsigned int last_change; /* time of last position change */ user_handle_t win; /* window that contains the cursor */
From: Rémi Bernon rbernon@codeweavers.com
Based on a patch by Huw Davies huw@codeweavers.com. --- server/protocol.def | 1 + server/queue.c | 17 +++++++++++------ server/user.h | 1 - 3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/server/protocol.def b/server/protocol.def index 577ff2a520a..b6707ae776f 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -887,6 +887,7 @@ struct shared_cursor { int x; /* cursor position */ int y; + unsigned int last_change; /* time of last position change */ };
typedef volatile struct diff --git a/server/queue.c b/server/queue.c index cf23628e77d..d829e97b420 100644 --- a/server/queue.c +++ b/server/queue.c @@ -468,6 +468,7 @@ static int update_desktop_cursor_pos( struct desktop *desktop, user_handle_t win { const desktop_shm_t *desktop_shm = get_shared_desktop( desktop->session_index ); int updated; + unsigned int time = get_tick_count();
x = max( min( x, desktop->cursor.clip.right - 1 ), desktop->cursor.clip.left ); y = max( min( y, desktop->cursor.clip.bottom - 1 ), desktop->cursor.clip.top ); @@ -477,11 +478,10 @@ static int update_desktop_cursor_pos( struct desktop *desktop, user_handle_t win updated = shared->cursor.x != x || shared->cursor.y != y; shared->cursor.x = x; shared->cursor.y = y; + shared->cursor.last_change = time; } SHARED_WRITE_END;
- desktop->cursor.last_change = get_tick_count(); - if (!win || !is_window_visible( win ) || is_window_transparent( win )) win = shallow_window_from_point( desktop, x, y ); if (update_desktop_cursor_window( desktop, win )) updated = 1; @@ -1998,7 +1998,7 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons struct rawinput_message raw_msg; struct message *msg; struct thread *foreground; - unsigned int i, time, flags; + unsigned int i, time = get_tick_count(), flags; struct hw_msg_source source = { IMDT_MOUSE, origin }; int wait = 0, x, y;
@@ -2019,10 +2019,15 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons WM_MOUSEHWHEEL /* 0x1000 = MOUSEEVENTF_HWHEEL */ };
- desktop->cursor.last_change = get_tick_count(); + SHARED_WRITE_BEGIN( desktop_shm, desktop_shm_t ) + { + shared->cursor.last_change = time; + } + SHARED_WRITE_END; + flags = input->mouse.flags; time = input->mouse.time; - if (!time) time = desktop->cursor.last_change; + if (!time) time = desktop_shm->cursor.last_change;
if (flags & MOUSEEVENTF_MOVE) { @@ -3554,7 +3559,7 @@ DECL_HANDLER(set_cursor) reply->new_x = desktop_shm->cursor.x; reply->new_y = desktop_shm->cursor.y; reply->new_clip = desktop->cursor.clip; - reply->last_change = desktop->cursor.last_change; + reply->last_change = desktop_shm->cursor.last_change; }
/* Get the history of the 64 last cursor positions */ diff --git a/server/user.h b/server/user.h index dd29b1b1be5..dc64b9c6003 100644 --- a/server/user.h +++ b/server/user.h @@ -57,7 +57,6 @@ struct winstation struct global_cursor { rectangle_t clip; /* cursor clip rectangle */ - unsigned int last_change; /* time of last position change */ user_handle_t win; /* window that contains the cursor */ };
From: Rémi Bernon rbernon@codeweavers.com
Based on a patch by Huw Davies huw@codeweavers.com. --- dlls/win32u/input.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/dlls/win32u/input.c b/dlls/win32u/input.c index ef8d564c264..e05fc90bd8d 100644 --- a/dlls/win32u/input.c +++ b/dlls/win32u/input.c @@ -749,22 +749,29 @@ BOOL WINAPI NtUserSetCursorPos( INT x, INT y ) */ BOOL get_cursor_pos( POINT *pt ) { - BOOL ret; - DWORD last_change; + BOOL valid = TRUE, ret = FALSE; + struct session_object *object; + DWORD last_change = 0; UINT dpi;
if (!pt) return FALSE;
- SERVER_START_REQ( set_cursor ) + while ((object = get_shared_desktop( !valid ))) { - if ((ret = !wine_server_call( req ))) + SHARED_READ_BEGIN( &object->shared->desktop, desktop_shm_t ) { - pt->x = reply->new_x; - pt->y = reply->new_y; - last_change = reply->last_change; + if ((valid = object->id == shared->obj.id)) + { + pt->x = shared->cursor.x; + pt->y = shared->cursor.y; + last_change = shared->cursor.last_change; + ret = TRUE; + } } + SHARED_READ_END; + session_object_release( object ); + if (valid) break; } - SERVER_END_REQ;
/* query new position from graphics driver if we haven't updated recently */ if (ret && NtGetTickCount() - last_change > 100) ret = user_driver->pGetCursorPos( pt );
Rebased and addressed the comments above, as well as using weak ref for the session and returning object id from the server request like we discussed.
On Tue Feb 20 13:08:45 2024 +0000, Jinoh Kang wrote:
`session_mapping` should be treated as a reference.
session_mapping = (struct mapping *)grab_object( &mapping->obj );
Is there a reason this is not yet addressed? If you don't like the extra ref, I think it's better to force `(attr & OBJ_PERMANENT) != 0`, by either:
```diff + assert(attr & OBJ_PERMANENT); // ensure attr is permanent ```
or
```diff - unsigned int attr, const struct security_descriptor *sd ) + const struct security_descriptor *sd ) // don't let caller supply attr ```
On Fri Mar 1 11:06:26 2024 +0000, Jinoh Kang wrote:
Is there a reason this is not yet addressed? If you don't like the extra ref, I think it's better to force `(attr & OBJ_PERMANENT) != 0`, by either:
+ assert(attr & OBJ_PERMANENT); // ensure attr is permanentor
- unsigned int attr, const struct security_descriptor *sd ) + const struct security_descriptor *sd ) // don't let caller supply attr
I didn't see why this would be necessary as the mapping is created during server init as a permanent object. There are other cases like that, such as the root directory, or the user shared mapping, though I don't really mind using a reference. Will change.
I didn't see why this would be necessary as the mapping is created during server init as a permanent object.
There are other cases like that, such as the root directory,
`root_directory` assignment is in the caller side of `create_directory`; the function itself doesn't keep a reference. Meanwhile we both assign the pointer *and* return the pointer, hence two references.
the user shared mapping
In this case, we only store the `mmap` address (which is managed independently), not the object pointer itself. So I don't think this is applicable as a precedent either.
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
- const session_shm_t *shared;
+};
+/* acquire a weak reference on a session */ +static void shared_session_acquire_weak( struct shared_session *session ) +{
- int ref = InterlockedIncrement( &session->weak_ref );
- TRACE( "session %p incrementing weak_ref to %d\n", session, ref );
+}
+/* try acquiring a strong reference from a possibly weak referenced session */ +static struct shared_session *shared_session_acquire( struct shared_session *session ) +{
- if (InterlockedOr( &session->ref, 0 ))
- {
int ref = InterlockedIncrement( &session->ref );
It looks like we have a race: another thread may invalidate `shared_session` just before here. Is it true?
* In that case, consulting `dlls/combase/marshal.c:proxy_manager_addref_if_alive()` might be a help.
* Otherwise, please explain (in comments) how it's not possible for `ref` to concurrently drop to zero.
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
return reply->index;}SERVER_END_REQ;break;- }
- return -1;
+}
+/* return a strong reference on the last known session object for a thread id and type */ +static struct session_object *get_thread_session_object( UINT tid, enum object_type type ) +{
- struct shared_session *session;
- struct session_object *object;
- BOOL valid = TRUE;
- int index;
[-Wpedantic nit] `comparison between signed and unsigned` at `valid = index < session->object_capacity`
```suggestion:-0+0 UINT index; ```
This will also ensure we won't get negative indices on overflow (unlikely, hence nit).
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
break;}shared_session_release( session );- }
- if (!object->shared)
- {
WARN( "Failed to find object type %u for thread %04x\n", type, tid );if (session) shared_session_release( session );free( object );return NULL;- }
- /* every object holds an additional weak ref on their session */
- shared_session_acquire_weak( session );
- object->session = session;
[stylistic nit] how about returning arg as-is from `shared_session_acquire_weak` so that:
```suggestion:-1+0 object->session = shared_session_acquire_weak( session ); ```
Jinoh Kang (@iamahuman) commented about server/mapping.c:
index = session.object_count++;- else
- {
for (index = 0; index < session.object_count; index++)if (!session.shared->objects[index].obj.id)break;if (index == session.object_count){if (grow_session_mapping()) return -1;index = session.object_count++;}- }
- SHARED_WRITE_BEGIN( &session.shared->objects[index], session_obj_t )
- {
shared->obj.id = ++session.next_object_id;
Confusing variable name.
```suggestion:-0+0 shared->obj.id = ++session.last_object_id; ```
```suggestion:-0+0 shared->obj.id = session.next_object_id++; ```
Jinoh Kang (@iamahuman) commented about server/mapping.c:
- unsigned int capacity;
- mem_size_t size;
- int unix_fd;
- void *tmp;
- capacity = session.object_capacity * 3 / 2;
- size = offsetof(session_shm_t, objects[capacity]);
- size = (size + page_mask) & ~((mem_size_t)page_mask);
- unix_fd = get_unix_fd( session_mapping->fd );
- if (!grow_file( unix_fd, size )) return -1;
- session_mapping->size = size;
- if ((tmp = mmap( NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, unix_fd, 0 )) == MAP_FAILED) return -1;
- munmap( (void *)session.shared, session.shared->size );
- session.shared = tmp;
1. Ensure `session_mapping->size == session.shared.size` at all times, since we rely on it in the `map_shared_session_section` loop. - Note that we don't have to ensure `backing_file[st_size] == session_mapping->size`. It's normal for a section to only have a portion of the backing file. 2. Consistently use `mapping->size` as we've done in `create_session_mapping`.
```suggestion:-4+0 if ((tmp = mmap( NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, unix_fd, 0 )) == MAP_FAILED) return -1; munmap( (void *)session.shared, session_mapping->size ); session.shared = tmp;
session_mapping->size = size; ```
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
- int ref = InterlockedDecrement( &session->ref );
- TRACE( "session %p decrementing ref to %d\n", session, ref );
- if (!ref)
- {
/* global shared_session has a strong reference, unmap when strong refcount reaches 0. */NtUnmapViewOfSection( GetCurrentProcess(), (void *)session->shared );shared_session_release_weak( session );- }
+}
+static NTSTATUS map_shared_session_section( struct shared_session *session, HANDLE handle ) +{
- NTSTATUS status;
- while (!(status = NtMapViewOfSection( handle, GetCurrentProcess(), (void **)&session->shared, 0, 0,
NULL, &session->size, ViewUnmap, 0, PAGE_READONLY )))
We don't need to store `size` into session. It's currently made redundant by `object_capacity`.
Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
#define DESKTOP_ALL_ACCESS 0x01ff
+static pthread_mutex_t session_lock = PTHREAD_MUTEX_INITIALIZER; +static struct shared_session *shared_session;
+struct shared_session +{
- LONG ref;
- LONG weak_ref;
I think our code now matches "locking" pattern more closely than strong/weak ref analogy. In fact IMHO it's harder for reviewers to follow the code when we think of the usual "strong" vs "weak" pattern, inviting gratuitous bikeshedding (e.g., "why do shared session weakref and session object weakref behave differently?" "because keeping track of weak/strong in session objects are more complex, so I deleted it.")
I propose the following nomenclature-only changes (without changes to behavior)[^refs]:
```suggestion:-1+0 LONG refs; // previously "weak_ref". Keeps the reference count of the shared_session object itself. LONG locks; // previously "ref". Similar to GlobalLock or GdipBitmapLockBits, keeps the "lock" (don't free) count of the memory buffer. ```
In addition:
| Before | After | |:------------------------------|:--------------------------------------------| | `shared_session_acquire_weak` | `shared_session_acquire`[^1] | | `shared_session_acquire` | `shared_session_try_lock`[^2][^try] | | `shared_session_release_weak` | `shared_session_release`[^1] | | `shared_session_release` | `shared_session_unlock`[^2] | | `session_object_acquire_weak` | `session_object_acquire`[^1] | | `session_object_acquire` | `session_object_try_lock_acquire`[^3][^try] | | `session_object_release_weak` | `session_object_release`[^1] | | `session_object_release` | `session_object_unlock_release`[^3] | | `weak reference` (comment) | `reference` | | `strong reference` (comment) | `lock` or `locked reference` |
[^refs]: I made `weak_ref` plural so that it's consistent with `locks`, and more unlikely to be confused with previous `ref` (strongref) when doing find-replace. That said, I don't mind `ref` either. [^1]: Weak-only acquire/release [^2]: Strong-only acquire/release, with special case for strong=0 (since nonzero strongref implies one unique weakref) [^3]: Strong *as well as* weak acquire/release [^try]: "Try" à la [TryAcquireSRWLockShared](https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-tryacquiresrwlockshared). I think this communicates "can fail" nature of these functions. Feel free to drop `try_` if it feels too verbose.
On Sat Mar 2 03:50:36 2024 +0000, Jinoh Kang wrote:
I think this needs to be revisited at some point, but this MR as-is seems fine (except perhaps mappings duplicated for each thread). We still have some unresolved questions (e.g., how is information specific to winstation/desktop in the same session placed in the shared memory), though (out of scope of this MR).
Rewritten not to use separate mappings per thread.
On Mon Feb 19 22:56:17 2024 +0000, Rémi Bernon wrote:
changed this line in [version 19 of the diff](/wine/wine/-/merge_requests/3103/diffs?diff_id=100554&start_sha=6df7b45c26e350f3a6a318717d0f2fe68ae4fcdc#df52417d44d1b9241a6b9cbe05754e277e8d0f2d_107_106)
`object_ops` and per-thread mappings are now gone. (If you have other concerns, it's best that we discuss this in a separate thread.)
-
Jinoh Kang (＠iamahuman) -
Rémi Bernon
-
Rémi Bernon (＠rbernon)