Windows OpCache bug fix

I want to increase visibility for my PR 1531,
https://github.com/php/php-src/pull/1531, my patch for fixing an
intermittent OpCache issue on Windows.

Details, etc... are on the PR.

Regards
-M

-----Original Message-----
From: Dmitry Stogov [mailto:dmitry@zend.com]
Sent: Monday, October 5, 2015 3:31 PM
To: Anatol Belski anatol.php@belski.net
Cc: Matt Ficken themattficken@gmail.com; Pierre Joye
pierre.php@gmail.com; Laruence laruence@php.net; PHP Internals
internals@lists.php.net; dmitry@php.net
Subject: Re: [PHP-DEV] Re: Windows OpCache bug fix

Subject: Re: [PHP-DEV] Re: Windows OpCache bug fix

Dmitry, I'd have a question to this

Also. if we can't map SHM into desired address space, we may
map it in
any
other address and copy data into the process memory similar
to
file-cache.
In randomized memory layout, even the base were available and
OpenFileMapping were worky, some parts of that memory were
already
taken.
How exactly it could be done, could you please give a couple
of pointers to this?

If MapViewOfFileEx(..., wanted_mapping_base) fails, we do
MapViewOfFileEx(..., NULL).

Would the file cache be always required then?

This is not necessary, but depends on implementation of course.

Thanks for the advice. I was playing with this over these days.
There are two usual cases where it possibly fails when reattaching
ATM

https://github.com/php/php-src/blob/PHP-7.0/ext/opcache/shared_all
oc_w
in32.c#L151

• the saved address is available but is not suitable

https://github.com/php/php-src/blob/PHP-7.0/ext/opcache/shared_all
oc_w
in32.c#L159

• the actual MapViewOfFileEx case

An unrelated plain C test shows, that MapViewOfFileEx can possibly
fail when called second time, too. Even with NULL or with another
address as base. But even if it could map at a different base -
the internal structures will probably have invalid addresses.

right. we might need different code for zend_accle_hash access or
convert corresponding structures to PIC.
For opcdeos "invalid address" don't matter because we will copy them
into process memory and fix (like in file-cache).

Ah, I have to study the file cache code then. But generally it sounds
not like something that can be done offhand. I've also thought about
other things like interned strings (maybe something else), not sure
they're stored with the cache.

So it looks like there's indeed no profit to do any retry once the
actualy base address needed was failed to reattach to.

IMHO the scenario that were usable - in case it fails to reattach
to the exact address, it has to switch to heap. Just for one
request, it should get a heap allocated segment and then
invalidate all the
cache.
That way we fulfill the following

• the request is served for sure
• the scripts are not cached indeed, so no divergence with the
  actual real cache

A heap fallback memory handler would be probably quite easy to
implement.
What do you think?

Apropos with the heap - it also looks like when PHP is used as
module under mpm_winnt, all the cache could use heap instead of
SHM. In that case, there is only one Apache process serving with
many
threads.
Except one would want to share that cache outside Apache, using
heap there could be a much simpler solution.

Heap based cache makes the same problems. It increase the memory
usage
and
doesn't provide cache consistency.
Just fall-back to file-cache may be better.
Do you think something like this would suffice as file cache fallback
https://gist.github.com/weltling/224001a468f04de13693 ? Though it'd
still diverge from the "main" cache.

I think we should enable file-cache automatically, but we can set
ZCG(accel_directives).file_cache_only
if the file_cache already enabled.

I've revoked the approach
https://gist.github.com/weltling/69bd1e47dc15273edde5 , also added
enforcement per request (was missing in the previous version). Or did you
mean "we should NOT enable file cache automatically"? Can be easy changed
ofc. IMHO one can enforce automatically, careful programmers do check error
logs :)

I wouldn't enable file cache automatically, but this really not an
implementation problem.

• if (NULL != ZCG(accel_directives).file_cache) {

•   ZCG(accel_directives).file_cache_only = 1;
  

• }

Actually in such case all the processes should switch to file cache?

No. Only the processes that weren't be able to attach to SHM.

