> -----Original Message-----
> From: Majd Dibbiny
> Sent: Friday, January 18, 2019 12:13 PM
> To: Parav Pandit <parav@xxxxxxxxxxxx>
> Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford
> <dledford@xxxxxxxxxx>; Jason Gunthorpe <jgg@xxxxxxxxxxxx>; Danit
> Goldberg <danitg@xxxxxxxxxxxx>; RDMA mailing list <linux-
> rdma@xxxxxxxxxxxxxxx>; Moni Shoua <monis@xxxxxxxxxxxx>; Leon
> Romanovsky <leonro@xxxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next] IB/cma: Define option to set ack timeout and
> pack tos_set
>
>
> > 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?
I was saying for ucma_set_option_id() should accept 16-bit value because its facing userspace ABI.
But it is not needed.
I am sorry, even now sometimes I mix rnr timeout with ack timeout, which is incorrect. :-(
So u8 is still fine for ack_timeout covering much larger range.
> > (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
> >
