User handles changes

[Jacek Caban]

Hi Rémi,

On 2/14/22 22:46, Rémi Bernon wrote:
> Hi Jacek,
>
>
> I had a quick look, although I didn't really look into detail. I 
> really like the idea of having shared memory introduced, with a 
> similar mechanism as for the Proton patches, it makes it possible to 
> think about having them upstream too one day.


Thanks for looking at it.


> Then, I think I would rather use the same locking mechanism for both 
> if possible. I like the way Huw did it with SHM read/write blocks, as 
> it maps quite closely to the SERVER_START_REQ / SERVER_END_REQ 
> pattern, making shared memory communication very visible and explicit 
> (although it could maybe be even better by requiring a scope block, as 
> for server requests).


I agree that we will need a mechanism like that for some cases, but it 
doesn't seem like a mechanism we'd want to use for everything. For 
example, handle table already has its own mechanism compatible with 
Windows (the uniq field) and doesn't need any more locking. I believe 
that having an extra lock (global to handle table?) would be redundant 
and complicate the code.


On client side, there are a number of cases where we don't need any 
extra shared memory lock. For example, when we own user lock anyway, we 
don't need any additional locking to access its shared data (at least 
not for most things).


Some locking of user data will be needed for cases where we don't have 
user lock and will need more than one field. I'm not sure yet, but it 
seems likely that we may want to have a per-window lock. In this case, 
we'd need a separated mechanism (like handle validation) to make sure 
that window owning the lock is not destroyed. That would require a bit 
more complicated equivalent of SHARED_READ_BEGIN.


> I also think having atomic_store_ulong spread a bit everywhere is not 
> making the code very readable. It's also not as efficient I believe, 
> as potentially every store needs to lock, whereas the SHM block only 
> requires it on the sequence counter atomic operations.


In many cases, like SetWindowLong, we just need one store, so sequence 
counter adds an extra store. Anyway, I would be surprised if you could 
measure any difference, especially on x86, where it's just a regular store.


> On the same note, and it's also a potential issue with Proton SHM 
> patches, I think we're using and relying on volatile a little bit too 
> much and that may not be ideal. For instance, every shared memory 
> store is done through a volatile qualified struct, and are all 
> strongly ordered together, although the SHM block is already atomic.


Same as above, I don't think it's a problem. (FWIW, in my patches, I 
already do non-"atomic" stores in some places where strong ordering is 
not needed).


> Regarding the session shared memory, I'm a bit worried about the 
> allocator. I think it'd be much simpler using fixed size blocks and a 
> freelist, especially as it looks like we only need to store window 
> shared data there, for the moment at least. If we need very 
> differently sized blocks, then adding pools is also probably simpler.


I didn't include that in the branch, but window data also contains 
window extra bytes.


Thanks,

Jacek