Skip to content

Commit

Permalink
WT-2025 Pass simple arguments to compare-and-swap macros
Browse files Browse the repository at this point in the history 
Merge pull request #2111 from wiredtiger/cas-simple-args
(cherry picked from commit 5e75a5e)
  • Loading branch information
@michaelcahill
michaelcahill committed Aug 6, 2015
1 parent 426c9d2 commit 48648de
Showing 1 changed file with 12 additions and 5 deletions.
17 changes: 12 additions & 5 deletions src/include/serial.i
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,15 @@ __insert_simple_func(WT_SESSION_IMPL *session,
*
* All structure setup must be flushed before the structure is entered
* into the list. We need a write barrier here, our callers depend on
* it.
* it. Don't pass complex arguments to the macro, some implementations
* read the old value multiple times.
*/
for (i = 0; i < skipdepth; i++)
if (!WT_ATOMIC_CAS8(*ins_stack[i], new_ins->next[i], new_ins))
for (i = 0; i < skipdepth; i++) {
WT_INSERT *old_ins = *ins_stack[i];
if (old_ins != new_ins->next[i] ||
!WT_ATOMIC_CAS8(*ins_stack[i], old_ins, new_ins))
return (i == 0 ? WT_RESTART : 0);
}

return (0);
}
Expand Down Expand Up @@ -83,10 +87,13 @@ __insert_serial_func(WT_SESSION_IMPL *session, WT_INSERT_HEAD *ins_head,
*
* All structure setup must be flushed before the structure is entered
* into the list. We need a write barrier here, our callers depend on
* it.
* it. Don't pass complex arguments to the macro, some implementations
* read the old value multiple times.
*/
for (i = 0; i < skipdepth; i++) {
if (!WT_ATOMIC_CAS8(*ins_stack[i], new_ins->next[i], new_ins))
WT_INSERT *old_ins = *ins_stack[i];
if (old_ins != new_ins->next[i] ||
!WT_ATOMIC_CAS8(*ins_stack[i], old_ins, new_ins))
return (i == 0 ? WT_RESTART : 0);
if (ins_head->tail[i] == NULL ||
ins_stack[i] == &ins_head->tail[i]->next[i])
Expand Down

2 comments on commit 48648de

@keithbostic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelcahill, you didn't say how you found this one, but it looks like an easy mistake to make in the future, and a difficult one to debug.

What would you think of doing something like this in gcc.h:

static inline uint64_t
__wt_atomic_cas8(uint64_t *v, uint64_t old, uint64_t new)
{
        return (__sync_bool_compare_and_swap(v, old, new));
}
static inline void *
__wt_atomic_cas_ptr(void **v, void *old, void *new)
{
        return (__sync_bool_compare_and_swap(v, old, new));
}
static inline uint64_t
__wt_atomic_store8(uint64_t *v, uint64_t val)
{
        return (__sync_lock_test_and_set(v, val));
}

and so on? Then we know the argument is only evaluated once.

A positive is we get type checking (the WT_STATIC_ASSERT in the underlying macros goes away).

A negative is we have to specify the address of the object to the inline function, and we have separate functions for pointers and 8B objects.

Thoughts?

@keithbostic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #2112.

Please sign in to comment.