OK, sounds reasonable. So if we follow this reasoning there are more places to fix in the post send function:
static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
struct mlx5_wqe_ctrl_seg **ctrl,
struct ib_send_wr *wr, unsigned *idx,
int *size, int nreq)
{
int err = 0;
if (unlikely(mlx5_wq_overflow(&qp->sq, nreq, qp->ibqp.send_cq))) {
err = -ENOMEM;
return err;
}
-----Original Message-----
From: Chuck Lever [mailto:chuck.lever@xxxxxxxxxx]
Sent: Thursday, July 28, 2016 8:19 AM
To: Eli Cohen <eli@xxxxxxxxxxxx>
Cc: Leon Romanovsky <leonro@xxxxxxxxxxxx>; Matan Barak <matanb@xxxxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too many SGEs
Hi Eli-
> On Jul 28, 2016, at 9:08 AM, Eli Cohen <eli@xxxxxxxxxxxx> wrote:
>
> I am not sure I agree with this. I intentionally put ENOMEM since this indicates that the WQ buffer is not large enough to contain all the s/g entries. I return EINVAL if the opcode is invalid for example.
All other drivers I reviewed return EINVAL in this case. So there is an implied API contract already.
The functional issue here is that the consumer has attempted to post a Send WR with an incorrect num_sge value. sq.max_gs is a limit also specified by the consumer. There's no dynamic memory allocation here to fail.
ENOMEM would be correct if a dynamic memory allocation failed, where posting again with the same arguments is likely to succeed. In this case, an invalid parameter has been provided by the consumer, which is a permanent error.
Therefore EINVAL is the correct return.
> -----Original Message-----
> From: linux-rdma-owner@xxxxxxxxxxxxxxx
> [mailto:linux-rdma-owner@xxxxxxxxxxxxxxx] On Behalf Of Leon Romanovsky
> Sent: Thursday, July 28, 2016 12:23 AM
> To: Chuck Lever <chuck.lever@xxxxxxxxxx>
> Cc: Matan Barak <matanb@xxxxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] IB-mlx5: Return EINVAL when caller specifies too
> many SGEs
>
> On Wed, Jul 27, 2016 at 03:38:36PM -0400, Chuck Lever wrote:
>> Other similar functions (mlx5_ib_post_recv being the closest) return
>> EINVAL in this case.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>
> Thanks Chuck,
> You are absolutely right, it should be EINVAL error.
> Do you mind if I submit it by myself together with other mlx5 fixes?
>
> Looks ok, except title :)
> Acked-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>
>> ---
>> Hi Matan, Leon-
>>
>> I noticed this nit while debugging a problem in xprtrdma.
>>
>> drivers/infiniband/hw/mlx5/qp.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/hw/mlx5/qp.c
>> b/drivers/infiniband/hw/mlx5/qp.c index ce0a7ab..b0e5498 100644
>> --- a/drivers/infiniband/hw/mlx5/qp.c
>> +++ b/drivers/infiniband/hw/mlx5/qp.c
>> @@ -3436,7 +3436,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>> num_sge = wr->num_sge;
>> if (unlikely(num_sge > qp->sq.max_gs)) {
>> mlx5_ib_warn(dev, "\n");
>> - err = -ENOMEM;
>> + err = -EINVAL;
>> *bad_wr = wr;
>> goto out;
>> }
>>
>> --
>> 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
--
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
