diff mbox series

[net-next] rds: rely on IB/core to determine if device is ODP capable

Message ID bfc8ffb7ea207ed90c777a4f61a8afe1badef212.1744109826.git.leonro@nvidia.com (mailing list archive)
State New
Headers show
Series [net-next] rds: rely on IB/core to determine if device is ODP capable | expand

Commit Message

Leon Romanovsky April 8, 2025, 11:04 a.m. UTC 
From: Leon Romanovsky <leonro@nvidia.com>

There is no need to perform checks if IB device ODP capable as
ib_reg_user_mr() will check all access flags anyway.

RDS is the only one in-kernel ODP user, so change return value for ODP
not supported case, to the value used by RDS.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/verbs.c | 2 +-
 net/rds/ib.c                    | 8 --------
 net/rds/ib.h                    | 1 -
 net/rds/ib_rdma.c               | 5 -----
 4 files changed, 1 insertion(+), 15 deletions(-)

Comments

Jason Gunthorpe April 8, 2025, 12:23 p.m. UTC | #1 
On Tue, Apr 08, 2025 at 02:04:55PM +0300, Leon Romanovsky wrote:
> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index 9826fe7f9d00..c62aa2ff4963 100644
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
> @@ -153,14 +153,6 @@ static int rds_ib_add_one(struct ib_device *device)
>  	rds_ibdev->max_wrs = device->attrs.max_qp_wr;
>  	rds_ibdev->max_sge = min(device->attrs.max_send_sge, RDS_IB_MAX_SGE);
>  
> -	rds_ibdev->odp_capable =
> -		!!(device->attrs.kernel_cap_flags &
> -		   IBK_ON_DEMAND_PAGING) &&
> -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> -		   IB_ODP_SUPPORT_WRITE) &&
> -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> -		   IB_ODP_SUPPORT_READ);

This patch seems to drop the check for WRITE and READ support on the
ODP.

Jason
Leon Romanovsky April 8, 2025, 12:34 p.m. UTC | #2 
On Tue, Apr 08, 2025 at 09:23:38AM -0300, Jason Gunthorpe wrote:
> On Tue, Apr 08, 2025 at 02:04:55PM +0300, Leon Romanovsky wrote:
> > diff --git a/net/rds/ib.c b/net/rds/ib.c
> > index 9826fe7f9d00..c62aa2ff4963 100644
> > --- a/net/rds/ib.c
> > +++ b/net/rds/ib.c
> > @@ -153,14 +153,6 @@ static int rds_ib_add_one(struct ib_device *device)
> >  	rds_ibdev->max_wrs = device->attrs.max_qp_wr;
> >  	rds_ibdev->max_sge = min(device->attrs.max_send_sge, RDS_IB_MAX_SGE);
> >  
> > -	rds_ibdev->odp_capable =
> > -		!!(device->attrs.kernel_cap_flags &
> > -		   IBK_ON_DEMAND_PAGING) &&
> > -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> > -		   IB_ODP_SUPPORT_WRITE) &&
> > -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> > -		   IB_ODP_SUPPORT_READ);
> 
> This patch seems to drop the check for WRITE and READ support on the
> ODP.

Right, and they are part of IBK_ON_DEMAND_PAGING support. All ODP
providers support both IB_ODP_SUPPORT_WRITE and IB_ODP_SUPPORT_READ.

RDS doesn't need to check more than general ODP support and can safely
rely on internal driver logic to create right MR.

Thanks

> 
> Jason
Jason Gunthorpe April 8, 2025, 12:38 p.m. UTC | #3 
On Tue, Apr 08, 2025 at 03:34:13PM +0300, Leon Romanovsky wrote:
> On Tue, Apr 08, 2025 at 09:23:38AM -0300, Jason Gunthorpe wrote:
> > On Tue, Apr 08, 2025 at 02:04:55PM +0300, Leon Romanovsky wrote:
> > > diff --git a/net/rds/ib.c b/net/rds/ib.c
> > > index 9826fe7f9d00..c62aa2ff4963 100644
> > > --- a/net/rds/ib.c
> > > +++ b/net/rds/ib.c
> > > @@ -153,14 +153,6 @@ static int rds_ib_add_one(struct ib_device *device)
> > >  	rds_ibdev->max_wrs = device->attrs.max_qp_wr;
> > >  	rds_ibdev->max_sge = min(device->attrs.max_send_sge, RDS_IB_MAX_SGE);
> > >  
> > > -	rds_ibdev->odp_capable =
> > > -		!!(device->attrs.kernel_cap_flags &
> > > -		   IBK_ON_DEMAND_PAGING) &&
> > > -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> > > -		   IB_ODP_SUPPORT_WRITE) &&
> > > -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> > > -		   IB_ODP_SUPPORT_READ);
> > 
> > This patch seems to drop the check for WRITE and READ support on the
> > ODP.
> 
> Right, and they are part of IBK_ON_DEMAND_PAGING support. All ODP
> providers support both IB_ODP_SUPPORT_WRITE and IB_ODP_SUPPORT_READ.

