On Mon, Apr 01, 2019 at 03:07:09PM +0300, Gal Pressman wrote:
> On 01-Apr-19 11:47, Leon Romanovsky wrote:
> > From: Mark Zhang <markz@xxxxxxxxxxxx>
> >
> > In auto mode all QPs belong to one category are bind automatically to
> > a single counter set. Currently only "qp type" is supported.
> >
> > In this mode the qp counter is set in RST2INIT modification, and when
> > a qp is destroyed the counter is unbound.
>
> This seems like a strange lifetime choice, why set it on RST2INIT and not on
> creation? Isn't that very tied to mlx5 implementation?
It is our attempt to delay depletion of HW objects (counters). They are
limited in number and change RST2INIT means that this QP was enabled.
>
> >
> > Signed-off-by: Mark Zhang <markz@xxxxxxxxxxxx>
> > Reviewed-by: Majd Dibbiny <majd@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > ---
> > drivers/infiniband/core/counters.c | 222 +++++++++++++++++++++++++++++
> > drivers/infiniband/core/device.c | 2 +
> > drivers/infiniband/core/verbs.c | 9 ++
> > include/rdma/ib_verbs.h | 20 +++
> > include/rdma/rdma_counter.h | 8 ++
> > 5 files changed, 261 insertions(+)
> > +
> > +static int __rdma_counter_bind_qp(struct rdma_counter *counter,
> > + struct ib_qp *qp)
> > +{
> > + int ret;
> > +
> > + if (qp->counter)
> > + return -EINVAL;
>
> So a certain resource (QP) cannot be bind to more than one counter?
Right, it is not HW limitation, but desire to do not over complicate
SW implementation.
>
> > +
> > + if (!qp->device->ops.counter_bind_qp)
> > + return -EOPNOTSUPP;
> > +
> > + mutex_lock(&counter->lock);
> > + ret = qp->device->ops.counter_bind_qp(counter, qp);
> > + mutex_unlock(&counter->lock);
> > +
> > + return ret;
> > +}
> > +
> > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> > index ab61be545c6c..c52f07f33eb5 100644
> > --- a/include/rdma/ib_verbs.h
> > +++ b/include/rdma/ib_verbs.h
> > @@ -1791,6 +1791,11 @@ struct ib_qp {
> > * Implementation details of the RDMA core, don't use in drivers:
> > */
> > struct rdma_restrack_entry res;
> > +
> > + /*
> > + * The counter qp bind to
> > + */
>
> A single line comment is enough here, while at it I would change to "The counter
> the qp is bind to".
Thanks
>
> > + struct rdma_counter *counter;
> > };
> >
> > struct ib_dm {
> > @@ -2556,6 +2561,21 @@ struct ib_device_ops {
> > */
> > void (*dealloc_driver)(struct ib_device *dev);
> >
> > + /**
> > + * counter_bind_qp - Bind a QP to a counter.
> > + * @counter - The counter to be bound. If counter->id is zero then
> > + * the driver needs to allocate a new counter and set counter->id
> > + */
> > + int (*counter_bind_qp)(struct rdma_counter *counter, struct ib_qp *qp);
> > + /**
> > + * counter_unbind_qp - Unbind the qp from the dynamically-allocated
> > + * counter and bind it onto the default one. If this is the last
> > + * bound qp, then this counter will be deallocated.
> > + * @force - If it is true then free the counter in case of any error.
> > + * used in cases like qp destroy.
> > + */
> > + int (*counter_unbind_qp)(struct ib_qp *qp, bool force);
> > +
>
> Are we going to add bind/unbind callbacks for every resource type? Maybe have
> one bind/unbind callback with a resource type parameter?
It is internal to kernel and to our subsystem, so I preferred to see how it
is evolving before asking internally for implementing very general thing.
I can definitely be wrong about it.
>
> > DECLARE_RDMA_OBJ_SIZE(ib_ah);
> > DECLARE_RDMA_OBJ_SIZE(ib_pd);
> > DECLARE_RDMA_OBJ_SIZE(ib_srq);
Attachment:
signature.asc
Description: PGP signature
