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, [email protected] 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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
