Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-22859

Possible hang in InnoDB rw_lock_lock_word_decr()

Details

    Description

      In MDEV-10813, the function rw_lock_lock_word_decr() was simplified in an incorrect way:

      @@ -275,12 +275,11 @@ rw_lock_lock_word_decr(
      		 
       	os_rmb;
      		 
       	local_lock_word = lock->lock_word;
      		 
       	while (local_lock_word > threshold) {
      		 
      -		if (os_compare_and_swap_lint(&lock->lock_word,
      		 
      -					     local_lock_word,
      		 
      -					     local_lock_word - amount)) {
      		 
      +		if (my_atomic_caslint(&lock->lock_word,
      		 
      +				      &local_lock_word,
      		 
      +				      local_lock_word - amount)) {
      		 
       			return(true);
      		 
       		}
      		 
      -		local_lock_word = lock->lock_word;
      		 
       	}
      		 
       	return(false);
      		 
       #else /* INNODB_RW_LOCKS_USE_ATOMICS */

      Because we sample the local_lock_word only once at the start of the loop, it is possible that the following happens during rw_lock_s_lock():

      1. Another thread is holding an S-latch, so that local_lock_word will be X_LOCK_DECR-1.
      2. The other thread will release its S-latch and never come back.
      3. An infinite loop will occur, because lock->lock_word will be X_LOCK_DECR from that point onwards.
      4. Only a subsequent S-latch acquisition from another thread could resolve the hang.

      Attachments

        Issue Links

          is caused by

          Task - A task that needs to be done. MDEV-10813 Clean-up InnoDB atomics, memory barriers and mutexes

          • Critical - Crashes, loss of data, severe memory leak.
          • Closed
          relates to

          Bug - A problem which impairs or prevents the functions of the product. MDEV-22850 Reduce buf_pool.page_hash latch contention

          • Major - Major loss of function.
          • Closed

          Activity

            marko Marko Mäkelä added a comment -

            Sorry, I missed the fact that my_atomic_caslint() is reloading the local_lock_word. On GCC-compatible compilers, the macro maps to the following code:

            #define make_atomic_cas_body64                                    \
            		 
              asm volatile ("push %%ebx;"                                     \
            		 
                            "movl (%%ecx), %%ebx;"                            \
            		 
                            "movl 4(%%ecx), %%ecx;"                           \
            		 
                            LOCK_prefix "; cmpxchg8b (%%esi);"                \
            		 
                            "setz %2; pop %%ebx"                              \
            		 
                            : "+S" (a), "+A" (*cmp), "=c" (ret)               \
            		 
                            : "c" (&set)                                      \
            		 
                            : "memory", "esp")

            The "+S" means that the parameter a is both read and written by the assembler snippet via the esi (or rsi) register. The + feels unnecessary to me, because we are changing the data that esi (or rsi) points to, not esi itself. The "+A" means that the register pair eax:edx would be both read and written on IA-32. So, it says that cmp will be assigned to.

            The cmpxchg8b instruction is documented to operate on eax:edx and ecx:ebx and its operand. So, everything looks correct after all.

            The previous primitive os_compare_and_swap_lint() failed to return the value that was read.

            marko Marko Mäkelä added a comment - Sorry, I missed the fact that my_atomic_caslint() is reloading the local_lock_word . On GCC-compatible compilers, the macro maps to the following code: #define make_atomic_cas_body64 \ asm volatile ( "push %%ebx;" \ "movl (%%ecx), %%ebx;" \ "movl 4(%%ecx), %%ecx;" \ LOCK_prefix "; cmpxchg8b (%%esi);" \ "setz %2; pop %%ebx" \ : "+S" (a), "+A" (*cmp), "=c" (ret) \ : "c" (&set) \ : "memory" , "esp" ) The "+S" means that the parameter a is both read and written by the assembler snippet via the esi (or rsi ) register. The + feels unnecessary to me, because we are changing the data that esi (or rsi ) points to, not esi itself. The "+A" means that the register pair eax:edx would be both read and written on IA-32. So, it says that cmp will be assigned to. The cmpxchg8b instruction is documented to operate on eax:edx and ecx:ebx and its operand. So, everything looks correct after all. The previous primitive os_compare_and_swap_lint() failed to return the value that was read.

            People

              marko Marko Mäkelä
              marko Marko Mäkelä
              Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.