[PATCH] ntdll: Don't enter uninterrupted section when get_cached_fd hits

[Ken Thomases]

On Apr 5, 2014, at 7:36 PM, Daniel Horn wrote:

> What about using a lighter weight locking primitive such as one that does not require a syscall?

When it's not under contention, the critical section implementation ought to avoid syscalls already.  When it is under contention, it's pretty unavoidable that syscalls will be necessary.  (A critical section can use a spinlock to put that off for a bit if the thread that owns the lock will be done with it very quickly.  That might help in the common case where the fd is found in the cache _if_ the problem is contention over the critical section.  That's a big "if".)

However, I think the real issue is the calls to pthread_sigmask() in server_{enter,leave}_uninterrupted_section().  I think those are necessary to prevent the wineserver from suspending the thread while it holds the critical section and thus deadlocking with other threads.  I don't know if anything can be done about that.

It might be possible to have a safe, lockless version of get_cached_fd() that sometimes produces a false-negative.  Then, if it fails, you'd enter the uninterrupted section and try the reliable version before making the server call.  Devising such a scheme and convincing us that it's safe and reliable is the hard part.  It might be sufficient put a memory barrier before the lookup of the fd in get_cached_fd() and then something like this at the bottom of that block:

    if (interlocked_cmpxchg( &fd_cache[entry][idx].fd, fd + 1, fd + 1) != fd + 1)
        fd = -1;

The idea is to verify that the fd has not been replaced or removed after we fetched it and its properties.  (There's a chance that server_remove_fd_from_cache() is called after we make that check and then the caller of server_remove_fd_from_cache() closes the fd.  And the fd could be reused for something else.  However, as near as I can tell, that's already possible with server_get_unix_fd() as it stands.)

It's very possible that this scheme has a hole that I've missed.  Getting this sort of thing right is notoriously difficult.

-Ken