Re: [PATCH v13 0/2] MR3504: ntdll: An implementation of SRWLOCK that closer matches Windows'.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/ntdll/sync.c:
> -
>        if (!old.s.owners)
> -
>        struct srwlock_waiter *tmp = NULL;
> -
>        if (last_to_remove != last)
> -
>            tmp = ReadPointerAcquire((void **)&last_to_remove->next);
> -
>        if (tmp != new_head)
>        {
> -
>            /* Not locked exclusive or shared. We can try to grab it. */
> -
>            new.s.owners = -1;
> -
>            --new.s.exclusive_waiters;
> -
>            wait = FALSE;
> -
>            new_head = tmp;
> -
>            last_up_to_date = last_to_remove;
>        }
> -
>        else
> -
>        if (!new_head)
> -
>            break;

last_to_remove should always be the entry right before new_head, right? And if last_to_remove == last then new_head == NULL, and vice versa, right? And if new_head is NULL, then we're removing everything from the list and we don't need to update any head pointers.

So unless I'm missing something, we can simplify this part a bit, to:

```c
if (last_to_remove == last)
    break;
tmp = ReadAcquire(&last_to_remove->next);
if (tmp != new_head)
{
    new_head = tmp;
    last_up_to_date = last_to_remove;
}
```

Somewhat relatedly, does the head pointer actually need to be accurate for every waiter? It's only ever read from the tail.

Even if so, I wonder if we can just simplify the "last_up_to_date" tracking in general. After playing with it a bit I came up with the following code structure; maybe this is a bit easier to follow?

```c
new_head = NULL;
last_up_to_date = last;
for (;;) /* cmpxchg loop */
{
    if (!new_head)
    {
        for (;;)
        {
            /* walk the list, update last_to_remove */
        }
        last_up_to_date = last_to_remove;
    }

while (last_up_to_date != last)
        /* update heads */
}
```