From: Huw Davies Subject: Re: [PATCH 3/5] server: Move the cursor position and last change time to the shared data. Message-Id: <5D1CB5EB-4D80-40EB-BA59-A1C9A8490653@codeweavers.com> Date: Thu, 19 Nov 2020 14:51:35 +0000 In-Reply-To: <7e2befcb-8a3d-d5c3-a344-2320339efda4@codeweavers.com> References: <20201119130931.89653-3-huw@codeweavers.com> <294c9baa-5f40-ff73-aa9c-fb70e1d987c5@codeweavers.com> <62fb6d2e-7771-bc48-ecd9-9df765d39006@codeweavers.com> <7BD6A7A2-E25D-4F17-A6A3-680676FE6131@codeweavers.com> <7e2befcb-8a3d-d5c3-a344-2320339efda4@codeweavers.com> On 19 Nov 2020, at 14:41, Rémi Bernon wrote: > On 11/19/20 3:21 PM, Huw Davies wrote: >> On 19 Nov 2020, at 14:12, Rémi Bernon wrote: >>> On 11/19/20 3:01 PM, Huw Davies wrote: >>>> On 19 Nov 2020, at 13:42, Rémi Bernon wrote: >>>>> On 11/19/20 2:09 PM, Huw Davies wrote: >>>>>> Signed-off-by: Huw Davies >>>>>> --- >>>>>> server/protocol.def | 9 ++++++- >>>>>> server/queue.c | 62 ++++++++++++++++++++++----------------------- >>>>>> server/user.h | 3 --- >>>>>> 3 files changed, 39 insertions(+), 35 deletions(-) >>>>>> >>>>> Just a quick thought, as the shared desktop data struct is flagged volatile, this will probably prevent optimizations on the server-side reads too, maybe we could avoid that and only make the writes volatile? >>>> Hmm, interesting idea. That would most likely involve volatile casts >>>> while writing (or macros to hide them), neither of which are >>>> particularly appealing. I'd be tempted to wait to see if this becomes >>>> a real issue before doing this, but I'm open to being persuaded otherwise. >>>> Huw. >>> >>> We could also keep the non-shared state like it is and add a separate shared state that gets updated with the non-shared state, in the helper? >> Again that's possible, but also seems fairly ugly. Not least because a >> future patch will add the desktop keystate to the shared mapping and >> keeping two copies of that in sync seems wrong. >> Huw. > > But if we consider other architectures than X86, at least for wineserver itself, and as it's done in set_user_shared_data_time, I don't think updating the sequence is theoretically enough, and we probably should do the updates using atomic intrinsics anyway. For non-intel archs it using fences before and after the sequence is updated, so the actual data doesn't need protecting. > So maybe doing it with the __atomic intrinsics for every arch is easier and adding volatile casts there too wouldn't be much uglier. Or it could also be custom macros that does either volatile or intrinsics depending on the arch, and they could also be used for set_user_shared_data_time. We need to support older compilers on intel that don't have the atomic ops. > I also noticed that the client side of the shared data isn't flagged volatile, it should probably be, assuming it's always X86 there. The client should always use get_desktop_shared_memory() which does return a volatile ptr, so I don't think there's a need to flag the actual ptr in struct user_thread_info (of course doing so doesn't hurt). Huw.