Date: Tue, 18 Mar 2014 14:52:13 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-hackers@freebsd.org Cc: "Meyer, Conrad" <conrad.meyer@isilon.com>, Bryan Drewery <bdrewery@freebsd.org> Subject: Re: [PATCH] amd64/pcpu.h: Use Clang builtins for clarity when referencing thread's pcpu Message-ID: <201403181452.13685.jhb@freebsd.org> In-Reply-To: <1394821826-19412-1-git-send-email-conrad.meyer@isilon.com> References: <1394821826-19412-1-git-send-email-conrad.meyer@isilon.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday, March 14, 2014 2:31:08 pm Meyer, Conrad wrote:
> We can efficiently reference thread-local pcpu members via the %gs
> register with Clang-annotated C code, in place of inline GNU assembly.
>
> Motivations:
> - Use C in leiu of inline assembly for clarity
> - Clang's static analyser may be better able to understand PCPU_*
> macros using the C constructs rather than inline assembly
> (unverified)
>
> Sponsored by: EMC/Isilon storage division
> Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com>
> Reviewed-by: Max Laier <mlaier@FreeBSD.org>
> ---
> This is more of a "what do you think?" than a pull request. It seems like using
> annotated C instead of asm is nice (in particular, Clang detects casts from
> pointers typed with one segment to another, or unsegmented type). On the other
> hand, this is code that doesn't change frequently, and we may still need to
> support GCC for some time. So adding a second, parallel implementation just
> doubles room for bugs.
I think this is neat and wanted to look at doing this when I first noticed the
address_space() attribute in the clang docs.
> Open questions:
> - How long is GCC intended to be supported as a compiler?
That I don't know.
> - How atomic does PCPU_INC() need to be? It looks like it updates cpu-local
> counters; as long as it's a single asm instruction, should it be fine
> w.r.t. interrupts? The existing implementation does NOT use the 'lock; ' prefix.
I think a single instruction is fine.
> diff --git a/sys/amd64/include/pcpu.h b/sys/amd64/include/pcpu.h
> index fe898e9..68892fc 100644
> --- a/sys/amd64/include/pcpu.h
> +++ b/sys/amd64/include/pcpu.h
> +#define curthread __extension__ ({ \
> + *((volatile __pcpu_type(pc_curthread) __GS_RELATIVE *) \
> + __pcpu_offset(pc_curthread)); \
> +})
Would be nice to not lose the __pure2 attribute for curthread (you
might need it to still be an inline function to keep that)
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201403181452.13685.jhb>