On Wed, Dec 19, 2018 at 03:44:45PM +0000, Jason Gunthorpe wrote:
> On Wed, Dec 19, 2018 at 12:39:29PM +0200, Leon Romanovsky wrote:
> > On Tue, Dec 18, 2018 at 09:14:50PM +0200, Leon Romanovsky wrote:
> > > On Tue, Dec 18, 2018 at 05:39:30PM +0000, Jason Gunthorpe wrote:
> > > > On Tue, Dec 18, 2018 at 02:15:56PM +0200, Leon Romanovsky wrote:
> > > > > From: Huy Nguyen <huyn@xxxxxxxxxxxx>
> > > > >
> > > > > On NVMe offloads connection with many IO queues, EEH takes long time to
> > > > > recover. The culprit is the synchronize_srcu in the destroy_mkey. Solution
> > > > > is to use synchronize_srcu only for ODP mkey.
> > > > >
> > > > > Fixes: b4cfe447d47b ("IB/mlx5: Implement on demand paging by adding support for MMU notifiers")
> > > > > Signed-off-by: Huy Nguyen <huyn@xxxxxxxxxxxx>
> > > > > Reviewed-by: Daniel Jurgens <danielj@xxxxxxxxxxxx>
> > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > drivers/infiniband/hw/mlx5/mr.c | 19 ++++++++++++++++---
> > > > > 1 file changed, 16 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
> > > > > index d45a9163c198..0f01059724e9 100644
> > > > > +++ b/drivers/infiniband/hw/mlx5/mr.c
> > > > > @@ -73,7 +73,8 @@ static int destroy_mkey(struct mlx5_ib_dev *dev, struct mlx5_ib_mr *mr)
> > > > >
> > > > > #ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING
> > > > > /* Wait until all page fault handlers using the mr complete. */
> > > > > - synchronize_srcu(&dev->mr_srcu);
> > > > > + if (mr->umem && mr->umem->is_odp)
> > > > > + synchronize_srcu(&dev->mr_srcu);
> > > > > #endif
> > > >
> > > > We don't need any of these gross #ifdef's..
> > > >
> > > > Add a
> > > >
> > > > static inline bool is_odp_mr(struct mlx5_ib_mr *mr)
> > > > {
> > > > return IS_ENABLED(CONFIG_INFINIBAND_ON_DEMAND_PAGING) && mr->umem && mr->umem->is_odp;
> > > > }
> > > >
> > > > And use it in place of all the is_odp tests.
> > > >
> > > > Since it returns the constant false if ODP is disabled the compiler
> > > > will drop all the branches naturally without the horrible #ifdef mess.
> > > >
> > > > Someone should send a cleanup patch for ODP to get rid of more
> > > > #ifdefs.
> > >
> > > Jason,
> > >
> > > Let's take this patch as is and I'll clean ODP code after that in one shot.
> > >
> > > Thanks
> >
> > Jason,
> >
> > After more thinking and trials, your claim that compiler will optimize
> > out this code is not fully correct. My compiler continues to try to
> > access those variables.
> >
> > _ kernel git:(rdma-next) _ gcc --version
> > gcc (GCC) 8.2.1 20181011 (Red Hat 8.2.1-4)
> >
> > The patch below produces the following errors while I'm trying to
> > compile with CONFIG_INFINIBAND_ON_DEMAND_PAGING=n
> >
> > _ kernel git:(rdma-next) time make -j64 -s
> > drivers/infiniband/hw/mlx5/mr.c: In function _destroy_mkey_:
> > drivers/infiniband/hw/mlx5/mr.c:76:24: error: _struct mlx5_ib_dev_ has no member named _mr_srcu_
> > synchronize_srcu(&dev->mr_srcu);
> > ^~
> > drivers/infiniband/hw/mlx5/mr.c: In function _remove_keys_:
> > drivers/infiniband/hw/mlx5/mr.c:260:24: error: _struct mlx5_ib_dev_ has no member named _mr_srcu_
> > synchronize_srcu(&dev->mr_srcu);
> > ^~
>
> Generally you don't also put #ifdefs around the structure members when
> doing this... ie that is an unnecessary micro optimization.
In mlx5_ib, I can easily drop "#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING", but what about "struct ib_ucontext"? Are you going to accept it?
Anyway, can we please progress with current patch and continue
discussion?
>
> The code still has to be syntatically correct.
>
> Jason
Attachment:
signature.asc
Description: PGP signature
