> On Feb 3, 2019, at 10:00 AM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>
> On Sat, 2019-02-02 at 17:46 -0500, Chuck Lever wrote:
>>> On Feb 1, 2019, at 9:30 PM, Tom Talpey <tom@xxxxxxxxxx> wrote:
>>>
>>> On 2/1/2019 2:57 PM, Chuck Lever wrote:
>>>> Byte-swapping causes a CPU pipeline bubble on some processors.
>>>> When
>>>> a decoder is comparing an on-the-wire value for equality, byte-
>>>> swapping can be avoided by comparing it directly to a pre-byte-
>>>> swapped constant value.
>>>> The current set of pre-xdr'd constants is missing some common
>>>> values
>>>> used in the RPC header. Fill those out.
>>>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>>>> ---
>>>> include/linux/sunrpc/auth_gss.h | 5 ++-
>>>> include/linux/sunrpc/xdr.h | 66 ++++++++++++++++++++++++-
>>>> --------------
>>>> 2 files changed, 45 insertions(+), 26 deletions(-)
>>>> diff --git a/include/linux/sunrpc/auth_gss.h
>>>> b/include/linux/sunrpc/auth_gss.h
>>>> index 30427b7..adc4be2 100644
>>>> --- a/include/linux/sunrpc/auth_gss.h
>>>> +++ b/include/linux/sunrpc/auth_gss.h
>>>> @@ -19,7 +19,10 @@
>>>> #include <linux/sunrpc/svc.h>
>>>> #include <linux/sunrpc/gss_api.h>
>>>> -#define RPC_GSS_VERSION 1
>>>> +enum {
>>>> + RPC_GSS_VERSION = 1,
>>>> + rpc_gss_version = cpu_to_be32(RPC_GSS_VERSION)
>>>> +};
>>>> #define MAXSEQ 0x80000000 /* maximum legal sequence number,
>>>> from rfc 2203 */
>>>> diff --git a/include/linux/sunrpc/xdr.h
>>>> b/include/linux/sunrpc/xdr.h
>>>> index 787939d..69161cb 100644
>>>> --- a/include/linux/sunrpc/xdr.h
>>>> +++ b/include/linux/sunrpc/xdr.h
>>>> @@ -17,6 +17,7 @@
>>>> #include <asm/byteorder.h>
>>>> #include <asm/unaligned.h>
>>>> #include <linux/scatterlist.h>
>>>> +#include <linux/sunrpc/msg_prot.h>
>>>> struct bio_vec;
>>>> struct rpc_rqst;
>>>> @@ -79,31 +80,46 @@ struct xdr_buf {
>>>> buf->buflen = len;
>>>> }
>>>> -/*
>>>> - * pre-xdr'ed macros.
>>>> - */
>>>> -
>>>> -#define xdr_zero cpu_to_be32(0)
>>>> -#define xdr_one cpu_to_be32(1)
>>>> -#define xdr_two cpu_to_be32(2)
>>>> -
>>>> -#define rpc_success cpu_to_be32(RPC_SUCCESS)
>>>> -#define rpc_prog_unavail cpu_to_be32(RPC_PROG_UNAVAIL)
>>>> -#define rpc_prog_mismatch cpu_to_be32(RPC_PROG_MISMATCH)
>>>> -#define rpc_proc_unavail cpu_to_be32(RPC_PROC_UNAVAIL)
>>>> -#define rpc_garbage_args cpu_to_be32(RPC_GARBAGE_ARGS)
>>>> -#define rpc_system_err cpu_to_be32(RPC_SYSTEM_ER
>>>> R)
>>>> -#define rpc_drop_reply cpu_to_be32(RPC_DROP_REPL
>>>> Y)
>>>> -
>>>> -#define rpc_auth_ok cpu_to_be32(RPC_AUTH_OK)
>>>> -#define rpc_autherr_badcred cpu_to_be32(RPC_AUTH_BADCRED)
>>>> -#define rpc_autherr_rejectedcred
>>>> cpu_to_be32(RPC_AUTH_REJECTEDCRED)
>>>> -#define rpc_autherr_badverf cpu_to_be32(RPC_AUTH_BADVERF)
>>>> -#define rpc_autherr_rejectedverf
>>>> cpu_to_be32(RPC_AUTH_REJECTEDVERF)
>>>> -#define rpc_autherr_tooweak cpu_to_be32(RPC_AUTH_TOOWEAK)
>>>> -#define rpcsec_gsserr_credproblem cpu_to_be32(RPCSEC_GSS_CR
>>>> EDPROBLEM)
>>>> -#define rpcsec_gsserr_ctxproblem cpu_to_be32(RPCSEC_GSS_CT
>>>> XPROBLEM)
>>>> -#define rpc_autherr_oldseqnum cpu_to_be32(101)
>>>> +enum xdr_be32_equivalents {
>>>> + xdr_zero = cpu_to_be32(0),
>>>> + xdr_one = cpu_to_be32(1),
>>>> + xdr_two = cpu_to_be32(2),
>>>
>>> It is clever to use an enum to pre-compute these values, but
>>
>> Perhaps not clever; it is a current Linux kernel coding
>> practice to use an enum in favor of a C macro for constants.
>
> Sure, but won't that confuse 'sparse' static checking? These constants
> will no longer appear as being big endian.
Agreed, that is not a desirable situation for these symbolic constants.
> Doesn't gcc's __builtin_bswap32() already compute the result at compile
> time when you feed it a constant value? AFAICS it is supposed to, which
> is why we use it directly in include/uapi/linux/swab.h instead of using
> a special cased __builtin_constant_p().
The return type of __builtin_bswap32 is uint32_t, not __be32.
I will stick with cpu_to_be32(), and simply add the missing constants.
--
Chuck Lever
