PBXT

Merge lp:~vkolesnikov/pbxt/pbxt-heap-corruption into lp:pbxt

  1. pbxt-heap-corruption
  2. Merge into trunk
Proposed by Vladimir Kolesnikov
Status: Merged
Merged at revision: not available
Proposed branch: lp:~vkolesnikov/pbxt/pbxt-heap-corruption
Merge into: lp:pbxt
Diff against target: None lines
To merge this branch: bzr merge lp:~vkolesnikov/pbxt/pbxt-heap-corruption
Reviewer Review Type Date Requested Status
PBXT Core Pending
Review via email: mp+6828@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Vladimir Kolesnikov (vkolesnikov) wrote :

All tests passed on dev8.

Revision history for this message
Paul McCullagh (paul-mccullagh) wrote :

Hi Vlad,

This patch:

On May 27, 2009, at 5:40 PM, Vladimir Kolesnikov wrote:

> if (table_pool->opt_locked && !table_pool->opt_flushing) {
> - table_pool->opt_total_open--;
> /* Table will be closed below: */
> - if (table_pool->opt_total_open > 0)
> + if (table_pool->opt_total_open == 1)
> flush_table = FALSE;
> }

reverse the original logic.

Previously the table was only flushed when the last open table object
was closed.

Now it has been changed to, the table is always flushed, but not when
the last open table object is closed.

Was there are reason for changing this?

--
Paul McCullagh
PrimeBase Technologies
www.primebase.org
www.blobstreaming.org
pbxt.blogspot.com

Revision history for this message
Paul McCullagh (paul-mccullagh) wrote :

Hi Vlad,

On May 27, 2009, at 5:40 PM, Vladimir Kolesnikov wrote:

> +
> + if (ot) {
> xt_close_table(ot, flush_table, FALSE);
>
> + /* assume that table_pool cannot be invalidated in between as we
> have table_pool->opt_total_open > 0 */
> + xt_lock_mutex_ns(&db->db_ot_pool.opt_lock);
> + table_pool->opt_total_open--;

We need to move this code to here:

 db_free_open_table_pool(NULL, table_pool);

 if (!xt_broadcast_cond_ns(&db->db_ot_pool.opt_cond))
  goto failed;

Because threads that wait on opt_cond are either waiting for
table_pool->opt_total_open == 0 or for table_pool->opt_locked == 0.

Although "table_pool->opt_locked" is not adjusted by this function.

>
> + xt_unlock_mutex_ns(&db->db_ot_pool.opt_lock);
> + }
> +

--
Paul McCullagh
PrimeBase Technologies
www.primebase.org
www.blobstreaming.org
pbxt.blogspot.com

Revision history for this message
Paul McCullagh (paul-mccullagh) wrote :

On May 27, 2009, at 5:40 PM, Vladimir Kolesnikov wrote:

> - if (file_ptr->fil_memmap) {
> + if (file_ptr->fil_memmap && !file_ptr->fil_handle_count) {
> fs_close_fmap(self, file_ptr->fil_memmap);
> file_ptr->fil_memmap = NULL;
> }
> @@ -169,7 +169,8 @@
> file_ptr->fil_path = NULL;
> }
>
> - xt_free(self, file_ptr);

Small optimization, "!file_ptr->fil_ref_count && " is not required
here (checked above).

>
> + if (!file_ptr->fil_ref_count && !file_ptr->fil_handle_count)
> + xt_free(self, file_ptr);
> }
> }

--
Paul McCullagh
PrimeBase Technologies
www.primebase.org
www.blobstreaming.org
pbxt.blogspot.com

Revision history for this message
Paul McCullagh (paul-mccullagh) wrote :

I don't think " && !file_ptr->fil_handle_count" is correct here.

The "fs_globals.fsg_open_files" file object list only depends on the
"fil_ref_count" reference count.

It is OK to remove it from the list when this count goes to zero.

The open table object will still have a reference to the object, but
it can no longer be found using the list.

On May 27, 2009, at 5:40 PM, Vladimir Kolesnikov wrote:

