[squid-dev] [PATCH] shared_memory_locking

[Amos Jeffries]

On 11/03/2016 4:01 a.m., Alex Rousskov wrote:
> On 03/10/2016 01:33 AM, Amos Jeffries wrote:
>> On 10/03/2016 11:14 a.m., Alex Rousskov wrote:
>>> Hello,
>>>
>>>     The attached patch adds a "shared_memory_locking" configuration
>>> directive to control mlock(2).
>>>
>>> Locking shared memory at startup avoids SIGBUS crashes when kernel runs
>>> out of RAM during runtime. This has been discussed during the "[RFC] Fix
>>> shared memory initialization, cleanup. Ensure its usability." thread
>>> archived at
>>> http://lists.squid-cache.org/pipermail/squid-dev/2015-December/004112.html
>>>
>>> Why not enable locking by default? Unfortunately, locking requires
>>> privileges and/or much-higher-than-default RLIMIT_MEMLOCK limits. Thus,
>>> requiring locked memory by default is likely to cause too many
>>> complaints, especially since Squid has not required that before. Even
>>> "make check" will not work in most environments (without making our unit
>>> tests even less representative)! Thus, the default is off, at least for now.
>>>
>>> As we gain more experience, we may enable locking by default while
>>> making default locking failures non-fatal and warning about significant
>>> [accumulated] locking delays. The proposed code was written to make
>>> those possible future changes easier, but I am not volunteering to
>>> implement them at this time.
>>>
>>
>> In Ipc::Mem::Segment::lock():
>>
>> * please use #if defined() instead of #ifdef.
> 
> Done.
> 
> 
>> * fatalf() sends a FATAL level error to cache.log. No need to preceed it
>> with a less important debugs ERROR message saying the same thing.
> 
> There is such a need, unfortunately: The FATAL error is printed much
> later than the error is found, making triage a lot more difficult in
> some cases. In the extreme cases, the FATAL error is not printed at all
> (because Squid crashes long before fatal.cc code gets to printing the
> error). Fixing this fatal() problem is outside this patch scope.

I'm talking about these lines:

 ... {
+        const int savedError = errno;
+        debugs(54, DBG_IMPORTANT, "ERROR: shared_memory_locking enabled
but mlock(" << theName << ',' << theSize << ") failed: " <<
xstrerr(savedError));
+        fatalf("shared_memory_locking on but failed to mlock(%s, %"
PRId64 "): %s\n",
+               theName.termedBuf(), theSize, xstrerr(savedError));
+    }

> 
> To partially address your concern, I set debugs() level to 5 and removed
> the ERROR prefix. This still helps with triage when higher debugging
> levels are configured and makes the new code consistent with most other
> fatal() calls in Segment.cc.

If there is a bug in fatalf() that's something else that needs to be
fixed. But there has been no sign that I'm aware of about any such issue
in for any of the hundreds of other calls to it. Please dont make bad
code here just for that.

Amos