Where? mlx5 reads this from FW and I don't see anything blocking
IBK_ON_DEMAND_PAGING if the FW is weird.

Jason
Leon Romanovsky April 8, 2025, 7:11 p.m. UTC | #4 
On Tue, Apr 08, 2025 at 09:38:14AM -0300, Jason Gunthorpe wrote:
> On Tue, Apr 08, 2025 at 03:34:13PM +0300, Leon Romanovsky wrote:
> > On Tue, Apr 08, 2025 at 09:23:38AM -0300, Jason Gunthorpe wrote:
> > > On Tue, Apr 08, 2025 at 02:04:55PM +0300, Leon Romanovsky wrote:
> > > > diff --git a/net/rds/ib.c b/net/rds/ib.c
> > > > index 9826fe7f9d00..c62aa2ff4963 100644
> > > > --- a/net/rds/ib.c
> > > > +++ b/net/rds/ib.c
> > > > @@ -153,14 +153,6 @@ static int rds_ib_add_one(struct ib_device *device)
> > > >  	rds_ibdev->max_wrs = device->attrs.max_qp_wr;
> > > >  	rds_ibdev->max_sge = min(device->attrs.max_send_sge, RDS_IB_MAX_SGE);
> > > >  
> > > > -	rds_ibdev->odp_capable =
> > > > -		!!(device->attrs.kernel_cap_flags &
> > > > -		   IBK_ON_DEMAND_PAGING) &&
> > > > -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> > > > -		   IB_ODP_SUPPORT_WRITE) &&
> > > > -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> > > > -		   IB_ODP_SUPPORT_READ);
> > > 
> > > This patch seems to drop the check for WRITE and READ support on the
> > > ODP.
> > 
> > Right, and they are part of IBK_ON_DEMAND_PAGING support. All ODP
> > providers support both IB_ODP_SUPPORT_WRITE and IB_ODP_SUPPORT_READ.
> 
> Where? mlx5 reads this from FW and I don't see anything blocking
> IBK_ON_DEMAND_PAGING if the FW is weird.

As the one who added it, I can assure you that we added these checks not
because of weird FW, but because these caps existed.

RDS calls to ib_reg_user_mr() with the following access_flags.

  564                 int access_flags =
  565                         (IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
  566                          IB_ACCESS_REMOTE_WRITE | IB_ACCESS_REMOTE_ATOMIC |
  567                          IB_ACCESS_ON_DEMAND);
  <...>
  575
  576                 ib_mr = ib_reg_user_mr(rds_ibdev->pd, start, length, virt_addr,
  577                                        access_flags);

If for some reason ODP doesn't support WRITE and/or READ, ib_reg_user_mr() will return an error from FW,

Thanks


