Skip to content

Commit 6e73f57

Browse files
nikicnikic
committed
Avoid recursive locking in apcu_entry()
Don't try to acquire any recursive cache locks if the current thread is already inside apcu_entry(). This means that it is now safe to call other apcu_* functions inside apcu_entry().
1 parent 61f3c16 commit 6e73f57

8 files changed

+101
-66
lines changed

‎apc_cache.c

+29-49
Original file line numberDiff line numberDiff line change
@@ -504,15 +504,15 @@ PHP_APCU_API zend_bool apc_cache_store(
504504
}
505505

506506
/* execute an insertion */
507-
if (!APC_WLOCK(cache->header)) {
507+
if (!apc_cache_wlock(cache)) {
508508
free_entry(cache, entry);
509509
return 0;
510510
}
511511

512512
php_apc_try {
513513
ret = apc_cache_wlocked_insert(cache, entry, exclusive);
514514
} php_apc_finally {
515-
APC_WUNLOCK(cache->header);
515+
apc_cache_wunlock(cache);
516516
} php_apc_end_try();
517517

518518
if (!ret) {
@@ -701,8 +701,7 @@ PHP_APCU_API void apc_cache_clear(apc_cache_t* cache)
701701
return;
702702
}
703703

704-
/* lock header */
705-
if (!APC_WLOCK(cache->header)) {
704+
if (!apc_cache_wlock(cache)) {
706705
return;
707706
}
708707

@@ -713,8 +712,7 @@ PHP_APCU_API void apc_cache_clear(apc_cache_t* cache)
713712
cache->header->stime = apc_time();
714713
cache->header->nexpunges = 0;
715714

716-
/* unlock header */
717-
APC_WUNLOCK(cache->header);
715+
apc_cache_wunlock(cache);
718716
}
719717
/* }}} */
720718

@@ -734,7 +732,7 @@ PHP_APCU_API void apc_cache_default_expunge(apc_cache_t* cache, size_t size)
734732
t = apc_time();
735733

736734
/* get the lock for header */
737-
if (!APC_WLOCK(cache->header)) {
735+
if (!apc_cache_wlock(cache)) {
738736
return;
739737
}
740738

@@ -783,8 +781,7 @@ PHP_APCU_API void apc_cache_default_expunge(apc_cache_t* cache, size_t size)
783781
}
784782
}
785783

786-
/* unlock header */
787-
APC_WUNLOCK(cache->header);
784+
apc_cache_wunlock(cache);
788785
}
789786
/* }}} */
790787

@@ -797,12 +794,12 @@ PHP_APCU_API apc_cache_entry_t *apc_cache_find(apc_cache_t* cache, zend_string *
797794
return NULL;
798795
}
799796

800-
if (!APC_RLOCK(cache->header)) {
797+
if (!apc_cache_rlock(cache)) {
801798
return NULL;
802799
}
803800

804801
entry = apc_cache_rlocked_find_incref(cache, key, t);
805-
APC_RUNLOCK(cache->header);
802+
apc_cache_runlock(cache);
806803

807804
return entry;
808805
}
@@ -818,12 +815,12 @@ PHP_APCU_API zend_bool apc_cache_fetch(apc_cache_t* cache, zend_string *key, tim
818815
return 0;
819816
}
820817

