Hi Bart!
> On Jul 12, 2018, at 5:12 AM, Max Gurtovoy <maxg@xxxxxxxxxxxx> wrote:
>
>
>
> On 7/10/2018 9:27 PM, Bart Van Assche wrote:
>> Since neither ib_post_send() nor ib_post_recv() modify the data structure
>> their second argument points at, declare that argument const.
That's the mechanics of it, but I'd like this patch description to
justify why this is a wise change.
>> This change
>> makes it necessary to declare the 'bad_wr' argument const too and also
>> to modify all ULPs that call ib_post_send(), ib_post_recv() or
>> ib_post_srq_recv().
It's probably my own personal bias, but API contracts are less
readable when an output parameter is declared as a const. Adding
"const" in all the callers is also more cluttered, IMO.
Instead, can a typecast be done inside ib_post_send/recv where
bad_wr is set after an error? Just a (perhaps bad) thought.
>> This patch does not change any functionality.
>> To make this possible, only one cast had to be introduce that casts away
>> constness, namely in rpcrdma_post_recvs(). The only way I can think of
>> to avoid that cast is to change the data type of bad_wr from
>> struct ib_recv_wr ** into int (an index in the work request list). However,
>> that would require even more extensive changes than this patch.
An int would be an index into an array. The WR chain is a list.
Returning an index seems awkward to me for both the drivers and
the ULPs.
Jason has previously suggested that because it is the common case
that callers pass a dummy third argument, the primary post APIs
should drop the third parameter. Maybe that can help simplify this
situation.
>> Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxx>
[ ... snipped ... ]
>> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
>> index 112a15abc4a4..02d8fdad12ff 100644
>> --- a/net/sunrpc/xprtrdma/verbs.c
>> +++ b/net/sunrpc/xprtrdma/verbs.c
>> @@ -1516,7 +1516,8 @@ void
>> rpcrdma_post_recvs(struct rpcrdma_xprt *r_xprt, bool temp)
>> {
>> struct rpcrdma_buffer *buf = &r_xprt->rx_buf;
>> - struct ib_recv_wr *wr, *bad_wr;
>> + struct ib_recv_wr *wr;
>> + struct ib_recv_wr *bad_wr;
>
> const ?
If no type qualifier change is needed here, then perhaps this hunk
can be dropped.
>> int needed, count, rc;
>> needed = buf->rb_credits + (buf->rb_bc_srv_max_requests << 1);
>> @@ -1559,7 +1560,8 @@ rpcrdma_post_recvs(struct rpcrdma_xprt *r_xprt, bool temp)
>> if (!count)
>> return;
>> - rc = ib_post_recv(r_xprt->rx_ia.ri_id->qp, wr, &bad_wr);
>> + rc = ib_post_recv(r_xprt->rx_ia.ri_id->qp, wr,
>> + (const struct ib_recv_wr **)&bad_wr);
>
> is this cast needed ?
>
>> if (rc) {
>> for (wr = bad_wr; wr; wr = wr->next) {
>> struct rpcrdma_rep *rep;
--
Chuck Lever
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
