Re: [PATCH v2 02/20] xprtrdma: Modernize htonl and ntohl

[Anna Schumaker <Anna.Schumaker-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>]

On 01/16/2015 01:56 PM, Chuck Lever wrote:
> 
> On Jan 16, 2015, at 1:33 PM, Anna Schumaker <Anna.Schumaker-HgOvQuBEEgRhl2p70BpVqQ@public.gmane.orgm> wrote:
> 
>> Hi Chuck,
>>
>> On 01/13/2015 11:25 AM, Chuck Lever wrote:
>>> Clean up: Replace htonl and ntohl with the be32 equivalents.
>>
>> After applying this patch I still see an ntohl() in both xprtrdma/transport.c and xprtrdma/verbs.c.  Should these be changed as well?
> 
> Thanks for the review!
> 
> The one in verbs.c is removed in 08/20.

I'll take a look in a sec, I'm up to 07/20 :)

> 
> I was mostly interested in updating the XDR usage of the byte
> swapping macros. For sin_addr, transport.c uses ntohl() the same way
> as xprtsock.c. It’s typical for IP address manipulation to use ntohl().
> git grep shows only two or three places in the kernel where the new
> style byte-swap macros are used with sin_addr.

Fair enough.  Thanks for answering!

Anna

> 
> The code in xprt_rdma_format_addresses() should be fixed up to handle
> IPv6 too.
> 
>> Thanks,
>> Anna
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>> ---
>>> include/linux/sunrpc/rpc_rdma.h |    9 +++++++
>>> include/linux/sunrpc/svc_rdma.h |    2 --
>>> net/sunrpc/xprtrdma/rpc_rdma.c  |   48 +++++++++++++++++++++------------------
>>> 3 files changed, 35 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/include/linux/sunrpc/rpc_rdma.h b/include/linux/sunrpc/rpc_rdma.h
>>> index b78f16b..1578ed2 100644
>>> --- a/include/linux/sunrpc/rpc_rdma.h
>>> +++ b/include/linux/sunrpc/rpc_rdma.h
>>> @@ -42,6 +42,9 @@
>>>
>>> #include <linux/types.h>
>>>
>>> +#define RPCRDMA_VERSION		1
>>> +#define rpcrdma_version		cpu_to_be32(RPCRDMA_VERSION)
>>> +
>>> struct rpcrdma_segment {
>>> 	__be32 rs_handle;	/* Registered memory handle */
>>> 	__be32 rs_length;	/* Length of the chunk in bytes */
>>> @@ -115,4 +118,10 @@ enum rpcrdma_proc {
>>> 	RDMA_ERROR = 4		/* An RPC RDMA encoding error */
>>> };
>>>
>>> +#define rdma_msg	cpu_to_be32(RDMA_MSG)
>>> +#define rdma_nomsg	cpu_to_be32(RDMA_NOMSG)
>>> +#define rdma_msgp	cpu_to_be32(RDMA_MSGP)
>>> +#define rdma_done	cpu_to_be32(RDMA_DONE)
>>> +#define rdma_error	cpu_to_be32(RDMA_ERROR)
>>> +
>>> #endif				/* _LINUX_SUNRPC_RPC_RDMA_H */
>>> diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h
>>> index 975da75..ddfe88f 100644
>>> --- a/include/linux/sunrpc/svc_rdma.h
>>> +++ b/include/linux/sunrpc/svc_rdma.h
>>> @@ -63,8 +63,6 @@ extern atomic_t rdma_stat_rq_prod;
>>> extern atomic_t rdma_stat_sq_poll;
>>> extern atomic_t rdma_stat_sq_prod;
>>>
>>> -#define RPCRDMA_VERSION 1
>>> -
>>> /*
>>>  * Contexts are built when an RDMA request is created and are a
>>>  * record of the resources that can be recovered when the request
>>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
>>> index df01d12..a6fb30b 100644
>>> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
>>> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
>>> @@ -209,9 +209,11 @@ rpcrdma_create_chunks(struct rpc_rqst *rqst, struct xdr_buf *target,
>>> 		if (cur_rchunk) {	/* read */
>>> 			cur_rchunk->rc_discrim = xdr_one;
>>> 			/* all read chunks have the same "position" */
>>> -			cur_rchunk->rc_position = htonl(pos);
>>> -			cur_rchunk->rc_target.rs_handle = htonl(seg->mr_rkey);
>>> -			cur_rchunk->rc_target.rs_length = htonl(seg->mr_len);
>>> +			cur_rchunk->rc_position = cpu_to_be32(pos);
>>> +			cur_rchunk->rc_target.rs_handle =
>>> +						cpu_to_be32(seg->mr_rkey);
>>> +			cur_rchunk->rc_target.rs_length =
>>> +						cpu_to_be32(seg->mr_len);
>>> 			xdr_encode_hyper(
>>> 					(__be32 *)&cur_rchunk->rc_target.rs_offset,
>>> 					seg->mr_base);
>>> @@ -222,8 +224,10 @@ rpcrdma_create_chunks(struct rpc_rqst *rqst, struct xdr_buf *target,
>>> 			cur_rchunk++;
>>> 			r_xprt->rx_stats.read_chunk_count++;
>>> 		} else {		/* write/reply */
>>> -			cur_wchunk->wc_target.rs_handle = htonl(seg->mr_rkey);
>>> -			cur_wchunk->wc_target.rs_length = htonl(seg->mr_len);
>>> +			cur_wchunk->wc_target.rs_handle =
>>> +						cpu_to_be32(seg->mr_rkey);
>>> +			cur_wchunk->wc_target.rs_length =
>>> +						cpu_to_be32(seg->mr_len);
>>> 			xdr_encode_hyper(
>>> 					(__be32 *)&cur_wchunk->wc_target.rs_offset,
>>> 					seg->mr_base);
>>> @@ -257,7 +261,7 @@ rpcrdma_create_chunks(struct rpc_rqst *rqst, struct xdr_buf *target,
>>> 		*iptr++ = xdr_zero;	/* encode a NULL reply chunk */
>>> 	} else {
>>> 		warray->wc_discrim = xdr_one;
>>> -		warray->wc_nchunks = htonl(nchunks);
>>> +		warray->wc_nchunks = cpu_to_be32(nchunks);
>>> 		iptr = (__be32 *) cur_wchunk;
>>> 		if (type == rpcrdma_writech) {
>>> 			*iptr++ = xdr_zero; /* finish the write chunk list */
>>> @@ -404,11 +408,11 @@ rpcrdma_marshal_req(struct rpc_rqst *rqst)
>>>
>>> 	/* build RDMA header in private area at front */
>>> 	headerp = (struct rpcrdma_msg *) req->rl_base;
>>> -	/* don't htonl XID, it's already done in request */
>>> +	/* don't byte-swap XID, it's already done in request */
>>> 	headerp->rm_xid = rqst->rq_xid;
>>> -	headerp->rm_vers = xdr_one;
>>> -	headerp->rm_credit = htonl(r_xprt->rx_buf.rb_max_requests);
>>> -	headerp->rm_type = htonl(RDMA_MSG);
>>> +	headerp->rm_vers = rpcrdma_version;
>>> +	headerp->rm_credit = cpu_to_be32(r_xprt->rx_buf.rb_max_requests);
>>> +	headerp->rm_type = rdma_msg;
>>>
>>> 	/*
>>> 	 * Chunks needed for results?
>>> @@ -482,11 +486,11 @@ rpcrdma_marshal_req(struct rpc_rqst *rqst)
>>> 						RPCRDMA_INLINE_PAD_VALUE(rqst));
>>>
>>> 		if (padlen) {
>>> -			headerp->rm_type = htonl(RDMA_MSGP);
>>> +			headerp->rm_type = rdma_msgp;
>>> 			headerp->rm_body.rm_padded.rm_align =
>>> -				htonl(RPCRDMA_INLINE_PAD_VALUE(rqst));
>>> +				cpu_to_be32(RPCRDMA_INLINE_PAD_VALUE(rqst));
>>> 			headerp->rm_body.rm_padded.rm_thresh =
>>> -				htonl(RPCRDMA_INLINE_PAD_THRESH);
>>> +				cpu_to_be32(RPCRDMA_INLINE_PAD_THRESH);
>>> 			headerp->rm_body.rm_padded.rm_pempty[0] = xdr_zero;
>>> 			headerp->rm_body.rm_padded.rm_pempty[1] = xdr_zero;
>>> 			headerp->rm_body.rm_padded.rm_pempty[2] = xdr_zero;
>>> @@ -570,7 +574,7 @@ rpcrdma_count_chunks(struct rpcrdma_rep *rep, unsigned int max, int wrchunk, __b
>>> 	unsigned int i, total_len;
>>> 	struct rpcrdma_write_chunk *cur_wchunk;
>>>
>>> -	i = ntohl(**iptrp);	/* get array count */
>>> +	i = be32_to_cpu(**iptrp);
>>> 	if (i > max)
>>> 		return -1;
>>> 	cur_wchunk = (struct rpcrdma_write_chunk *) (*iptrp + 1);
>>> @@ -582,11 +586,11 @@ rpcrdma_count_chunks(struct rpcrdma_rep *rep, unsigned int max, int wrchunk, __b
>>> 			xdr_decode_hyper((__be32 *)&seg->rs_offset, &off);
>>> 			dprintk("RPC:       %s: chunk %d@0x%llx:0x%x\n",
>>> 				__func__,
>>> -				ntohl(seg->rs_length),
>>> +				be32_to_cpu(seg->rs_length),
>>> 				(unsigned long long)off,
>>> -				ntohl(seg->rs_handle));
>>> +				be32_to_cpu(seg->rs_handle));
>>> 		}
>>> -		total_len += ntohl(seg->rs_length);
>>> +		total_len += be32_to_cpu(seg->rs_length);
>>> 		++cur_wchunk;
>>> 	}
>>> 	/* check and adjust for properly terminated write chunk */
>>> @@ -749,9 +753,9 @@ rpcrdma_reply_handler(struct rpcrdma_rep *rep)
>>> 		goto repost;
>>> 	}
>>> 	headerp = (struct rpcrdma_msg *) rep->rr_base;
>>> -	if (headerp->rm_vers != xdr_one) {
>>> +	if (headerp->rm_vers != rpcrdma_version) {
>>> 		dprintk("RPC:       %s: invalid version %d\n",
>>> -			__func__, ntohl(headerp->rm_vers));
>>> +			__func__, be32_to_cpu(headerp->rm_vers));
>>> 		goto repost;
>>> 	}
>>>
>>> @@ -793,7 +797,7 @@ repost:
>>> 	/* check for expected message types */
>>> 	/* The order of some of these tests is important. */
>>> 	switch (headerp->rm_type) {
>>> -	case htonl(RDMA_MSG):
>>> +	case rdma_msg:
>>> 		/* never expect read chunks */
>>> 		/* never expect reply chunks (two ways to check) */
>>> 		/* never expect write chunks without having offered RDMA */
>>> @@ -832,7 +836,7 @@ repost:
>>> 		rpcrdma_inline_fixup(rqst, (char *)iptr, rep->rr_len, rdmalen);
>>> 		break;
>>>
>>> -	case htonl(RDMA_NOMSG):
>>> +	case rdma_nomsg:
>>> 		/* never expect read or write chunks, always reply chunks */
>>> 		if (headerp->rm_body.rm_chunks[0] != xdr_zero ||
>>> 		    headerp->rm_body.rm_chunks[1] != xdr_zero ||
>>> @@ -853,7 +857,7 @@ badheader:
>>> 		dprintk("%s: invalid rpcrdma reply header (type %d):"
>>> 				" chunks[012] == %d %d %d"
>>> 				" expected chunks <= %d\n",
>>> -				__func__, ntohl(headerp->rm_type),
>>> +				__func__, be32_to_cpu(headerp->rm_type),
>>> 				headerp->rm_body.rm_chunks[0],
>>> 				headerp->rm_body.rm_chunks[1],
>>> 				headerp->rm_body.rm_chunks[2],
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html