Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants

[Trond Myklebust <trondmy@hammerspace.com>]

On Sat, 2019-02-02 at 17:46 -0500, Chuck Lever wrote:
> > On Feb 1, 2019, at 9:30 PM, Tom Talpey <tom@talpey.com> 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@oracle.com>
> > > ---
> > >  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.

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().

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com