OS-6851: some bhyve atomics shims are too heavy-handed

Details

Issue Type:Improvement
Priority:4 - Normal
Status:Resolved
Created at:2018-03-29T19:40:58.337Z
Updated at:2018-04-16T15:23:47.464Z

People

Created by:Patrick Mooney [X]
Reported by:Patrick Mooney [X]
Assigned to:Patrick Mooney [X]

Resolution

Fixed: A fix for this issue is checked into the tree and tested.
(Resolution Date: 2018-04-16T15:23:47.454Z)

Fix Versions

2018-04-12 Promised Land (Release Date: 2018-04-12)

Related Links

Labels

bhyve

Description

While reviewing the compatibility shims for bhyve, due to an unrelated issue, I discovered that the definitions for our atomic_load_acq_* implementations are doing much more work than they need to. The FreeBSD definitions (on amd64), while somewhat roundabout in their phrasing, are as follows:

#define ATOMIC_LOAD(TYPE)                                       \
static __inline u_##TYPE                                        \
atomic_load_acq_##TYPE(volatile u_##TYPE *p)                    \
{                                                               \
        u_##TYPE res;                                           \
                                                                \
        res = *p;                                               \
        __compiler_membar();                                    \
        return (res);                                           \
}                                                               \

With compiler member defined here:

/*
 * Compiler memory barriers, specific to gcc and clang.
 */
#if defined(__GNUC__)
#define __compiler_membar()     __asm __volatile(" " : : : "memory")
#endif

That effectively evaluates to simply a volatile load. The existing shim, sourced from the initial Pluribus port, goes way beyond:

static __inline u_long
atomic_load_acq_long(volatile u_long *p)
{
        u_long res;

        __asm volatile("lock ; " "cmpxchgq %0,%1"
                       : "=a" (res), "=m" (*p)
                       : "m" (*p)
                       : "memory", "cc");
        return (res);
}

After confirming that the FreeBSD-native definitions compile down to a simple load (thanks to @dale.ghent for extracting that info from a running FreeBSD system). I'm confident we should migrate to the simpler and lighter version.

Comments

Comment by Patrick Mooney [X]
Created at 2018-04-02T20:14:37.464Z

The attached screenshot is from a FreeBSD system where @dale.ghent disassembled one of the functions in question.


Comment by Jira Bot
Created at 2018-04-02T20:40:20.647Z

illumos-joyent commit 89f28c29187081c8db00a633d949df9b5f2a403d (branch master, by Patrick Mooney)

OS-6851 some bhyve atomics shims are too heavy-handed
Reviewed by: John Levon <john.levon@joyent.com>
Reviewed by: Mike Gerdts <mike.gerdts@joyent.com>
Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com>
Approved by: Jerry Jelinek <jerry.jelinek@joyent.com>