> -----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.
(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
