unmatched mm_unlock

[Terry Duncan]

I am seeing something very similar to bug 232 occasionally using lighttpd
1.4.11, EA 0.9.5 and PHP 5.2.2 on an ARM processor. The php processes go
nuts. The suggestion there was to try the patch for #224. That patch is for
spin locks and I'm not using spinlocks so .I have debugged it a bit and
discovered that the free list in the shared memory segment is corrupt - I
hacked up a little utility to dump the shared memory free list and it shows
one node on the free list and it points back to itself which is the reason
the php processes are spinning.

I have also noticed that the semaphore value is hosed. Using an IPC
semaphore, the value should always be 1 unless some process has it locked.
Using another little debug utility I am seeing an increasing value there.
So, it follows that if the locking mechanism is not working, it leaves open
the possibility of corrupting the free list.

I have a lot of testing before I can declare victory but I believe the
problem is that there is a call to mm_unlock() in
eacclerator_clean_request() without a matching mm_lock(). Can anyone tell me
why that call is there? Is it going to hose something up if I remove it?

[Bart Vanbrabant]

Hi,

I've gone over that code a thousand time before because I've been
suspecting that something fishy has been going one there. I have no
idea why that mm_unlock is there and I don't know why I haven't looked
at it sooner why it's there. It has been added by the original mmcache
author when he added shared memory support.

I think it's safe to remove it, I'm really short on time to test this.
Can you try it and let us know how it turned out?

gr,

Bart

--
Bart Vanbrabant <bart.va...@zoeloelip.be>

[Terry Duncan]

Thanks Bart. I have tested this and there seem to be no side effects to
removing the unlock call. I have also not seen the spinning php's since
making the change. I'm including a patch which I will also attach to bug
#232 unless there are objections. The change to mm.c is not necessary but it
would prevent something similar from occuring in the future if there were an
unmatched lock or unlock.

--- eaccelerator.c.orig 2007-05-16 12:07:31.000000000 -0700
+++ eaccelerator.c 2007-12-10 13:41:23.000000000 -0800
@@ -1752,7 +1752,6 @@
mm_used_entry *p = (mm_used_entry*)EAG(used_entries);
if (eaccelerator_mm_instance != NULL) {
EACCELERATOR_UNPROTECT();
- mm_unlock(eaccelerator_mm_instance->mm);
if (p != NULL || eaccelerator_mm_instance->locks != NULL) {
EACCELERATOR_LOCK_RW();
while (p != NULL) {
--- mm.c.orig 2006-10-11 05:45:52.000000000 -0700
+++ mm.c 2007-12-07 16:14:44.000000000 -0800
@@ -357,10 +357,18 @@
return 1;
}

+static int locked = 0;
+
static int mm_do_lock(mm_mutex* lock, int kind) {
int rc;
struct sembuf op;

+ if (locked)
+ {
+ ea_debug_log("eAccelerator: attempted double lock: %u\n", getpid());
+ return 1;
+ }
+ locked++;
op.sem_num = 0;
op.sem_op = -1;
op.sem_flg = SEM_UNDO;
@@ -374,6 +382,12 @@
int rc;
struct sembuf op;

+ if (!locked)
+ {
+ ea_debug_log("eAccelerator: attempted double unlock: %u\n", getpid());
+ return 1;
+ }
+ locked--;
op.sem_num = 0;
op.sem_op = 1;

[Bart Vanbrabant]

> Thanks Bart. I have tested this and there seem to be no side effects to
> removing the unlock call. I have also not seen the spinning php's since
> making the change. I'm including a patch which I will also attach to bug
> #232 unless there are objections. The change to mm.c is not necessary but it
> would prevent something similar from occuring in the future if there were an
> unmatched lock or unlock.

Sure, attach it to that ticket so other can test it to. If it solves
the problem I'm inclined to release new version for it.

I'm not to happy about this detection code. Incrementing a global
outside a lock could cause some race condition. If I include it I
would put it inside DEBUG ifdef's so it would be compiled
conditionally.