Re: [PATCH v7 04/25] RDMA/rtrs: core: lib functions shared between client and server modules

[Leon Romanovsky <leon@kernel.org>]

On Mon, Jan 20, 2020 at 12:32:00PM +0100, Jinpu Wang wrote:
> On Sun, Jan 19, 2020 at 3:48 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Thu, Jan 16, 2020 at 01:58:54PM +0100, Jack Wang wrote:
> > > From: Jack Wang <jinpu.wang@cloud.ionos.com>
> > >
> > > This is a set of library functions existing as a rtrs-core module,
> > > used by client and server modules.
> > >
> > > Mainly these functions wrap IB and RDMA calls and provide a bit higher
> > > abstraction for implementing of RTRS protocol on client or server
> > > sides.
> > >
> > > Signed-off-by: Danil Kipnis <danil.kipnis@cloud.ionos.com>
> > > Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
> > > ---
> > >  drivers/infiniband/ulp/rtrs/rtrs.c | 597 +++++++++++++++++++++++++++++
> > >  1 file changed, 597 insertions(+)
> > >  create mode 100644 drivers/infiniband/ulp/rtrs/rtrs.c
> > >
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs.c b/drivers/infiniband/ulp/rtrs/rtrs.c
> > > new file mode 100644
> > > index 000000000000..7b84d76e2a67
> > > --- /dev/null
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs.c
> > > @@ -0,0 +1,597 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * RDMA Transport Layer
> > > + *
> > > + * Copyright (c) 2014 - 2018 ProfitBricks GmbH. All rights reserved.
> > > + *
> > > + * Copyright (c) 2018 - 2019 1&1 IONOS Cloud GmbH. All rights reserved.
> > > + *
> > > + * Copyright (c) 2019 - 2020 1&1 IONOS SE. All rights reserved.
> > > + */
> > > +#undef pr_fmt
> > > +#define pr_fmt(fmt) KBUILD_MODNAME " L" __stringify(__LINE__) ": " fmt
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/inet.h>
> > > +
> > > +#include "rtrs-pri.h"
> > > +#include "rtrs-log.h"
> > > +
> > > +MODULE_DESCRIPTION("RDMA Transport Core");
> > > +MODULE_LICENSE("GPL");
> > > +
> > > +struct rtrs_iu *rtrs_iu_alloc(u32 queue_size, size_t size, gfp_t gfp_mask,
> > > +                           struct ib_device *dma_dev,
> > > +                           enum dma_data_direction dir,
> > > +                           void (*done)(struct ib_cq *cq, struct ib_wc *wc))
> > > +{
> > > +     struct rtrs_iu *ius, *iu;
> > > +     int i;
> > > +
> > > +     WARN_ON(!queue_size);
> > > +     ius = kcalloc(queue_size, sizeof(*ius), gfp_mask);
> > > +     if (unlikely(!ius))
> > > +             return NULL;
> >
> > Let's do not add useless WARN_ON() and unlikely to every error path.
> I can remove the WARN_ON, but the unlikey for error case seems normal to use,
> small size memory allocation is unlikely to fail.

The unlikely() makes sense in data-path only and mostly it doesn't give
any performance advantage, kcalloc() in this function means that this
function is not performance oriented.

As general note, as long as this code will leave impression of
not-stable enough, we will have hard time to accept it. The overuse of
WARN_ON(), debug prints and micro-optimizations add to such feeling.

Thanks