From: "Rémi Bernon" Subject: Re: [PATCH 3/5] server: Move the cursor position and last change time to the shared data. Message-Id: <7e2befcb-8a3d-d5c3-a344-2320339efda4@codeweavers.com> Date: Thu, 19 Nov 2020 15:41:50 +0100 In-Reply-To: <7BD6A7A2-E25D-4F17-A6A3-680676FE6131@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> 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. 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. 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. Cheers, -- Rémi Bernon