> On Jan 18, 2019, at 7:52 PM, Parav Pandit <parav@xxxxxxxxxxxx> wrote:
>
>
>
>> -----Original Message-----
>> From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
>> owner@xxxxxxxxxxxxxxx> On Behalf Of Leon Romanovsky
>> Sent: Thursday, January 17, 2019 11:54 AM
>> To: Doug Ledford <dledford@xxxxxxxxxx>; Jason Gunthorpe
>> <jgg@xxxxxxxxxxxx>
>> Cc: Danit Goldberg <danitg@xxxxxxxxxxxx>; RDMA mailing list <linux-
>> rdma@xxxxxxxxxxxxxxx>; Moni Shoua <monis@xxxxxxxxxxxx>; Leon
>> Romanovsky <leonro@xxxxxxxxxxxx>
>> Subject: [PATCH rdma-next] IB/cma: Define option to set ack timeout and
>> pack tos_set
>>
>> From: Danit Goldberg <danitg@xxxxxxxxxxxx>
>>
>> Define new option in 'rdma_set_option' to override calculated QP timeout
>> when requested to provide QP attributes to modify a QP.
>>
>> At the same time, pack tos_set to be bitfield.
>>
>> Signed-off-by: Danit Goldberg <danitg@xxxxxxxxxxxx>
>> Reviewed-by: Moni Shoua <monis@xxxxxxxxxxxx>
>> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>> ---
>> drivers/infiniband/core/cma.c | 14 ++++++++++++++
>> drivers/infiniband/core/cma_priv.h | 4 +++-
>> drivers/infiniband/core/ucma.c | 7 +++++++
>> include/rdma/rdma_cm.h | 7 +++++++
>> include/uapi/rdma/rdma_user_cm.h | 4 ++++
>> 5 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>> index 3880fbb3bee7..d4c861f64194 100644
>> --- a/drivers/infiniband/core/cma.c
>> +++ b/drivers/infiniband/core/cma.c
>> @@ -888,6 +888,7 @@ struct rdma_cm_id *__rdma_create_id(struct net
>> *net,
>> id_priv->id.ps = ps;
>> id_priv->id.qp_type = qp_type;
>> id_priv->tos_set = false;
>> + id_priv->timeout_set = false;
>> id_priv->gid_type = IB_GID_TYPE_IB;
>> spin_lock_init(&id_priv->lock);
>> mutex_init(&id_priv->qp_mutex);
>> @@ -1130,6 +1131,9 @@ int rdma_init_qp_attr(struct rdma_cm_id *id,
>> struct ib_qp_attr *qp_attr,
>> } else
>> ret = -ENOSYS;
>>
>> + if ((*qp_attr_mask & IB_QP_TIMEOUT) && id_priv->timeout_set)
>> + qp_attr->timeout = id_priv->timeout;
>> +
>> return ret;
>> }
>> EXPORT_SYMBOL(rdma_init_qp_attr);
>> @@ -2490,6 +2494,16 @@ void rdma_set_service_type(struct rdma_cm_id
>> *id, int tos) } EXPORT_SYMBOL(rdma_set_service_type);
>>
>> +void rdma_set_ack_timeout(struct rdma_cm_id *id, u8 timeout) {
>> + struct rdma_id_private *id_priv;
>> +
>> + id_priv = container_of(id, struct rdma_id_private, id);
>> + id_priv->timeout = timeout;
>> + id_priv->timeout_set = true;
>> +}
>> +EXPORT_SYMBOL(rdma_set_ack_timeout);
>> +
> Please include kernel doc format comment block for this exported symbol, as this is standard practice.
> IB spec defined ack timeout is 5-bit value so currently u8 looks fine.
> However it's better to define it as 16-bit value to not invent a new v2 version of API in future if its gets extended.
Which part? The rdma_set_option only and take u8 out of it?
> (There is startup working on running RoCE over WAN).
> You can argue why not use 32-bit. Instead of 3-bits, double size seems good enough to me, hence request for 16-bit.
>
> Please mention following things in the comment and about the API.
> (a) unit of timeout if it follows the 4.0 * 2 (x) usec equation
> (b) mention that setting this on active or passive side only affect in single direction of the QP (it is not negotiated with remote side)
> (c) mention that zero disables the timer
> (d) applicable to primary path only
> (even though most pieces of rdmacm are primary part, some quirks exist for primary and alternative paths)
>
>> static void cma_query_handler(int status, struct sa_path_rec *path_rec,
>> void *context)
>> {
>> diff --git a/drivers/infiniband/core/cma_priv.h
>> b/drivers/infiniband/core/cma_priv.h
>> index cf47c69436a7..ca7307277518 100644
>> --- a/drivers/infiniband/core/cma_priv.h
>> +++ b/drivers/infiniband/core/cma_priv.h
>> @@ -84,9 +84,11 @@ struct rdma_id_private {
>> u32 options;
>> u8 srq;
>> u8 tos;
>> - bool tos_set;
>> + u8 tos_set:1;
>> + u8 timeout_set:1;
>> u8 reuseaddr;
>> u8 afonly;
>> + u8 timeout;
>> enum ib_gid_type gid_type;
>>
>> /*
>> diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
>> index 01d68ed46c1b..236b60136459 100644
>> --- a/drivers/infiniband/core/ucma.c
>> +++ b/drivers/infiniband/core/ucma.c
>> @@ -1236,6 +1236,13 @@ static int ucma_set_option_id(struct
>> ucma_context *ctx, int optname,
>> }
>> ret = rdma_set_afonly(ctx->cm_id, *((int *) optval) ? 1 : 0);
>> break;
>> + case RDMA_OPTION_ID_ACK_TIMEOUT:
>> + if (optlen != sizeof(u8)) {
>> + ret = -EINVAL;
>> + break;
>> + }
>> + rdma_set_ack_timeout(ctx->cm_id, *((u8 *) optval));
>> + break;
>> default:
>> ret = -ENOSYS;
>> }
>> diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h index
>> 008dc8bbe2fb..3309aa44e400 100644
>> --- a/include/rdma/rdma_cm.h
>> +++ b/include/rdma/rdma_cm.h
>> @@ -374,6 +374,13 @@ int rdma_set_reuseaddr(struct rdma_cm_id *id, int
>> reuse);
>> */
>> int rdma_set_afonly(struct rdma_cm_id *id, int afonly);
>>
>> +/**
>> + * rdma_set_ack_timeout - Set the ack timeout of QP associated with a
>> + * connection identifier.
>> + * @id: Communication identifier to associated with service type.
>> + * @timeout: Ack timeout to set a QP.
>> + */
>> +void rdma_set_ack_timeout(struct rdma_cm_id *id, u8 timeout);
>> /**
>> * rdma_get_service_id - Return the IB service ID for a specified address.
>> * @id: Communication identifier associated with the address.
>> diff --git a/include/uapi/rdma/rdma_user_cm.h
>> b/include/uapi/rdma/rdma_user_cm.h
>> index 0d1e78ebad05..e42940a215a3 100644
>> --- a/include/uapi/rdma/rdma_user_cm.h
>> +++ b/include/uapi/rdma/rdma_user_cm.h
>> @@ -300,6 +300,10 @@ enum {
>> RDMA_OPTION_ID_TOS = 0,
>> RDMA_OPTION_ID_REUSEADDR = 1,
>> RDMA_OPTION_ID_AFONLY = 2,
>> + RDMA_OPTION_ID_ACK_TIMEOUT = 3
>> +};
>> +
>> +enum {
>> RDMA_OPTION_IB_PATH = 1
>> };
>>
>> --
>> 2.19.1
>
