On Mon, Feb 04, 2019 at 09:56:20AM -0500, Chuck Lever wrote:
>
>
> > On Feb 4, 2019, at 9:32 AM, Christoph Hellwig <hch@xxxxxxxxxxxxx> 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
>
>