> @@ -500,7 +501,7 @@
> pushr_(xt_sl_unlock, fs_globals.fsg_open_files);
>
> file_ptr->fil_ref_count--;
> - if (!file_ptr->fil_ref_count) {
> + if (!file_ptr->fil_ref_count && !file_ptr->fil_handle_count) {
> xt_sl_delete(self, fs_globals.fsg_open_files, file_ptr->fil_path);
> }

--
Paul McCullagh
PrimeBase Technologies
www.primebase.org
www.blobstreaming.org
pbxt.blogspot.com

Revision history for this message
Vladimir Kolesnikov (vkolesnikov) wrote :

my bug...

Revision history for this message
Vladimir Kolesnikov (vkolesnikov) wrote :

yes, i see...

Revision history for this message
Vladimir Kolesnikov (vkolesnikov) wrote :

ok, i thought we should retain it in the list while it's bbeing referenced. will fix.

lp:~vkolesnikov/pbxt/pbxt-heap-corruption updated
637. By Vladimir Kolesnikov

merged with upstream

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ChangeLog'
2--- ChangeLog 2009-05-11 11:50:46 +0000
3+++ ChangeLog 2009-05-27 14:45:14 +0000
4@@ -3,6 +3,8 @@
5
6 ------- 1.0.08 RC - Not yet released
7
8+RN241: Fixed a heap corruption bug
9+
10 RN238: Added an option to wait for the sweeper to clean up old transactions on a particular connection. This prevents the sweeper from getting too far behind.
11
12 RN237: Added an option to lazy delete fixed length index entries. This means the index entries are just marked for deletion, instead of removing the items from the index page. This has the advantage that an exclusive lock is not always required for deletion.
13
14=== modified file 'src/database_xt.cc'
15--- src/database_xt.cc 2009-05-11 10:35:16 +0000
16+++ src/database_xt.cc 2009-05-27 14:39:04 +0000
17@@ -1133,9 +1133,8 @@
18 goto failed;
19
20 if (table_pool->opt_locked && !table_pool->opt_flushing) {
21- table_pool->opt_total_open--;
22 /* Table will be closed below: */
23- if (table_pool->opt_total_open > 0)
24+ if (table_pool->opt_total_open == 1)
25 flush_table = FALSE;
26 }
27 else {
28@@ -1166,9 +1165,16 @@
29 if (!xt_broadcast_cond_ns(&db->db_ot_pool.opt_cond))
30 goto failed;
31 xt_unlock_mutex_ns(&db->db_ot_pool.opt_lock);
32- if (ot)
33+
34+ if (ot) {
35 xt_close_table(ot, flush_table, FALSE);
36
37+ /* assume that table_pool cannot be invalidated in between as we have table_pool->opt_total_open > 0 */
38+ xt_lock_mutex_ns(&db->db_ot_pool.opt_lock);
39+ table_pool->opt_total_open--;
40+ xt_unlock_mutex_ns(&db->db_ot_pool.opt_lock);
41+ }
42+
43 return;
44
45 failed:
46
47=== modified file 'src/filesys_xt.cc'
48--- src/filesys_xt.cc 2009-04-26 23:44:06 +0000
49+++ src/filesys_xt.cc 2009-05-27 14:39:04 +0000
50@@ -152,7 +152,7 @@
51 file_ptr->fil_filedes = XT_NULL_FD;
52 }
53
54- if (file_ptr->fil_memmap) {
55+ if (file_ptr->fil_memmap && !file_ptr->fil_handle_count) {
56 fs_close_fmap(self, file_ptr->fil_memmap);
57 file_ptr->fil_memmap = NULL;
58 }
59@@ -169,7 +169,8 @@
60 file_ptr->fil_path = NULL;
61 }
62
63- xt_free(self, file_ptr);
64+ if (!file_ptr->fil_ref_count && !file_ptr->fil_handle_count)
65+ xt_free(self, file_ptr);
66 }
67 }
68
69@@ -500,7 +501,7 @@
70 pushr_(xt_sl_unlock, fs_globals.fsg_open_files);
71
72 file_ptr->fil_ref_count--;
73- if (!file_ptr->fil_ref_count) {
74+ if (!file_ptr->fil_ref_count && !file_ptr->fil_handle_count) {
75 xt_sl_delete(self, fs_globals.fsg_open_files, file_ptr->fil_path);
76 }
77
78@@ -1262,20 +1263,13 @@
79 xtPublic void xt_close_fmap(XTThreadPtr self, XTMapFilePtr map)
80 {
81 if (map->fr_file) {
82- xt_fs_release_file(self, map->fr_file);
83-
84 xt_sl_lock(self, fs_globals.fsg_open_files);
85 pushr_(xt_sl_unlock, fs_globals.fsg_open_files);
86-
87 map->fr_file->fil_handle_count--;
88- if (!map->fr_file->fil_handle_count)
89- fs_free_file(self, NULL, &map->fr_file);
90-
91 freer_();
92-
93+
94+ xt_fs_release_file(self, map->fr_file);
95 map->fr_file = NULL;
96-
97-
98 }
99 map->mf_memmap = NULL;
100 xt_free(self, map);
101
102=== modified file 'src/heap_xt.cc'
103--- src/heap_xt.cc 2009-04-02 20:27:49 +0000
104+++ src/heap_xt.cc 2009-05-27 14:42:16 +0000
105@@ -31,7 +31,7 @@
106 #undef xt_heap_new
107 #endif
108
109-#ifdef DEBUG
110+#ifdef DEBUG_MEMORY
111 xtPublic XTHeapPtr xt_mm_heap_new(XTThreadPtr self, size_t size, XTFinalizeFunc finalize, u_int line, c_char *file, xtBool track)
112 #else
113 xtPublic XTHeapPtr xt_heap_new(XTThreadPtr self, size_t size, XTFinalizeFunc finalize)
114@@ -39,7 +39,7 @@
115 {
116 volatile XTHeapPtr hp;
117
118-#ifdef DEBUG
119+#ifdef DEBUG_MEMORY
120 hp = (XTHeapPtr) xt_mm_calloc(self, size, line, file);
121 hp->h_track = track;
122 if (track)
123@@ -67,19 +67,19 @@
124
125 xtPublic void xt_check_heap(XTThreadPtr self __attribute__((unused)), XTHeapPtr hp __attribute__((unused)))
126 {
127-#ifdef DEBUG
128+#ifdef DEBUG_MEMORY
129 xt_mm_malloc_size(self, hp);
130 #endif
131 }
132
133-#ifdef DEBUG
134+#ifdef DEBUG_MEMORY
135 xtPublic void xt_mm_heap_reference(XTThreadPtr self, XTHeapPtr hp, u_int line, c_char *file)
136 #else
137 xtPublic void xt_heap_reference(XTThreadPtr, XTHeapPtr hp)
138 #endif
139 {
140 xt_spinlock_lock(&hp->h_lock);
141-#ifdef DEBUG
142+#ifdef DEBUG_MEMORY
143 if (hp->h_track)
144 printf("HEAP: +1 %d->%d %s:%d\n", (int) hp->h_ref_count, (int) hp->h_ref_count+1, file, (int) line);
145 #endif
146@@ -91,7 +91,7 @@
147 {
148 if (!hp)
149 return;
150-#ifdef DEBUG
151+#ifdef DEBUG_MEMORY
152 xt_spinlock_lock(&hp->h_lock);
153 ASSERT(hp->h_ref_count != 0);
154 xt_spinlock_unlock(&hp->h_lock);
155@@ -100,7 +100,7 @@
156 if (hp->h_onrelease)
157 (*hp->h_onrelease)(self, hp);
158 if (hp->h_ref_count > 0) {
159-#ifdef DEBUG
160+#ifdef DEBUG_MEMORY
161 if (hp->h_track)
162 printf("HEAP: -1 %d->%d\n", (int) hp->h_ref_count, (int) hp->h_ref_count-1);
163 #endif
164
165=== modified file 'src/heap_xt.h'
166--- src/heap_xt.h 2009-03-24 16:24:48 +0000
167+++ src/heap_xt.h 2009-05-27 14:42:16 +0000
168@@ -25,6 +25,7 @@
169
170 #include "xt_defs.h"
171 #include "lock_xt.h"
172+#include "memory_xt.h"
173
174 struct XTThread;
175
176@@ -59,7 +60,7 @@
177
178 void xt_check_heap(struct XTThread *self, XTHeapPtr mem);
179
180-#ifdef DEBUG
181+#ifdef DEBUG_MEMORY
182 #define xt_heap_new(t, s, f) xt_mm_heap_new(t, s, f, __LINE__, __FILE__, FALSE)
183 #define xt_heap_new_track(t, s, f) xt_mm_heap_new(t, s, f, __LINE__, __FILE__, TRUE)
184 #define xt_heap_reference(t, s) xt_mm_heap_reference(t, s, __LINE__, __FILE__)
185
186=== modified file 'src/locklist_xt.cc'
187--- src/locklist_xt.cc 2009-02-20 14:26:09 +0000
188+++ src/locklist_xt.cc 2009-05-27 14:41:19 +0000
189@@ -77,6 +77,12 @@
190 ptr->li_lock_type = XTThreadLockInfo::ATOMIC_RW_LOCK;
191 }
192
193+void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, XTSkewRWLock *lock)
194+{
195+ ptr->li_skew_rwlock = lock;
196+ ptr->li_lock_type = XTThreadLockInfo::SKEW_RW_LOCK;
197+}
198+
199 void xt_thread_lock_info_free(XTThreadLockInfoPtr ptr)
200 {
201 /* TODO: check to see if it's present in a thread's list */
202@@ -168,7 +174,7 @@
203 break;
204 case XTThreadLockInfo::SPIN_RW_LOCK:
205 lock_type = "XTSpinRWLock";
206- lock_name = li->li_spin_rwlock->srw_name;
207+ lock_name = li->li_spin_rwlock->nrw_name;
208 break;
209 case XTThreadLockInfo::ATOMIC_RW_LOCK:
210 lock_type = "XTAtomicRWLock";
211
212=== modified file 'src/locklist_xt.h'
213--- src/locklist_xt.h 2009-02-20 14:26:09 +0000
214+++ src/locklist_xt.h 2009-05-27 14:41:19 +0000
215@@ -43,6 +43,7 @@
216 struct XTFastRWLock;
217 struct XTSpinRWLock;
218 struct XTAtomicRWLock;
219+struct XTSkewRWLock;
220
221 #ifdef XT_THREAD_LOCK_INFO
222
223@@ -61,7 +62,7 @@
224 */
225 typedef struct XTThreadLockInfo {
226
227- enum LockType { SPIN_LOCK, RW_MUTEX, MUTEX, RW_LOCK, FAST_LOCK, FAST_RW_LOCK, SPIN_RW_LOCK, ATOMIC_RW_LOCK };
228+ enum LockType { SPIN_LOCK, RW_MUTEX, MUTEX, RW_LOCK, FAST_LOCK, FAST_RW_LOCK, SPIN_RW_LOCK, ATOMIC_RW_LOCK, SKEW_RW_LOCK };
229
230 LockType li_lock_type;
231
232@@ -74,6 +75,7 @@
233 XTAtomicRWLock *li_atomic_rwlock; // ATOMIC_RW_LOCK
234 xt_mutex_struct *li_mutex; // MUTEX
235 xt_rwlock_struct *li_rwlock; // RW_LOCK
236+ XTSkewRWLock *li_skew_rwlock; // SKEW_RW_LOCK
237 };
238 }
239 XTThreadLockInfoRec, *XTThreadLockInfoPtr;
240@@ -86,6 +88,7 @@
241 void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, XTAtomicRWLock *lock);
242 void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, xt_mutex_struct *lock);
243 void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, xt_rwlock_struct *lock);
244+void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, XTSkewRWLock *lock);
245 void xt_thread_lock_info_free(XTThreadLockInfoPtr ptr);
246
247 void xt_thread_lock_info_add_owner (XTThreadLockInfoPtr ptr);
248
249=== modified file 'src/memory_xt.cc'
250--- src/memory_xt.cc 2009-03-19 11:28:00 +0000
251+++ src/memory_xt.cc 2009-05-27 14:42:16 +0000
252@@ -186,7 +186,7 @@
253 free(ptr);
254 }
255
256-#ifdef DEBUG
257+#ifdef DEBUG_MEMORY
258
259 /*
260 * -----------------------------------------------------------------------
261@@ -849,7 +849,7 @@
262
263 xtPublic xtBool xt_init_memory(void)
264 {
265-#ifdef DEBUG
266+#ifdef DEBUG_MEMORY
267 XTThreadPtr self = NULL;
268
269 if (!xt_init_mutex_with_autoname(NULL, &mm_mutex))
270@@ -875,7 +875,7 @@
271
272 xtPublic void xt_exit_memory(void)
273 {
274-#ifdef DEBUG
275+#ifdef DEBUG_MEMORY
276 long mm;
277 int i;
278
279@@ -919,7 +919,7 @@
280 * MEMORY ALLOCATION UTILITIES
281 */
282
283-#ifdef DEBUG
284+#ifdef DEBUG_MEMORY
285 char *xt_mm_dup_string(XTThreadPtr self, c_char *str, u_int line, c_char *file)
286 #else
287 char *xt_dup_string(XTThreadPtr self, c_char *str)
288@@ -931,7 +931,7 @@
289 if (!str)
290 return NULL;
291 len = strlen(str);
292-#ifdef DEBUG
293+#ifdef DEBUG_MEMORY
294 new_str = (char *) xt_mm_malloc(self, len + 1, line, file);
295 #else
296 new_str = (char *) xt_malloc(self, len + 1);
297
298=== modified file 'src/memory_xt.h'
299--- src/memory_xt.h 2008-11-21 11:30:42 +0000
300+++ src/memory_xt.h 2009-05-27 14:42:16 +0000
301@@ -30,6 +30,10 @@
302 struct XTThread;
303
304 #ifdef DEBUG
305+#define DEBUG_MEMORY
306+#endif
307+
308+#ifdef DEBUG_MEMORY
309
310 #define XT_MM_STACK_TRACE 200
311 #define XT_MM_TRACE_DEPTH 4
312@@ -109,7 +113,7 @@
313
314 #endif
315
316-#ifdef DEBUG
317+#ifdef DEBUG_MEMORY
318 #define xt_dup_string(t, s) xt_mm_dup_string(t, s, __LINE__, __FILE__)
319
320 char *xt_mm_dup_string(struct XTThread *self, const char *path, u_int line, const char *file);

Subscribers

People subscribed via source and target branches