821-
if (!APC_RLOCK(cache->header)) {
818+
if (!apc_cache_rlock(cache)) {
822819
return 0;
823820
}
824821

825822
entry = apc_cache_rlocked_find_incref(cache, key, t);
826-
APC_RUNLOCK(cache->header);
823+
apc_cache_runlock(cache);
827824

828825
if (!entry) {
829826
return 0;
@@ -847,12 +844,12 @@ PHP_APCU_API zend_bool apc_cache_exists(apc_cache_t* cache, zend_string *key, ti
847844
return 0;
848845
}
849846

850-
if (!APC_RLOCK(cache->header)) {
847+
if (!apc_cache_rlock(cache)) {
851848
return 0;
852849
}
853850

854851
entry = apc_cache_rlocked_find_nostat(cache, key, t);
855-
APC_RUNLOCK(cache->header);
852+
apc_cache_runlock(cache);
856853

857854
return entry != NULL;
858855
}
@@ -872,7 +869,7 @@ PHP_APCU_API zend_bool apc_cache_update(
872869
}
873870

874871
retry_update:
875-
if (!APC_WLOCK(cache->header)) {
872+
if (!apc_cache_wlock(cache)) {
876873
return 0;
877874
}
878875

@@ -884,11 +881,11 @@ PHP_APCU_API zend_bool apc_cache_update(
884881
entry->mtime = t;
885882
}
886883

887-
APC_WUNLOCK(cache->header);
884+
apc_cache_wunlock(cache);
888885
return retval;
889886
}
890887

891-
APC_WUNLOCK(cache->header);
888+
apc_cache_wunlock(cache);
892889
if (insert_if_not_found) {
893890
/* Failed to find matching entry. Add key with value 0 and run the updater again. */
894891
zval val;
@@ -922,7 +919,7 @@ PHP_APCU_API zend_bool apc_cache_atomic_update_long(
922919
}
923920

924921
retry_update:
925-
if (!APC_RLOCK(cache->header)) {
922+
if (!apc_cache_rlock(cache)) {
926923
return 0;
927924
}
928925

@@ -934,11 +931,11 @@ PHP_APCU_API zend_bool apc_cache_atomic_update_long(
934931
entry->mtime = t;
935932
}
936933

937-
APC_RUNLOCK(cache->header);
934+
apc_cache_runlock(cache);
938935
return retval;
939936
}
940937

941-
APC_RUNLOCK(cache->header);
938+
apc_cache_runlock(cache);
942939
if (insert_if_not_found) {
943940
/* Failed to find matching entry. Add key with value 0 and run the updater again. */
944941
zval val;
@@ -971,8 +968,7 @@ PHP_APCU_API zend_bool apc_cache_delete(apc_cache_t *cache, zend_string *key)
971968
/* calculate hash and slot */
972969
apc_cache_hash_slot(cache, key, &h, &s);
973970

974-
/* lock cache */
975-
if (!APC_WLOCK(cache->header)) {
971+
if (!apc_cache_wlock(cache)) {
976972
return 0;
977973
}
978974

@@ -985,16 +981,14 @@ PHP_APCU_API zend_bool apc_cache_delete(apc_cache_t *cache, zend_string *key)
985981
/* executing removal */
986982
apc_cache_wlocked_remove_entry(cache, entry);
987983

988-
/* unlock header */
989-
APC_WUNLOCK(cache->header);
984+
apc_cache_wunlock(cache);
990985
return 1;
991986
}
992987

993988
entry = &(*entry)->next;
994989
}
995990

996-
/* unlock header */
997-
APC_WUNLOCK(cache->header);
991+
apc_cache_wunlock(cache);
998992
return 0;
999993
}
1000994
/* }}} */
@@ -1074,7 +1068,7 @@ PHP_APCU_API zend_bool apc_cache_info(zval *info, apc_cache_t *cache, zend_bool
10741068
return 0;
10751069
}
10761070

1077-
if (!APC_RLOCK(cache->header)) {
1071+
if (!apc_cache_rlock(cache)) {
10781072
return 0;
10791073
}
10801074

@@ -1127,7 +1121,7 @@ PHP_APCU_API zend_bool apc_cache_info(zval *info, apc_cache_t *cache, zend_bool
11271121
add_assoc_zval(info, "slot_distribution", &slots);
11281122
}
11291123
} php_apc_finally {
1130-
APC_RUNLOCK(cache->header);
1124+
apc_cache_runlock(cache);
11311125
} php_apc_end_try();
11321126

11331127
return 1;
@@ -1148,7 +1142,7 @@ PHP_APCU_API void apc_cache_stat(apc_cache_t *cache, zend_string *key, zval *sta
11481142
/* calculate hash and slot */
11491143
apc_cache_hash_slot(cache, key, &h, &s);
11501144

1151-
if (!APC_RLOCK(cache->header)) {
1145+
if (!apc_cache_rlock(cache)) {
11521146
return;
11531147
}
11541148

@@ -1174,7 +1168,7 @@ PHP_APCU_API void apc_cache_stat(apc_cache_t *cache, zend_string *key, zval *sta
11741168
entry = entry->next;
11751169
}
11761170
} php_apc_finally {
1177-
APC_RUNLOCK(cache->header);
1171+
apc_cache_runlock(cache);
11781172
} php_apc_end_try();
11791173
}
11801174

@@ -1234,19 +1228,11 @@ PHP_APCU_API void apc_cache_entry(apc_cache_t *cache, zend_string *key, zend_fca
12341228
return;
12351229
}
12361230

1237-
#ifndef APC_LOCK_RECURSIVE
1238-
if (APCG(recursion)++ == 0) {
1239-
if (!APC_WLOCK(cache->header)) {
1240-
APCG(recursion)--;
1241-
return;
1242-
}
1243-
}
1244-
#else
1245-
if (!APC_WLOCK(cache->header)) {
1231+
if (!apc_cache_wlock(cache)) {
12461232
return;
12471233
}
1248-
#endif
12491234

1235+
APCG(entry_level)++;
12501236
php_apc_try {
12511237
entry = apc_cache_rlocked_find_incref(cache, key, now);
12521238
if (!entry) {
@@ -1271,14 +1257,8 @@ PHP_APCU_API void apc_cache_entry(apc_cache_t *cache, zend_string *key, zend_fca
12711257
apc_cache_entry_release(cache, entry);
12721258
}
12731259
} php_apc_finally {
1274-
#ifndef APC_LOCK_RECURSIVE
1275-
if (--APCG(recursion) == 0) {
1276-
APC_WUNLOCK(cache->header);
1277-
}
1278-
#else
1279-
APC_WUNLOCK(cache->header);
1280-
#endif
1281-
1260+
APCG(entry_level)--;
1261+
apc_cache_wunlock(cache);
12821262
} php_apc_end_try();
12831263
}
12841264
/*}}}*/

‎apc_cache.h

+38
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include "apc.h"
3333
#include "apc_sma.h"
3434
#include "apc_lock.h"
35+
#include "apc_globals.h"
3536
#include "TSRM.h"
3637

3738
typedef struct apc_cache_slam_key_t apc_cache_slam_key_t;
@@ -276,6 +277,43 @@ PHP_APCU_API void apc_cache_default_expunge(apc_cache_t* cache, size_t size);
276277
*/
277278
PHP_APCU_API void apc_cache_entry(apc_cache_t *cache, zend_string *key, zend_fcall_info *fci, zend_fcall_info_cache *fcc, zend_long ttl, zend_long now, zval *return_value);
278279

280+
/* apcu_entry() holds a write lock on the cache while executing user code.
281+
* That code may call other apcu_* functions, which also try to acquire a
282+
* read or write lock, which would deadlock. As such, don't try to acquire a
283+
* lock if the current thread is inside apcu_entry().
284+
*
285+
* Whether the current thread is inside apcu_entry() is tracked by APCG(entry_level).
286+
* This breaks the self-contained apc_cache_t abstraction, but is currently
287+
* necessary because the entry_level needs to be tracked per-thread, while
288+
* apc_cache_t is a per-process structure.
289+
*/
290+
291+
static inline zend_bool apc_cache_wlock(apc_cache_t *cache) {
292+
if (!APCG(entry_level)) {
293+
return WLOCK(&cache->header->lock);
294+
}
295+
return 1;
296+
}
297+
298+
static inline void apc_cache_wunlock(apc_cache_t *cache) {
299+
if (!APCG(entry_level)) {
300+
WUNLOCK(&cache->header->lock);
301+
}
302+
}
303+
304+
static inline zend_bool apc_cache_rlock(apc_cache_t *cache) {
305+
if (!APCG(entry_level)) {
306+
return RLOCK(&cache->header->lock);
307+
}
308+
return 1;
309+
}
310+
311+
static inline void apc_cache_runlock(apc_cache_t *cache) {
312+
if (!APCG(entry_level)) {
313+
RUNLOCK(&cache->header->lock);
314+
}
315+
}
316+
279317
#endif
280318

281319
/*

‎apc_globals.h

+4-4
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,6 @@
3333
#define APC_GLOBALS_H
3434

3535
#include "apc.h"
36-
#include "apc_cache.h"
37-
#include "apc_stack.h"
3836

3937
ZEND_BEGIN_MODULE_GLOBALS(apcu)
4038
/* configuration parameters */
@@ -62,7 +60,8 @@ ZEND_BEGIN_MODULE_GLOBALS(apcu)
6260

6361
char *serializer_name; /* the serializer config option */
6462

65-
volatile unsigned recursion;
63+
/* Nesting level of apcu_entry calls. */
64+
unsigned int entry_level;
6665
ZEND_END_MODULE_GLOBALS(apcu)
6766

6867
/* (the following is defined in php_apc.c) */
@@ -74,7 +73,8 @@ ZEND_EXTERN_MODULE_GLOBALS(apcu)
7473
# define APCG(v) (apcu_globals.v)
7574
#endif
7675

77-
extern apc_cache_t* apc_user_cache;
76+
extern struct _apc_cache_t* apc_user_cache;
77+
7878
#endif
7979

8080
/*

‎apc_iterator.c

+6-6
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ static int apc_iterator_fetch_active(apc_iterator_t *iterator) {
218218
apc_iterator_item_dtor(apc_stack_pop(iterator->stack));
219219
}
220220

221-
if (!APC_RLOCK(apc_user_cache->header)) {
221+
if (!apc_cache_rlock(apc_user_cache)) {
222222
return count;
223223
}
224224

@@ -241,7 +241,7 @@ static int apc_iterator_fetch_active(apc_iterator_t *iterator) {
241241
}
242242
} php_apc_finally {
243243
iterator->stack_idx = 0;
244-
APC_RUNLOCK(apc_user_cache->header)
244+
apc_cache_runlock(apc_user_cache);
245245
} php_apc_end_try();
246246

247247
return count;
@@ -253,7 +253,7 @@ static int apc_iterator_fetch_deleted(apc_iterator_t *iterator) {
253253
int count = 0;
254254
apc_iterator_item_t *item;
255255

256-
if (!APC_RLOCK(apc_user_cache->header)) {
256+
if (!apc_cache_rlock(apc_user_cache)) {
257257
return count;
258258
}
259259

@@ -277,7 +277,7 @@ static int apc_iterator_fetch_deleted(apc_iterator_t *iterator) {
277277
} php_apc_finally {
278278
iterator->slot_idx += count;
279279
iterator->stack_idx = 0;
280-
APC_RUNLOCK(apc_user_cache->header);
280+
apc_cache_runlock(apc_user_cache);
281281
} php_apc_end_try();
282282

283283
return count;
@@ -289,7 +289,7 @@ static void apc_iterator_totals(apc_iterator_t *iterator) {
289289
time_t t = apc_time();
290290
int i;
291291

292-
if (!APC_RLOCK(apc_user_cache->header)) {
292+
if (!apc_cache_rlock(apc_user_cache)) {
293293
return;
294294
}
295295

@@ -309,7 +309,7 @@ static void apc_iterator_totals(apc_iterator_t *iterator) {
309309
}
310310
} php_apc_finally {
311311
iterator->totals_flag = 1;
312-
APC_RUNLOCK(apc_user_cache->header);
312+
apc_cache_runlock(apc_user_cache);
313313
} php_apc_end_try();
314314
}
315315
/* }}} */

‎apc_lock.h

-6
Original file line numberDiff line numberDiff line change
@@ -93,12 +93,6 @@ PHP_APCU_API void apc_lock_destroy(apc_lock_t *lock); /* }}} */
9393
#define RUNLOCK(lock) { apc_lock_runlock(lock); HANDLE_UNBLOCK_INTERRUPTIONS(); }
9494
/* }}} */
9595

96-
/* {{{ object locking macros */
97-
#define APC_WLOCK(o) WLOCK(&(o)->lock)
98-
#define APC_WUNLOCK(o) WUNLOCK(&(o)->lock)
99-
#define APC_RLOCK(o) RLOCK(&(o)->lock)
100-
#define APC_RUNLOCK(o) RUNLOCK(&(o)->lock) /* }}} */
101-
10296
/* atomic operations */
10397
#ifdef PHP_WIN32
10498
# ifdef _WIN64

0 commit comments

Comments
 (0)