> 
> Jason
Allison Henderson April 9, 2025, 12:54 a.m. UTC | #5 
On Tue, 2025-04-08 at 22:11 +0300, Leon Romanovsky wrote:
> On Tue, Apr 08, 2025 at 09:38:14AM -0300, Jason Gunthorpe wrote:
> > On Tue, Apr 08, 2025 at 03:34:13PM +0300, Leon Romanovsky wrote:
> > > On Tue, Apr 08, 2025 at 09:23:38AM -0300, Jason Gunthorpe wrote:
> > > > On Tue, Apr 08, 2025 at 02:04:55PM +0300, Leon Romanovsky wrote:
> > > > > diff --git a/net/rds/ib.c b/net/rds/ib.c
> > > > > index 9826fe7f9d00..c62aa2ff4963 100644
> > > > > --- a/net/rds/ib.c
> > > > > +++ b/net/rds/ib.c
> > > > > @@ -153,14 +153,6 @@ static int rds_ib_add_one(struct ib_device *device)
> > > > >  	rds_ibdev->max_wrs = device->attrs.max_qp_wr;
> > > > >  	rds_ibdev->max_sge = min(device->attrs.max_send_sge, RDS_IB_MAX_SGE);
> > > > >  
> > > > > -	rds_ibdev->odp_capable =
> > > > > -		!!(device->attrs.kernel_cap_flags &
> > > > > -		   IBK_ON_DEMAND_PAGING) &&
> > > > > -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> > > > > -		   IB_ODP_SUPPORT_WRITE) &&
> > > > > -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> > > > > -		   IB_ODP_SUPPORT_READ);
> > > > 
> > > > This patch seems to drop the check for WRITE and READ support on the
> > > > ODP.
> > > 
> > > Right, and they are part of IBK_ON_DEMAND_PAGING support. All ODP
> > > providers support both IB_ODP_SUPPORT_WRITE and IB_ODP_SUPPORT_READ.
> > 
> > Where? mlx5 reads this from FW and I don't see anything blocking
> > IBK_ON_DEMAND_PAGING if the FW is weird.
> 
> As the one who added it, I can assure you that we added these checks not
> because of weird FW, but because these caps existed.
Hi Leon,

Thanks for the patch.  Is there a commit id for the FW checks we can see?  Maybe we can just add a little more detail to
the commit description to make clear where they are and what they're checking for.  Thank you!

Allison

> 
> RDS calls to ib_reg_user_mr() with the following access_flags.
> 
>   564                 int access_flags =
>   565                         (IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
>   566                          IB_ACCESS_REMOTE_WRITE | IB_ACCESS_REMOTE_ATOMIC |
>   567                          IB_ACCESS_ON_DEMAND);
>   <...>
>   575
>   576                 ib_mr = ib_reg_user_mr(rds_ibdev->pd, start, length, virt_addr,
>   577                                        access_flags);
> 
> If for some reason ODP doesn't support WRITE and/or READ, ib_reg_user_mr() will return an error from FW,
> 
> Thanks
> 
> 
> > 
> > Jason
Leon Romanovsky April 10, 2025, 11:35 a.m. UTC | #6 
On Wed, Apr 09, 2025 at 12:54:39AM +0000, Allison Henderson wrote:
> On Tue, 2025-04-08 at 22:11 +0300, Leon Romanovsky wrote:
> > On Tue, Apr 08, 2025 at 09:38:14AM -0300, Jason Gunthorpe wrote:
> > > On Tue, Apr 08, 2025 at 03:34:13PM +0300, Leon Romanovsky wrote:
> > > > On Tue, Apr 08, 2025 at 09:23:38AM -0300, Jason Gunthorpe wrote:
> > > > > On Tue, Apr 08, 2025 at 02:04:55PM +0300, Leon Romanovsky wrote:
> > > > > > diff --git a/net/rds/ib.c b/net/rds/ib.c
> > > > > > index 9826fe7f9d00..c62aa2ff4963 100644
> > > > > > --- a/net/rds/ib.c
> > > > > > +++ b/net/rds/ib.c
> > > > > > @@ -153,14 +153,6 @@ static int rds_ib_add_one(struct ib_device *device)
> > > > > >  	rds_ibdev->max_wrs = device->attrs.max_qp_wr;
> > > > > >  	rds_ibdev->max_sge = min(device->attrs.max_send_sge, RDS_IB_MAX_SGE);
> > > > > >  
> > > > > > -	rds_ibdev->odp_capable =
> > > > > > -		!!(device->attrs.kernel_cap_flags &
> > > > > > -		   IBK_ON_DEMAND_PAGING) &&
> > > > > > -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> > > > > > -		   IB_ODP_SUPPORT_WRITE) &&
> > > > > > -		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
> > > > > > -		   IB_ODP_SUPPORT_READ);
> > > > > 
> > > > > This patch seems to drop the check for WRITE and READ support on the
> > > > > ODP.
> > > > 
> > > > Right, and they are part of IBK_ON_DEMAND_PAGING support. All ODP
> > > > providers support both IB_ODP_SUPPORT_WRITE and IB_ODP_SUPPORT_READ.
> > > 
> > > Where? mlx5 reads this from FW and I don't see anything blocking
> > > IBK_ON_DEMAND_PAGING if the FW is weird.
> > 
> > As the one who added it, I can assure you that we added these checks not
> > because of weird FW, but because these caps existed.
> Hi Leon,
> 
> Thanks for the patch.  Is there a commit id for the FW checks we can see?

