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.
The code still has to be syntatically correct.
Jason
