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

[bfields@fieldses.org (J. Bruce Fields)]

On Mon, Feb 04, 2019 at 09:56:20AM -0500, Chuck Lever wrote:
> 
> 
> > On Feb 4, 2019, at 9:32 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > 
> > On Mon, Feb 04, 2019 at 09:16:54AM -0500, Chuck Lever wrote:
> >> They are. The problem is that we are byte-swapping the incoming wire
> >> data and then comparing to CPU-endian constants in some hot paths.
> >> It's better to leave the incoming data alone and compare to a pre-
> >> byte-swapped constant. This patch adds some of these constants that
> >> were missing, in preparation for fixing the hot paths.
> >> 
> >> That is apparently not clear from the patch description, so I will
> >> endeavor to improve it.
> > 
> > Why do we need new enums / #defines for that?
> > 
> > Just replace:
> > 
> > 	if (beXX_to_cpu(pkt->field) == SOME_CONSTANT)
> > 
> > with
> > 
> > 	if (pkt->field == cpu_to_be32(SOME_CONSTANT))
> > 
> > and we are done.
> 
> True.

I'm a little surprised the compiler couldn't do that automatically.
(Disclaimer: I know nothing about compilers.)

> > The latter is a pretty common pattern through the kernel.
> 
> However the pattern in the NFS server and lockd is to define a
> lower-case version of the same macro that is pre-byte-swapped.
> I'm attempting to follow existing precedent in this area.

I've never really understood why that was done.  (Not saying it was
wrong, just that I don't understand it.)  As long as you're reading the
value, how could byte-swapping it actually add a significant expense?

The one thing I do like is that I can look at e.g.:

	int nfsd4_get_nfs4_acl	...
	__be32 nfsd4_set_nfs4_acl ...

and tell that the former returns a linux errno, the latter an NFS error.

--b.

> 
> We already have, for example, in various sunrpc headers:
> 
> enum rpc_accept_stat {
>         RPC_SUCCESS = 0,
>         RPC_PROG_UNAVAIL = 1,
>         RPC_PROG_MISMATCH = 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)
> 
> Or, for NFS:
> 
> enum nfs_stat {
>    ...
>    NFSERR_SHARE_DENIED = 10015,
>    ...
> };
> 
> #define nfserr_share_denied cpu_to_be32(NFSERR_SHARE_DENIED)
> 
> There are some missing lower-case macros, which I am trying to
> add to our existing collection before I rewrite the RPC header
> encoding and decoding functions. So I'm adding:
> 
> +       rpc_gss_version = cpu_to_be32(RPC_GSS_VERSION),
> 
> +       rpc_call                = cpu_to_be32(RPC_CALL),
> +       rpc_reply               = cpu_to_be32(RPC_REPLY),
> +
> +       rpc_msg_accepted        = cpu_to_be32(RPC_MSG_ACCEPTED),
> +       rpc_msg_denied          = cpu_to_be32(RPC_MSG_DENIED),
> 
> Actually since we have decided not to use enum for these, this
> smaller addition can simply be squashed into the later patches,
> and I can drop this patch, which was intended as a clean-up but
> now appears to be unnecessary.
> 
> --
> Chuck Lever
> 
>