It is part of FW checks to provided access_flags. In this case, you are
asking for IB_ACCESS_REMOTE_READ and IB_ACCESS_ON_DEMAND.

The check of IB_ODP_SUPPORT_READ is used when you need to dig which
transport actually supports it.

The thing is that ODP was always supported for RC QPs, from day one.

> Maybe we can just add a little more detail to
> the commit description to make clear where they are and what they're checking for.  Thank you!

Sure, will update it.

Thanks

> 
> Allison
> 
> > 
> > RDS calls to ib_reg_user_mr() with the following access_flags.
> > 
> >   564                 int access_flags =
> >   565                         (IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
> >   566                          IB_ACCESS_REMOTE_WRITE | IB_ACCESS_REMOTE_ATOMIC |
> >   567                          IB_ACCESS_ON_DEMAND);
> >   <...>
> >   575
> >   576                 ib_mr = ib_reg_user_mr(rds_ibdev->pd, start, length, virt_addr,
> >   577                                        access_flags);
> > 
> > If for some reason ODP doesn't support WRITE and/or READ, ib_reg_user_mr() will return an error from FW,
> > 
> > Thanks
> > 
> > 
> > > 
> > > Jason
>
diff mbox series

Patch

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index c5e78bbefbd0..61620787ee48 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -2218,7 +2218,7 @@  struct ib_mr *ib_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
 		if (!(pd->device->attrs.kernel_cap_flags &
 		      IBK_ON_DEMAND_PAGING)) {
 			pr_debug("ODP support not available\n");
-			return ERR_PTR(-EINVAL);
+			return ERR_PTR(-EOPNOTSUPP);
 		}
 	}
 
diff --git a/net/rds/ib.c b/net/rds/ib.c
index 9826fe7f9d00..c62aa2ff4963 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -153,14 +153,6 @@  static int rds_ib_add_one(struct ib_device *device)
 	rds_ibdev->max_wrs = device->attrs.max_qp_wr;
 	rds_ibdev->max_sge = min(device->attrs.max_send_sge, RDS_IB_MAX_SGE);
 
-	rds_ibdev->odp_capable =
-		!!(device->attrs.kernel_cap_flags &
-		   IBK_ON_DEMAND_PAGING) &&
-		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
-		   IB_ODP_SUPPORT_WRITE) &&
-		!!(device->attrs.odp_caps.per_transport_caps.rc_odp_caps &
-		   IB_ODP_SUPPORT_READ);
-
 	rds_ibdev->max_1m_mrs = device->attrs.max_mr ?
 		min_t(unsigned int, (device->attrs.max_mr / 2),
 		      rds_ib_mr_1m_pool_size) : rds_ib_mr_1m_pool_size;
diff --git a/net/rds/ib.h b/net/rds/ib.h
index 8ef3178ed4d6..f3ec4ff5951f 100644
--- a/net/rds/ib.h
+++ b/net/rds/ib.h
@@ -246,7 +246,6 @@  struct rds_ib_device {
 	struct list_head	conn_list;
 	struct ib_device	*dev;
 	struct ib_pd		*pd;
-	u8			odp_capable:1;
 
 	unsigned int		max_mrs;
 	struct rds_ib_mr_pool	*mr_1m_pool;
diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
index d1cfceeff133..75ab7b8db864 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -568,11 +568,6 @@  void *rds_ib_get_mr(struct scatterlist *sg, unsigned long nents,
 		struct ib_sge sge = {};
 		struct ib_mr *ib_mr;
 
-		if (!rds_ibdev->odp_capable) {
-			ret = -EOPNOTSUPP;
-			goto out;
-		}
-
 		ib_mr = ib_reg_user_mr(rds_ibdev->pd, start, length, virt_addr,
 				       access_flags);