Hi Sergei, thanks for you comments. Answers inline. On Fri, Mar 13, 2015 at 08:58:38PM +0100, Sergei Golubchik wrote:
Hi, Sergey!
On Mar 12, svoj@mariadb.org wrote:
revision-id: 20e5c37ccfac9d95a2db1f295f211656df1ce160 parent(s): 190858d996e7dc90e01fe8a5e7daec6af0012b23 committer: Sergey Vojtovich branch nick: 10.1 timestamp: 2015-03-12 18:15:08 +0400 message:
MDEV-7728 - Spiral patch 032_mariadb-10.0.15.scale_registering_xid.diff
XID cache is now based on lock-free hash. Also fixed lf_hash_destroy() to call alloc destructor.
Note that previous implementation had race condition when thread was accessing XA owned by different thread. This new implementation doesn't fix it either. ...skip...
@@ -5106,120 +5109,205 @@ void mark_transaction_to_rollback(THD *thd, bool all) /*************************************************************************** Handling of XA id cacheing ***************************************************************************/ +class XID_cache_element +{ + uint m_state;
shouldn't it be uint32 ? That's the type that my_atomic_add32/cas32 work with. Yes, it should. Thanks!
+ static const uint DELETED= 1 << 31; +public: + XID_STATE *m_xid_state;
O-okay. I *think* your hand-written locking logic works. But please add a comment, explaining how it works, all four methods, the meaning of DELETED, etc. Perhaps DELETED should rather be NOT_INITIALIZED?
A very valid request. IMHO mutex is overkill here, that's why I implemented this trivial locking. I'll add comments and think about better names.
+ bool lock() + { + if (my_atomic_add32_explicit(&m_state, 1, MY_MEMORY_ORDER_ACQUIRE) & DELETED) + { + unlock(); + return false; + } + return true; + } + void unlock() + { + my_atomic_add32_explicit(&m_state, -1, MY_MEMORY_ORDER_RELEASE); + } + void mark_deleted() + { + uint old= 0; + while (!my_atomic_cas32_weak_explicit(&m_state, &old, DELETED, + MY_MEMORY_ORDER_RELAXED, + MY_MEMORY_ORDER_RELAXED))
usually one should use LF_BACKOFF inside spin-loops.
Good point. Though LF_BACKOFF doesn't seem to do anything useful.
+ old= 0; + } + void mark_not_deleted() + { + DBUG_ASSERT(m_state & DELETED); + my_atomic_add32_explicit(&m_state, -DELETED, MY_MEMORY_ORDER_RELAXED); + } + static void lf_hash_initializer(LF_HASH *hash __attribute__((unused)), + XID_cache_element *element, + XID_STATE *xid_state) + { + element->m_xid_state= xid_state; + xid_state->xid_cache_element= element; + } + static void lf_alloc_constructor(uchar *ptr) + { + XID_cache_element *element= (XID_cache_element*) (ptr + LF_HASH_OVERHEAD); + element->m_state= DELETED; + } + static void lf_alloc_destructor(uchar *ptr) + { + XID_cache_element *element= (XID_cache_element*) (ptr + LF_HASH_OVERHEAD); + if (element->m_state != DELETED)
How can this happen? You mark an element DELETED before lf_hash_delete()
That's for elements inserted by ha_recover(). There's no guarantee that they will be deleted. ...skip...
-XID_STATE *xid_cache_search(XID *xid) + +XID_STATE *xid_cache_search(THD *thd, XID *xid) { - mysql_mutex_lock(&LOCK_xid_cache); - XID_STATE *res=(XID_STATE *)my_hash_search(&xid_cache, xid->key(), - xid->key_length()); - mysql_mutex_unlock(&LOCK_xid_cache); - return res; + DBUG_ASSERT(thd->xid_hash_pins); + XID_cache_element *element= + (XID_cache_element*) lf_hash_search(&xid_cache, thd->xid_hash_pins, + xid->key(), xid->key_length()); + if (element) + { + lf_hash_search_unpin(thd->xid_hash_pins); + return element->m_xid_state;
Normally, one should not access the element after it's unpinned, so the protocol is
XID_STATE *state= element->m_xid_state; lf_hash_search_unpin(thd->xid_hash_pins); return state;
Perhaps your locking/deleted m_state guarantees that element is safe to access? But I failed to see that :( This is noted in cset comment: Note that previous implementation had race condition when thread was accessing XA owned by different thread. This new implementation doesn't fix it either.
I can fix it if you like.
diff --git a/storage/spider/spd_table.cc b/storage/spider/spd_table.cc index 36768ae..411c7ae 100644 --- a/storage/spider/spd_table.cc +++ b/storage/spider/spd_table.cc @@ -41,11 +41,13 @@
I'd prefer to have spider changes in a separate commit Ok, I'll split them.
...skip... Thanks, Sergey