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 _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