Just not sure how they all would negotiate that when no SHM is
available (probably through files, or a separate shared chunk).

yeah. Processes that use file-cache-only won't be able to negotiate SHM
cache.
:(

ACK, so basically it is the same principle Matt suggested with sidestep
cache. I could imagine synchronizing all the processes through another
shared segment. Not the big one where all the cache is handled, but just a
couple of bytes that wouldn't require to be attached to the same address.
That would allow to signal other file-cache-only processes for cache reset,
etc.

This idea should work. Mapping of small portion into the same address space
shouldn't be required.

Thanks. Dmitry.

Actually, when I implemented file-cache I had a though about
different
storage
back-ends (e.g. sqlite or memcache).
This abstraction might make support for "improperly mapped SHM"
quite
easy.

Yeah, that could be useful. Maybe also module based so one could even
supply them separately, then an arbitrary storage mechanism could be
provided. Fe like I've mentioned for mpm_winnt -if there's no
intention to share the cache outside Apache, just one heap for all
could be much simpler to avoid all that reattach mechanics.

Does mpm_winnt use pure ZTS without processes?

The master process starts only one child (also documented here
http://httpd.apache.org/docs/2.4/mod/mpm_winnt.html). IMHO even If one
would decide supporting virtual hosts (not supported by the current
solution anyway), that were done relatively simple creating private heaps
inside the same child process.

Thanks

Anatol

I have a patch for the IPC mechanism we talked about (to avoid consistency
problems) and to allocate the side-step OpCache on process's private heap.
I've done some quick tests of this on Windows 8.1.

See:
https://github.com/mattficken/php-src/commit/e11b6f010be7d48ed4e29f3a758dffc9acf586fd

I added the ZEND_WIN32_SIDESTEP_TEST macro to shared_alloc_win32.c to force
using a side-step cache instead of the normal reattach procedure, for
testing.

Anatol, this would then fit with your file cache work.

Dmitry, see if my separate of accel_shared_globals and shared_segments
makes sense, ~line 250 of shared_alloc_win32.c For safety, I tried to
limit the view of memfile to only accel_shared_globals.

Regards
-M

Hi Dmitry,

-----Original Message-----
From: Dmitry Stogov [mailto:dmitry@zend.com]
Sent: Tuesday, October 6, 2015 10:01 AM
To: Anatol Belski anatol.php@belski.net
Cc: Matt Ficken themattficken@gmail.com; Pierre Joye
pierre.php@gmail.com; Laruence laruence@php.net; PHP Internals
internals@lists.php.net; dmitry@php.net
Subject: Re: [PHP-DEV] Re: Windows OpCache bug fix

On Mon, Oct 5, 2015 at 6:38 PM, Anatol Belski anatol.php@belski.net
wrote:

-----Original Message-----
From: Dmitry Stogov [mailto:dmitry@zend.com]
Sent: Monday, October 5, 2015 3:31 PM
To: Anatol Belski anatol.php@belski.net
Cc: Matt Ficken themattficken@gmail.com; Pierre Joye
pierre.php@gmail.com; Laruence laruence@php.net; PHP Internals
internals@lists.php.net; dmitry@php.net
Subject: Re: [PHP-DEV] Re: Windows OpCache bug fix

Subject: Re: [PHP-DEV] Re: Windows OpCache bug fix

Dmitry, I'd have a question to this

Also. if we can't map SHM into desired address space, we
may map it in
any
other address and copy data into the process memory
similar to
file-cache.
In randomized memory layout, even the base were available
and OpenFileMapping were worky, some parts of that memory
were already
taken.
How exactly it could be done, could you please give a
couple of pointers to this?

If MapViewOfFileEx(..., wanted_mapping_base) fails, we do
MapViewOfFileEx(..., NULL).

Would the file cache be always required then?

This is not necessary, but depends on implementation of
course.

Thanks for the advice. I was playing with this over these days.
There are two usual cases where it possibly fails when
reattaching ATM

https://github.com/php/php-src/blob/PHP-7.0/ext/opcache/shared
_all
oc_w
in32.c#L151

• the saved address is available but is not suitable

https://github.com/php/php-src/blob/PHP-7.0/ext/opcache/shared
_all
oc_w
in32.c#L159

• the actual MapViewOfFileEx case

An unrelated plain C test shows, that MapViewOfFileEx can
possibly fail when called second time, too. Even with NULL or
with another address as base. But even if it could map at a
different base - the internal structures will probably have
invalid
addresses.

right. we might need different code for zend_accle_hash access
or convert corresponding structures to PIC.
For opcdeos "invalid address" don't matter because we will copy
them into process memory and fix (like in file-cache).

Ah, I have to study the file cache code then. But generally it
sounds not like something that can be done offhand. I've also
thought about other things like interned strings (maybe something
else), not sure they're stored with the cache.

So it looks like there's indeed no profit to do any retry once
the actualy base address needed was failed to reattach to.

IMHO the scenario that were usable - in case it fails to
reattach to the exact address, it has to switch to heap. Just
for one request, it should get a heap allocated segment and
then
invalidate all the
cache.
That way we fulfill the following

• the request is served for sure
• the scripts are not cached indeed, so no divergence with the
  actual real cache

A heap fallback memory handler would be probably quite easy to
implement.
What do you think?

Apropos with the heap - it also looks like when PHP is used as
module under mpm_winnt, all the cache could use heap instead
of SHM. In that case, there is only one Apache process serving
with
many
threads.
Except one would want to share that cache outside Apache,
using heap there could be a much simpler solution.

Heap based cache makes the same problems. It increase the memory
usage
and
doesn't provide cache consistency.
Just fall-back to file-cache may be better.
Do you think something like this would suffice as file cache
fallback
https://gist.github.com/weltling/224001a468f04de13693 ? Though
it'd still diverge from the "main" cache.

I think we should enable file-cache automatically, but we can set
ZCG(accel_directives).file_cache_only
if the file_cache already enabled.

I've revoked the approach
https://gist.github.com/weltling/69bd1e47dc15273edde5 , also added
enforcement per request (was missing in the previous version). Or did
you mean "we should NOT enable file cache automatically"? Can be easy
changed ofc. IMHO one can enforce automatically, careful programmers
do check error logs :)

I wouldn't enable file cache automatically, but this really not an
implementation
problem.

• if (NULL != ZCG(accel_directives).file_cache) {
  

•         ZCG(accel_directives).file_cache_only = 1;
  

• }
  

Actually in such case all the processes should switch to file
cache?

No. Only the processes that weren't be able to attach to SHM.

Just not sure how they all would negotiate that when no SHM is
available (probably through files, or a separate shared chunk).

yeah. Processes that use file-cache-only won't be able to negotiate
SHM
cache.
:(

ACK, so basically it is the same principle Matt suggested with
sidestep cache. I could imagine synchronizing all the processes
through another shared segment. Not the big one where all the cache is
handled, but just a couple of bytes that wouldn't require to be
attached to the
same address.
That would allow to signal other file-cache-only processes for cache
reset, etc.

This idea should work. Mapping of small portion into the same address
space
shouldn't be required.

Thanks for the comments. I came up with this patch now
https://gist.github.com/weltling/a2aea8eae1013425e15a . The principle as
discussed - if opcache.file_cache is enabled, it does fall back to it when
failed to reattach. Also while developing it turned out that cache
invalidation doesn't need to be handled. Only the cache_reset() case is
what needs some IPC negotiation, thus it is done.

It seems to be some more complicated than I've thought at the start.
Needs more stress testing anyway, but so far tests was telling it works.
It'll still throw warnings into the error log when reattach fails, but then
serve with file_cache_only. Then, if some process will set restart_pending,
actually only one of the file-cache-only processes needs to react, then
cache is invalidated for all file-cache-only processes.

BTW while developing I came to another observation. Actually, if any fatal
happens, but we throw only zend_accel_error(ACCEL_LOG_WARNING, ...); - that
would allow to just set ZCG(enabled) = 0; and let the script run without
being cached. Probably not quite nice, but the most simple solution :)

Regards

Anatol

Yes, earlier I was advocating a side-step SHM area for the side-step
OpCache because it'll probably use less memory. Sounded like I lost that
argument, so I switched to in-process heap (which is more complicated too).

New commit using side-step SHM(not heap):
https://github.com/php/php-src/commit/92e568270957a63c8b0d1545d9dc0495a851b5c0

Hi Matt,

-----Original Message-----
From: Matt Ficken [mailto:themattficken@gmail.com]
Sent: Wednesday, October 7, 2015 12:18 PM
To: Anatol Belski anatol.php@belski.net
Cc: Dmitry Stogov dmitry@zend.com; Pierre Joye pierre.php@gmail.com;
Laruence laruence@php.net; PHP Internals internals@lists.php.net;
dmitry@php.net
Subject: Re: [PHP-DEV] Re: Windows OpCache bug fix

I have a patch for the IPC mechanism we talked about (to avoid
consistency
problems) and to allocate the side-step OpCache on process's private
heap.
I've done some quick tests of this on Windows 8.1.

See:
https://github.com/mattficken/php-
src/commit/e11b6f010be7d48ed4e29f3a758dffc9acf586fd

I added the ZEND_WIN32_SIDESTEP_TEST macro to shared_alloc_win32.c to
force using a side-step cache instead of the normal reattach procedure,
for
testing.

Anatol, this would then fit with your file cache work.

Dmitry, see if my separate of accel_shared_globals and shared_segments
makes
sense, ~line 250 of shared_alloc_win32.c For safety, I tried to limit
the view of
memfile to only accel_shared_globals.

I guess you mean this diff
https://github.com/php/php-src/compare/master...mattficken:master
(otherwise there are quite some remains from your older patches).

Yeah, some similar idea. I had a similar exercise, but then gave up on it
as discussed with Dmitry. There are at least two issues to falling back to
heap - MapViewOfFile() can fail as well. Then we're in the same situation.
MapViewOfFile() can specifically fail if the requested size is big, as
especially on 32-bit system has to find a contiguous chunk of memory.

But that is also something that can enormously increase memory usage. Say
a user would assign 1Gb to opcache, then every process using sidestep heap
would have to allocate 1Gb ... here we're pretty much at the boundary
again. But even we would do it - it should be the system heap (HeapAlloc,
VirtualAlloc or even malloc). Using ZMM is a wrong strategy at this place.

For these reasons I was suggesting using a separate shared memory of a
couple of bytes - that is unlikely to fail to reattach. Maybe it could be
even expedient to separate the actual SHM from the adminitstartive items
(like counters, etc) in the future. Doing so would give more flexibility in
such situation where the actual cache is unavailable (like failed to
reattach).

For mpm_winnt or similar SAPIs using heap only could be expedient.
Something for the future it could be. Right now all the cache is global,
but forcing it to be heap only would prevent users to share it outside
Apcahe. Even though, less bugs of such art would be expected.

Regards

Anatol

few notes:

• this is for Windows only. Right?
• are you sure SysV IPC API is available everywhere? I mean shmget()
  in file_cache_fallback_init().
  (I didn't know about Windows).
• I think all the changes should be wrapped by "#ifdef ...FALLBACK_SUPPORT"
  (at least we will able to easily remove this)
• it's better to add ini directive to enable/disable this callback
  triggering to minimize risks for users.

I would try to implement this in a simpler way.
I'll try to find time

Thanks. Dmitry.

Hi Dmitry,

-----Original Message-----
From: Dmitry Stogov [mailto:dmitry@zend.com]
Sent: Tuesday, October 6, 2015 10:01 AM
To: Anatol Belski anatol.php@belski.net
Cc: Matt Ficken themattficken@gmail.com; Pierre Joye
pierre.php@gmail.com; Laruence laruence@php.net; PHP Internals
internals@lists.php.net; dmitry@php.net
Subject: Re: [PHP-DEV] Re: Windows OpCache bug fix

On Mon, Oct 5, 2015 at 6:38 PM, Anatol Belski anatol.php@belski.net
wrote:

-----Original Message-----
From: Dmitry Stogov [mailto:dmitry@zend.com]
Sent: Monday, October 5, 2015 3:31 PM
To: Anatol Belski anatol.php@belski.net
Cc: Matt Ficken themattficken@gmail.com; Pierre Joye
pierre.php@gmail.com; Laruence laruence@php.net; PHP Internals
internals@lists.php.net; dmitry@php.net
Subject: Re: [PHP-DEV] Re: Windows OpCache bug fix

Subject: Re: [PHP-DEV] Re: Windows OpCache bug fix

Dmitry, I'd have a question to this

Also. if we can't map SHM into desired address space, we
may map it in
any
other address and copy data into the process memory
similar to
file-cache.
In randomized memory layout, even the base were available
and OpenFileMapping were worky, some parts of that memory
were already
taken.
How exactly it could be done, could you please give a
couple of pointers to this?

If MapViewOfFileEx(..., wanted_mapping_base) fails, we do
MapViewOfFileEx(..., NULL).

Would the file cache be always required then?

This is not necessary, but depends on implementation of
course.

Thanks for the advice. I was playing with this over these days.
There are two usual cases where it possibly fails when
reattaching ATM

https://github.com/php/php-src/blob/PHP-7.0/ext/opcache/shared
_all
oc_w
in32.c#L151

• the saved address is available but is not suitable

https://github.com/php/php-src/blob/PHP-7.0/ext/opcache/shared
_all
oc_w
in32.c#L159

• the actual MapViewOfFileEx case

An unrelated plain C test shows, that MapViewOfFileEx can
possibly fail when called second time, too. Even with NULL or
with another address as base. But even if it could map at a
different base - the internal structures will probably have
invalid
addresses.

right. we might need different code for zend_accle_hash access
or convert corresponding structures to PIC.
For opcdeos "invalid address" don't matter because we will copy
them into process memory and fix (like in file-cache).

Ah, I have to study the file cache code then. But generally it
sounds not like something that can be done offhand. I've also
thought about other things like interned strings (maybe something
else), not sure they're stored with the cache.

So it looks like there's indeed no profit to do any retry once
the actualy base address needed was failed to reattach to.

IMHO the scenario that were usable - in case it fails to
reattach to the exact address, it has to switch to heap. Just
for one request, it should get a heap allocated segment and
then
invalidate all the
cache.
That way we fulfill the following

• the request is served for sure
• the scripts are not cached indeed, so no divergence with the
  actual real cache

A heap fallback memory handler would be probably quite easy to
implement.
What do you think?

Apropos with the heap - it also looks like when PHP is used as
module under mpm_winnt, all the cache could use heap instead
of SHM. In that case, there is only one Apache process serving
with
many
threads.
Except one would want to share that cache outside Apache,
using heap there could be a much simpler solution.

Heap based cache makes the same problems. It increase the memory
usage
and
doesn't provide cache consistency.
Just fall-back to file-cache may be better.
Do you think something like this would suffice as file cache
fallback
https://gist.github.com/weltling/224001a468f04de13693 ? Though
it'd still diverge from the "main" cache.

I think we should enable file-cache automatically, but we can set
ZCG(accel_directives).file_cache_only
if the file_cache already enabled.

I've revoked the approach
https://gist.github.com/weltling/69bd1e47dc15273edde5 , also added
enforcement per request (was missing in the previous version). Or did
you mean "we should NOT enable file cache automatically"? Can be easy
changed ofc. IMHO one can enforce automatically, careful programmers
do check error logs :)

I wouldn't enable file cache automatically, but this really not an
implementation
problem.

• if (NULL != ZCG(accel_directives).file_cache) {
  

•         ZCG(accel_directives).file_cache_only = 1;
  

• }
  

Actually in such case all the processes should switch to file
cache?

No. Only the processes that weren't be able to attach to SHM.

Just not sure how they all would negotiate that when no SHM is
available (probably through files, or a separate shared chunk).

yeah. Processes that use file-cache-only won't be able to negotiate
SHM
cache.
:(

ACK, so basically it is the same principle Matt suggested with
sidestep cache. I could imagine synchronizing all the processes
through another shared segment. Not the big one where all the cache is
handled, but just a couple of bytes that wouldn't require to be
attached to the
same address.
That would allow to signal other file-cache-only processes for cache
reset, etc.

This idea should work. Mapping of small portion into the same address
space
shouldn't be required.

Thanks. Dmitry.

Actually, when I implemented file-cache I had a though about
different
storage
back-ends (e.g. sqlite or memcache).
This abstraction might make support for "improperly mapped SHM"
quite
easy.

Yeah, that could be useful. Maybe also module based so one could
even supply them separately, then an arbitrary storage mechanism
could be provided. Fe like I've mentioned for mpm_winnt -if
there's no intention to share the cache outside Apache, just one
heap for all could be much simpler to avoid all that reattach
mechanics.

Does mpm_winnt use pure ZTS without processes?

The master process starts only one child (also documented here
http://httpd.apache.org/docs/2.4/mod/mpm_winnt.html). IMHO even If one
would decide supporting virtual hosts (not supported by the current
solution anyway), that were done relatively simple creating private
heaps inside the same child process.

Updated the patch with one small fix
https://gist.github.com/weltling/925c0a774fa1261ede58 . I think it's
ready to be applied, could you please check? One could shrink it to Windows
yet, or leave as is so the fallback is worky crossplatform.

Thanks

anatol

Hi Matt,

-----Original Message-----
From: Matt Ficken [mailto:themattficken@gmail.com]
Sent: Wednesday, October 7, 2015 12:18 PM
To: Anatol Belski anatol.php@belski.net
Cc: Dmitry Stogov dmitry@zend.com; Pierre Joye pierre.php@gmail.com;
Laruence laruence@php.net; PHP Internals internals@lists.php.net;
dmitry@php.net
Subject: Re: [PHP-DEV] Re: Windows OpCache bug fix

I have a patch for the IPC mechanism we talked about (to avoid
consistency
problems) and to allocate the side-step OpCache on process's private
heap.
I've done some quick tests of this on Windows 8.1.

See:
https://github.com/mattficken/php-
src/commit/e11b6f010be7d48ed4e29f3a758dffc9acf586fd

I added the ZEND_WIN32_SIDESTEP_TEST macro to shared_alloc_win32.c to
force using a side-step cache instead of the normal reattach procedure,
for
testing.

Anatol, this would then fit with your file cache work.

Dmitry, see if my separate of accel_shared_globals and shared_segments
makes
sense, ~line 250 of shared_alloc_win32.c For safety, I tried to limit
the view of
memfile to only accel_shared_globals.

I guess you mean this diff
https://github.com/php/php-src/compare/master...mattficken:master
(otherwise there are quite some remains from your older patches).

Yeah, some similar idea. I had a similar exercise, but then gave up on it
as discussed with Dmitry. There are at least two issues to falling back to
heap - MapViewOfFile() can fail as well. Then we're in the same situation.
MapViewOfFile() can specifically fail if the requested size is big, as
especially on 32-bit system has to find a contiguous chunk of memory.

I hope this is a theoretical problem. Have you ever seen this?

But that is also something that can enormously increase memory usage. Say
a user would assign 1Gb to opcache, then every process using sidestep heap
would have to allocate 1Gb ... here we're pretty much at the boundary
again. But even we would do it - it should be the system heap (HeapAlloc,
VirtualAlloc or even malloc). Using ZMM is a wrong strategy at this place.

For these reasons I was suggesting using a separate shared memory of a
couple of bytes - that is unlikely to fail to reattach. Maybe it could be
even expedient to separate the actual SHM from the adminitstartive items
(like counters, etc) in the future. Doing so would give more flexibility in
such situation where the actual cache is unavailable (like failed to
reattach).

Instead of allocating additional SHM at run-time, we may remap only regular
SHM header with opcache shared globals (when the whole SHM can't be map).

Thanks. Dmitry.

For mpm_winnt or similar SAPIs using heap only could be expedient.
Something for the future it could be. Right now all the cache is global,
but forcing it to be heap only would prevent users to share it outside
Apcahe. Even though, less bugs of such art would be expected.

Regards

Anatol