[squid-dev] [RFC] Fix shared memory initialization, cleanup. Ensure its usability.

[Amos Jeffries]

On 10/12/2015 7:42 a.m., Alex Rousskov wrote:
> Hello,
> 
>     The attached patch does four things:
> 
> 1. Fixes shared memory initialization. The OS X portability fix (Bug
> 3805) is a gift that keeps on giving: After we fixed OS X compilation,
> we stopped initializing previously used shared memory after crashes!
> Search the patch for "truncate".
> 
> 2. Fixes shared memory cleanup. One segment was not deleted when running
> Squid in non-daemon mode (-N). Search the patch for "UsingSmp".
> 
> 3. Makes sure using shared memory does not lead to SIGBUS crashes.
> Search the patch for "mlock".
> 
> 4. Adds tests to check whether shared memory is usable. Search the patch
> for "memory checks" and "shouldTest".
> 
> 
> My plan is:
> 
> * Commit #1 and #2 fixes to trunk without review unless somebody
> requests one now: IMO, they are simple and non-controversial.
> 

+1 for #1 bits. ... without the new "HERE" macros.



> * Remove/forget #4. These paranoid checks should not be needed after #3,
> which they have helped to test. We can always add them later if I am wrong.
> 

Maybe make these optional. Or #if 0 them out for now.

At the very least the checks are broken on 32-bit machinery:

* Segment theSize is a off_t (64-bit unsigned).
- fillTest() at least is storing its value into an 'int' (32-bit signed)
for the page count math.
 - maybe causeing 32-bit systems with large-file support to have wrap
issues with 2GB+ signed/unsigned values.
- the math needs to be done directly in off_t or with explicit uint64_t.
- where conversion to int is required (for memset/memcpy?) it also needs
to do limit checking that the 64-bit value is (n < 2^31) before assignment.

If you choose to drop the *Test() code entirely this is not an issue. If
you #if 0 the above should probably be fixed or mentioned as a XXX fix.



> * Discuss mlock() changes in #3 (below). Post the corresponding change
> for the proper review. The code adding the mlock(2) call itself is
> simple, but the side effects of this change are serious enough to
> warrant proper review IMO.
> 
> 
> Questions: Should we add a configuration directive to control whether
> mlock(2) is called? If yes, should mlock(2) be called by default?
> Should mlock(2) failures be fatal?
> 
> 
> My answers are three YESes: I propose to call mlock(2) by default (if
> available) to guarantee that mmapped memory is usable. I also propose to
> make mlock(2) failures fatal. I hesitate offering a configuration option
> to control this behavior, but I think we should offer it because mlock()
> causes startup delays. I will discuss the problem and explain my
> rationale below.


> AFAICT, this large delay is the only valid argument for making mlock()
> calls optional -- if I am sure that Squid has enough RAM, then I might
> want to trade one large startup delay for numerous tiny delays as the
> mmapped memory gets allocated and used.
> 
> Needless to say, if we make mlock() calls optional, then some admins
> will disable them without giving Squid enough RAM and will then complain
> about mysterious crashes. On the other hand, all other factors being
> equal, we should provide tools for knowledgeable admins even if those
> tools may be misused by others.
> 
> How would you answer the three questions above?


My answers are no, yes, yes. Since this is a startup thing squid.conf
post-processing is probably too late.

In general if a setting is not something that can have a reasonable
temporary default for a while, then be switched around in realtime. Then
squid.conf is not a good place for it.

I would make it mandatory on "-k check" (but not -k parse). Optional
through a command line parameter on all other runs. (Remember we have
long-args possible now so no need to squeeze it into a gap.)


Amos