Hello!

On Wed, Dec 11, 2013 at 07:57:32AM -0500, itpp2012 wrote:

[...]

> A) about the added line ngx_accept_mutex_held=0 in ngx_event.c

[...]

> The line above, between 259 and 260, tried to fix this too, but, there, a
> call to ngx_disable_accept_events(...) is missing.
> In fact to be completely correct and not resulting in partially incorrect
> behaviour as you correctly pointed out, such a call must be added too.

This is wrong as well. There is no reason to disable accept
events till we are sure that we wasn't able to re-aquire accept
mutex before going into the kernel to wait for events. It's just
waste of resources.

You are misunderstanding the code, probably becase the
ngx_accept_mutex_held variable has somewhat misleading name. It
is not expected to mean "we have the accept mutex locked", it means
"we had the accept mutex locked on previous iteration, we have to
disable accept events if we won't be able to re-aquire it".

There is no need to fix it, it's not broken.

[...]

> B) About the second proposed patch:

[...]

> Hopefully at this point it should be clear enough that releasing the
> accept_mutex directly with
>
> *ngx_accept_mutex.lock=0;
>
> is as much safe as calling ngx_shmtx_force_unlock(), the choice of which one
> I just deem cosmetic (they're functionally equivalent): my choice then went
> for the more performant, straight variable assignment.

Even considering the code currently there for win32 version, there
is no guarantee that reading *ngx_accept_mutex.lock is atomic.
While usually it is, in theory it can be non-atomic, and the code
will start doing wrong things due to the if() check unexpectedly
succeeding.

> NOt wanting to make this post longer than it is I conclude just mentioning
> that, as of now, the accept mutex is the only mutex living in shared memory
> so unlocking other possibly shared mutexes is as well unrequired.

That's not true. Something like

$ grep -r shmtx_lock src/

will give you an idea where shared memory mutexes are used in
nginx.

While it's tricky to get all of this working on modern Windows
versions with ASLR, the accept mutex is certainly not the only
shared memory mutex used in nginx.

As I already wrote, I don't object adding an unlock here, but I
would like to see the code which is correct and in-line with the
unix version of the code.

--
Maxim Dounin
http://nginx.org/

_______________________________________________
nginx mailing list
nginx@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx