Re: [PATCH 1/4] infiniband/core: Add protection for shared CQs used by ULPs

[Leon Romanovsky <leon@kernel.org>]

On Mon, May 11, 2020 at 02:59:44PM +0300, Yamin Friedman wrote:
>
> On 5/11/2020 7:37 AM, Leon Romanovsky wrote:
> > On Sun, May 10, 2020 at 05:55:54PM +0300, Yamin Friedman wrote:
> > > A pre-step for adding shared CQs. Add the infra-structure to prevent
> > > shared CQ users from altering the CQ configurations. For now all cqs are
> > > marked as private (non-shared). The core driver should use the new force
> > > functions to perform resize/destroy/moderation changes that are not
> > > allowed for users of shared CQs.
> > >
> > > Signed-off-by: Yamin Friedman <yaminf@mellanox.com>
> > > Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>
> > > ---
> > >   drivers/infiniband/core/cq.c    | 25 ++++++++++++++++++-------
> > >   drivers/infiniband/core/verbs.c | 37 ++++++++++++++++++++++++++++++++++---
> > >   include/rdma/ib_verbs.h         | 20 +++++++++++++++++++-
> > >   3 files changed, 71 insertions(+), 11 deletions(-)
> > infiniband/core -> RDMA/core
> Will fix.
> >
> > > diff --git a/drivers/infiniband/core/cq.c b/drivers/infiniband/core/cq.c
> > > index 4f25b24..443a9cd 100644
> > > --- a/drivers/infiniband/core/cq.c
> > > +++ b/drivers/infiniband/core/cq.c
> > > @@ -37,6 +37,7 @@ static void ib_cq_rdma_dim_work(struct work_struct *w)
> > >   {
> > >   	struct dim *dim = container_of(w, struct dim, work);
> > >   	struct ib_cq *cq = dim->priv;
> > > +	int ret;
> > >
> > >   	u16 usec = rdma_dim_prof[dim->profile_ix].usec;
> > >   	u16 comps = rdma_dim_prof[dim->profile_ix].comps;
> > > @@ -44,7 +45,10 @@ static void ib_cq_rdma_dim_work(struct work_struct *w)
> > >   	dim->state = DIM_START_MEASURE;
> > >
> > >   	trace_cq_modify(cq, comps, usec);
> > > -	cq->device->ops.modify_cq(cq, comps, usec);
> > > +	ret = rdma_set_cq_moderation_force(cq, comps, usec);
> > > +	if (ret)
> > > +		WARN_ONCE(1, "Failed set moderation for CQ 0x%p\n", cq);
> > First WARN_ONCE(ret, ...), second no to pointer address print and third
> > this dump stack won't help, because CQ moderation will fail for many
> > reasons unrelated to the caller.
> Would it be better to not include any warning for failed calls?

At least for most of the places, the answer is yes, you are better to
delete WARN_*s.

WARN_*s are good thing to catch programmers errors, something that can't
be but happened. It is wrong to use them inform about the failures.

Thanks