On 2/4/2019 2:37 PM, J. Bruce Fields wrote:
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?
Shifts and Rotates are expensive on many processors, and hard to
pipeline for byteawap because each step needs to wait for the result
of the previous.
Maybe this is less of an issue with modern processors, but I'd
certainly not be surprised that embedded processors "did it the
hard way".
Tom.
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
