From: Leon Romanovsky <leon@kernel.org> Date: 2018-12-20 09:23:35
From: Leon Romanovsky <redacted>
Hi,
As a followup to Jason's request to rethink CONFIG_INFINIBAND_ON_DEMAND_PAGING
usage, this series cleans mlx5_ib and RDMA/core code and it is based on already
sent but not yet accepted patch https://patchwork.kernel.org/patch/10735547/
It is under extensive testing now, but I wanted to raise awareness as soon
as possible for the patch "RDMA/core: Don't depend device ODP capabilities
on kconfig option", which changes behavior for mlx5 devices with
CONFIG_INFINIBAND_ON_DEMAND_PAGING set to no.
Thanks
Leon Romanovsky (5):
RDMA: Clean structures from CONFIG_INFINIBAND_ON_DEMAND_PAGING
RDMA/core: Don't depend device ODP capabilities on kconfig option
RDMA/mlx5: Introduce and reuse helper to identify ODP MR
RDMA/mlx5: Embed into the code flow the ODP config option
RDMA/mlx5: Delete declaration of already removed function
drivers/infiniband/core/uverbs_cmd.c | 8 ++---
drivers/infiniband/hw/mlx5/main.c | 37 ++++++++++-------------
drivers/infiniband/hw/mlx5/mem.c | 5 +---
drivers/infiniband/hw/mlx5/mlx5_ib.h | 12 ++++++--
drivers/infiniband/hw/mlx5/mr.c | 45 ++++++++--------------------
drivers/infiniband/hw/mlx5/odp.c | 6 ++--
include/linux/mlx5/driver.h | 4 ---
include/rdma/ib_umem_odp.h | 26 ++++++++--------
include/rdma/ib_verbs.h | 2 --
9 files changed, 58 insertions(+), 87 deletions(-)
From: Leon Romanovsky <leon@kernel.org> Date: 2018-12-20 09:23:46
From: Leon Romanovsky <redacted>
Device capability bits are exposing what specific device supports from
HW perspective. Those bits are not dependent on kernel configurations
and RDMA/core should ensure that proper interfaces to users will be
disabled if CONFIG_INFINIBAND_ON_DEMAND_PAGING is not set.
Fixes: f4056bfd8ccf ("IB/core: Add on demand paging caps to ib_uverbs_ex_query_device")
Fixes: 8cdd312cfed7 ("IB/mlx5: Implement the ODP capability query verb")
Signed-off-by: Leon Romanovsky <redacted>
---
drivers/infiniband/core/uverbs_cmd.c | 2 --
drivers/infiniband/hw/mlx5/main.c | 2 --
2 files changed, 4 deletions(-)
From: Leon Romanovsky <leon@kernel.org> Date: 2018-12-20 09:23:50
From: Leon Romanovsky <redacted>
Consolidate various checks if MR is ODP backed to one simple
helper and update call sites to use it.
Signed-off-by: Leon Romanovsky <redacted>
---
drivers/infiniband/hw/mlx5/mlx5_ib.h | 10 ++++++++++
drivers/infiniband/hw/mlx5/mr.c | 29 ++++++----------------------
drivers/infiniband/hw/mlx5/odp.c | 6 +++---
3 files changed, 19 insertions(+), 26 deletions(-)
@@ -71,11 +71,9 @@ static int destroy_mkey(struct mlx5_ib_dev *dev, struct mlx5_ib_mr *mr){interr=mlx5_core_destroy_mkey(dev->mdev,&mr->mmkey);-#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING/* Wait until all page fault handlers using the mr complete. */-if(mr->umem&&mr->umem->is_odp)+if(is_odp_mr(mr))synchronize_srcu(&dev->mr_srcu);-#endifreturnerr;}
@@ -1895,12 +1893,10 @@ static int mlx5_ib_dealloc_ucontext(struct ib_ucontext *ibcontext)structmlx5_ib_dev*dev=to_mdev(ibcontext->device);structmlx5_bfreg_info*bfregi;-#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING/* All umem's must be destroyed before destroying the ucontext. */mutex_lock(&ibcontext->per_mm_list_lock);WARN_ON(!list_empty(&ibcontext->per_mm_list));mutex_unlock(&ibcontext->per_mm_list_lock);-#endifbfregi=&context->bfregi;mlx5_ib_dealloc_transport_domain(dev,context->tdn,context->devx_uid);
From: Leon Romanovsky <leon@kernel.org> Date: 2018-12-20 09:23:56
From: Leon Romanovsky <redacted>
The implementation of mlx5_core_page_fault_resume() was removed
in commit d5d284b829a6 ("{net,IB}/mlx5: Move Page fault EQ and
ODP logic to RDMA"). This patch removes declaration too.
Fixes: d5d284b829a6 ("{net,IB}/mlx5: Move Page fault EQ and ODP logic to RDMA")
Signed-off-by: Leon Romanovsky <redacted>
---
include/linux/mlx5/driver.h | 4 ----
1 file changed, 4 deletions(-)
From: Leon Romanovsky <leon@kernel.org> Date: 2018-12-20 09:23:59
From: Leon Romanovsky <redacted>
CONFIG_INFINIBAND_ON_DEMAND_PAGING is used in general structures
to micro-optimize the memory footprint. Remove it, so it will allow
us to simplify various ODP device flows.
Signed-off-by: Leon Romanovsky <redacted>
---
drivers/infiniband/hw/mlx5/mlx5_ib.h | 2 --
include/rdma/ib_verbs.h | 2 --
2 files changed, 4 deletions(-)
From: Jason Gunthorpe <hidden> Date: 2018-12-20 17:20:58
On Thu, Dec 20, 2018 at 11:23:15AM +0200, Leon Romanovsky wrote:
quoted hunk
From: Leon Romanovsky <redacted>
Device capability bits are exposing what specific device supports from
HW perspective. Those bits are not dependent on kernel configurations
and RDMA/core should ensure that proper interfaces to users will be
disabled if CONFIG_INFINIBAND_ON_DEMAND_PAGING is not set.
Fixes: f4056bfd8ccf ("IB/core: Add on demand paging caps to ib_uverbs_ex_query_device")
Fixes: 8cdd312cfed7 ("IB/mlx5: Implement the ODP capability query verb")
Signed-off-by: Leon Romanovsky <redacted>
drivers/infiniband/core/uverbs_cmd.c | 2 --
drivers/infiniband/hw/mlx5/main.c | 2 --
2 files changed, 4 deletions(-)
From: Leon Romanovsky <leon@kernel.org> Date: 2018-12-20 17:30:02
On Thu, Dec 20, 2018 at 05:20:51PM +0000, Jason Gunthorpe wrote:
On Thu, Dec 20, 2018 at 11:23:15AM +0200, Leon Romanovsky wrote:
quoted
From: Leon Romanovsky <redacted>
Device capability bits are exposing what specific device supports from
HW perspective. Those bits are not dependent on kernel configurations
and RDMA/core should ensure that proper interfaces to users will be
disabled if CONFIG_INFINIBAND_ON_DEMAND_PAGING is not set.
Fixes: f4056bfd8ccf ("IB/core: Add on demand paging caps to ib_uverbs_ex_query_device")
Fixes: 8cdd312cfed7 ("IB/mlx5: Implement the ODP capability query verb")
Signed-off-by: Leon Romanovsky <redacted>
drivers/infiniband/core/uverbs_cmd.c | 2 --
drivers/infiniband/hw/mlx5/main.c | 2 --
2 files changed, 4 deletions(-)
@@ -923,11 +923,9 @@ static int mlx5_ib_query_device(struct ib_device *ibdev,props->hca_core_clock=MLX5_CAP_GEN(mdev,device_frequency_khz);props->timestamp_mask=0x7FFFFFFFFFFFFFFFULL;-#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGINGif(MLX5_CAP_GEN(mdev,pg))props->device_cap_flags|=IB_DEVICE_ON_DEMAND_PAGING;props->odp_caps=dev->odp_caps;-#endif
But shouldn't this be protected? If the driver has compiled out ODP it
shouldn't set the cap flag...
I see those capabilities as device properties and not as kernel ones.
Current situation looks bad for me, when I have same device which
reports differently information depends on some compilation flag.
If driver was compiled without CONFIG_INFINIBAND_ON_DEMAND_PAGING, the
kernel core code should be compiled without this config set too and core
code wouldn't pass any ODP related verbs/flags.
Thanks
From: Jason Gunthorpe <hidden> Date: 2018-12-20 17:33:16
On Thu, Dec 20, 2018 at 07:29:56PM +0200, Leon Romanovsky wrote:
On Thu, Dec 20, 2018 at 05:20:51PM +0000, Jason Gunthorpe wrote:
quoted
On Thu, Dec 20, 2018 at 11:23:15AM +0200, Leon Romanovsky wrote:
quoted
From: Leon Romanovsky <redacted>
Device capability bits are exposing what specific device supports from
HW perspective. Those bits are not dependent on kernel configurations
and RDMA/core should ensure that proper interfaces to users will be
disabled if CONFIG_INFINIBAND_ON_DEMAND_PAGING is not set.
Fixes: f4056bfd8ccf ("IB/core: Add on demand paging caps to ib_uverbs_ex_query_device")
Fixes: 8cdd312cfed7 ("IB/mlx5: Implement the ODP capability query verb")
Signed-off-by: Leon Romanovsky <redacted>
drivers/infiniband/core/uverbs_cmd.c | 2 --
drivers/infiniband/hw/mlx5/main.c | 2 --
2 files changed, 4 deletions(-)
@@ -923,11 +923,9 @@ static int mlx5_ib_query_device(struct ib_device *ibdev,props->hca_core_clock=MLX5_CAP_GEN(mdev,device_frequency_khz);props->timestamp_mask=0x7FFFFFFFFFFFFFFFULL;-#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGINGif(MLX5_CAP_GEN(mdev,pg))props->device_cap_flags|=IB_DEVICE_ON_DEMAND_PAGING;props->odp_caps=dev->odp_caps;-#endif
But shouldn't this be protected? If the driver has compiled out ODP it
shouldn't set the cap flag...
I see those capabilities as device properties and not as kernel ones.
Current situation looks bad for me, when I have same device which
reports differently information depends on some compilation flag.
It is not device capabilities, it is a kernel API capability if the
API is not available the bit should not be set.
The field is badly named.
Jason
From: Leon Romanovsky <leon@kernel.org> Date: 2018-12-20 17:50:27
On Thu, Dec 20, 2018 at 05:33:11PM +0000, Jason Gunthorpe wrote:
On Thu, Dec 20, 2018 at 07:29:56PM +0200, Leon Romanovsky wrote:
quoted
On Thu, Dec 20, 2018 at 05:20:51PM +0000, Jason Gunthorpe wrote:
quoted
On Thu, Dec 20, 2018 at 11:23:15AM +0200, Leon Romanovsky wrote:
quoted
From: Leon Romanovsky <redacted>
Device capability bits are exposing what specific device supports from
HW perspective. Those bits are not dependent on kernel configurations
and RDMA/core should ensure that proper interfaces to users will be
disabled if CONFIG_INFINIBAND_ON_DEMAND_PAGING is not set.
Fixes: f4056bfd8ccf ("IB/core: Add on demand paging caps to ib_uverbs_ex_query_device")
Fixes: 8cdd312cfed7 ("IB/mlx5: Implement the ODP capability query verb")
Signed-off-by: Leon Romanovsky <redacted>
drivers/infiniband/core/uverbs_cmd.c | 2 --
drivers/infiniband/hw/mlx5/main.c | 2 --
2 files changed, 4 deletions(-)
@@ -923,11 +923,9 @@ static int mlx5_ib_query_device(struct ib_device *ibdev,props->hca_core_clock=MLX5_CAP_GEN(mdev,device_frequency_khz);props->timestamp_mask=0x7FFFFFFFFFFFFFFFULL;-#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGINGif(MLX5_CAP_GEN(mdev,pg))props->device_cap_flags|=IB_DEVICE_ON_DEMAND_PAGING;props->odp_caps=dev->odp_caps;-#endif
But shouldn't this be protected? If the driver has compiled out ODP it
shouldn't set the cap flag...
I see those capabilities as device properties and not as kernel ones.
Current situation looks bad for me, when I have same device which
reports differently information depends on some compilation flag.
It is not device capabilities, it is a kernel API capability if the
API is not available the bit should not be set.
Is it better? Should I resend the series?
diff --git a/drivers/infiniband/hw/mlx5/main.c
b/drivers/infiniband/hw/mlx5/main.c
index 0b68795fb04e..c95c2f545b8f 100644
But shouldn't this be protected? If the driver has compiled out ODP it
shouldn't set the cap flag...
I see those capabilities as device properties and not as kernel ones.
Current situation looks bad for me, when I have same device which
reports differently information depends on some compilation flag.
It is not device capabilities, it is a kernel API capability if the
API is not available the bit should not be set.
Is it better? Should I resend the series?
Hrm.. I think I'm inclined to keep the the if() in the same place as
the ifdefs. We can revise it later.
I noticed other busted up stuff:
- IB_DEVICE_ON_DEMAND_PAGING is apparently UAPI but not in a uapi
header (grr)
- ucontext shouldn't have a driver callback, that should be moved to
ops (ie to mlx5_ib_dev_odp_ops)
- IB_DEVICE_ON_DEMAND_PAGING should be set in core code based
on ops.invalidate_range being present, drivers shouldn't set it..
Jason
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2018-12-21 03:32:37
On Thu, Dec 20, 2018 at 11:23:13AM +0200, Leon Romanovsky wrote:
From: Leon Romanovsky <redacted>
Hi,
As a followup to Jason's request to rethink CONFIG_INFINIBAND_ON_DEMAND_PAGING
usage, this series cleans mlx5_ib and RDMA/core code and it is based on already
sent but not yet accepted patch https://patchwork.kernel.org/patch/10735547/
It is under extensive testing now, but I wanted to raise awareness as soon
as possible for the patch "RDMA/core: Don't depend device ODP capabilities
on kconfig option", which changes behavior for mlx5 devices with
CONFIG_INFINIBAND_ON_DEMAND_PAGING set to no.
Thanks
Leon Romanovsky (5):
RDMA: Clean structures from CONFIG_INFINIBAND_ON_DEMAND_PAGING
RDMA/core: Don't depend device ODP capabilities on kconfig option
RDMA/mlx5: Introduce and reuse helper to identify ODP MR
RDMA/mlx5: Embed into the code flow the ODP config option
RDMA/mlx5: Delete declaration of already removed function
I'm imagining something like this integrated into these patches, what
do you think?
From: Leon Romanovsky <leon@kernel.org> Date: 2018-12-21 14:00:00
On Thu, Dec 20, 2018 at 08:32:35PM -0700, Jason Gunthorpe wrote:
On Thu, Dec 20, 2018 at 11:23:13AM +0200, Leon Romanovsky wrote:
quoted
From: Leon Romanovsky <redacted>
Hi,
As a followup to Jason's request to rethink CONFIG_INFINIBAND_ON_DEMAND_PAGING
usage, this series cleans mlx5_ib and RDMA/core code and it is based on already
sent but not yet accepted patch https://patchwork.kernel.org/patch/10735547/
It is under extensive testing now, but I wanted to raise awareness as soon
as possible for the patch "RDMA/core: Don't depend device ODP capabilities
on kconfig option", which changes behavior for mlx5 devices with
CONFIG_INFINIBAND_ON_DEMAND_PAGING set to no.
Thanks
Leon Romanovsky (5):
RDMA: Clean structures from CONFIG_INFINIBAND_ON_DEMAND_PAGING
RDMA/core: Don't depend device ODP capabilities on kconfig option
RDMA/mlx5: Introduce and reuse helper to identify ODP MR
RDMA/mlx5: Embed into the code flow the ODP config option
RDMA/mlx5: Delete declaration of already removed function
I'm imagining something like this integrated into these patches, what
do you think?
My expectation that we won't be in this state because it is too far away
from entry where we could check and prevent unsupported access.
uverbs entry point -> driver code -> ib_umem_get
^^^^ this is better place to check right flags.
@@ -923,9 +923,11 @@ static int mlx5_ib_query_device(struct ib_device *ibdev,props->hca_core_clock=MLX5_CAP_GEN(mdev,device_frequency_khz);props->timestamp_mask=0x7FFFFFFFFFFFFFFFULL;-if(MLX5_CAP_GEN(mdev,pg))-props->device_cap_flags|=IB_DEVICE_ON_DEMAND_PAGING;-props->odp_caps=dev->odp_caps;+if(IS_ENABLED(CONFIG_INFINIBAND_ON_DEMAND_PAGING)){+if(MLX5_CAP_GEN(mdev,pg))+props->device_cap_flags|=IB_DEVICE_ON_DEMAND_PAGING;+props->odp_caps=dev->odp_caps;+}
I accepted your claim about odp_caps being SW properties, but why did
you place device_cap_flags under CONFIG_INFINIBAND_ON_DEMAND_PAGING?
Especially when it is set based on HW capability.
quoted hunk
if (MLX5_CAP_GEN(mdev, cd))
props->device_cap_flags |= IB_DEVICE_CROSS_CHANNEL;
How can we have is_odp == True and CONFIG_INFINIBAND_ON_DEMAND_PAGING = n?
mlx5 code expects that if CONFIG_INFINIBAND_ON_DEMAND_PAGING is not set,
all occurrences of is_odp are false.
I tried to preserve previous behavior and that piece of code was simply
skipped if CONFIG_INFINIBAND_ON_DEMAND_PAGING is not set. You will
return -EOPNOTSUPP in new code. It can be right, it can be wrong, but
that change should be standalone.
mr = mlx5_ib_alloc_implicit_mr(to_mpd(pd), access_flags);
if (IS_ERR(mr))
return ERR_CAST(mr);
My expectation that we won't be in this state because it is too far away
from entry where we could check and prevent unsupported access.
uverbs entry point -> driver code -> ib_umem_get
^^^^ this is better place to check right flags.
I view umem as the proper core entry point here. We should never
allow to create an ODP umem without driver support.
@@ -923,9 +923,11 @@ static int mlx5_ib_query_device(struct ib_device *ibdev,props->hca_core_clock=MLX5_CAP_GEN(mdev,device_frequency_khz);props->timestamp_mask=0x7FFFFFFFFFFFFFFFULL;-if(MLX5_CAP_GEN(mdev,pg))-props->device_cap_flags|=IB_DEVICE_ON_DEMAND_PAGING;-props->odp_caps=dev->odp_caps;+if(IS_ENABLED(CONFIG_INFINIBAND_ON_DEMAND_PAGING)){+if(MLX5_CAP_GEN(mdev,pg))+props->device_cap_flags|=IB_DEVICE_ON_DEMAND_PAGING;+props->odp_caps=dev->odp_caps;+}
I accepted your claim about odp_caps being SW properties, but why did
you place device_cap_flags under CONFIG_INFINIBAND_ON_DEMAND_PAGING?
Especially when it is set based on HW capability.
My claim was about device_cap_flags, it indicates the SW capability
for ODP, it has nothing to do with HW.
quoted
if (MLX5_CAP_GEN(mdev, cd))
props->device_cap_flags |= IB_DEVICE_CROSS_CHANNEL;
We are not supposed to call to invalidate_range() if umem is not ODP.
It means that the below "if" is redundant.
I want to see all function pointers the driver does not implement set
to NULL... I suppose this needs to have an IS_ENABLED too since
mlx5_ib_invalidate_range is in odp.c
How can we have is_odp == True and CONFIG_INFINIBAND_ON_DEMAND_PAGING = n?
mlx5 code expects that if CONFIG_INFINIBAND_ON_DEMAND_PAGING is not set,
all occurrences of is_odp are false.
Whenver you see the IS_ENABLED it is just size optimizing code. The
compiler can't know that is_odp == FALSE, so the extra check is
needed.
I tried to preserve previous behavior and that piece of code was simply
skipped if CONFIG_INFINIBAND_ON_DEMAND_PAGING is not set. You will
return -EOPNOTSUPP in new code. It can be right, it can be wrong, but
that change should be standalone.
Sure - it is a bug that it checks arguments differently depending on
compile options.
Jason
My expectation that we won't be in this state because it is too far away
from entry where we could check and prevent unsupported access.
uverbs entry point -> driver code -> ib_umem_get
^^^^ this is better place to check right flags.
I view umem as the proper core entry point here. We should never
allow to create an ODP umem without driver support.
And drivers should never ask ODP umem if they don't support it.
You are requesting to add by definition unreachable code.
"attr" is initialized to zero, there is no need to place those odp_caps under "if",
It is not zero, it is the result of query_device - and broken drivers
should not be allowed to put non-zero values here..
Do you have real example in mind? Because if we have such broken drivers
in subsystem, we would see it already, because ODP config is enabled by
default in many distributions.
@@ -923,9 +923,11 @@ static int mlx5_ib_query_device(struct ib_device *ibdev,props->hca_core_clock=MLX5_CAP_GEN(mdev,device_frequency_khz);props->timestamp_mask=0x7FFFFFFFFFFFFFFFULL;-if(MLX5_CAP_GEN(mdev,pg))-props->device_cap_flags|=IB_DEVICE_ON_DEMAND_PAGING;-props->odp_caps=dev->odp_caps;+if(IS_ENABLED(CONFIG_INFINIBAND_ON_DEMAND_PAGING)){+if(MLX5_CAP_GEN(mdev,pg))+props->device_cap_flags|=IB_DEVICE_ON_DEMAND_PAGING;+props->odp_caps=dev->odp_caps;+}
I accepted your claim about odp_caps being SW properties, but why did
you place device_cap_flags under CONFIG_INFINIBAND_ON_DEMAND_PAGING?
Especially when it is set based on HW capability.
My claim was about device_cap_flags, it indicates the SW capability
for ODP, it has nothing to do with HW.
ODP doesn't exist without HW support.
quoted
quoted
if (MLX5_CAP_GEN(mdev, cd))
props->device_cap_flags |= IB_DEVICE_CROSS_CHANNEL;
We are not supposed to call to invalidate_range() if umem is not ODP.
It means that the below "if" is redundant.
I want to see all function pointers the driver does not implement set
to NULL... I suppose this needs to have an IS_ENABLED too since
mlx5_ib_invalidate_range is in odp.c
How can we have is_odp == True and CONFIG_INFINIBAND_ON_DEMAND_PAGING = n?
mlx5 code expects that if CONFIG_INFINIBAND_ON_DEMAND_PAGING is not set,
all occurrences of is_odp are false.
Whenver you see the IS_ENABLED it is just size optimizing code. The
compiler can't know that is_odp == FALSE, so the extra check is
needed.a
It is always correct, but why is it important in this path and why
is_odp being in processor cache wouldn't enough? IMHO, you are saving
first cache miss only.
I tried to preserve previous behavior and that piece of code was simply
skipped if CONFIG_INFINIBAND_ON_DEMAND_PAGING is not set. You will
return -EOPNOTSUPP in new code. It can be right, it can be wrong, but
that change should be standalone.
Sure - it is a bug that it checks arguments differently depending on
compile options.
I'll take a look on it later and will send followup if needed.
Regarding rest of your comments, how will we converge?
I disagree with most of your points and believe that you are proposing
to fix non-existing problems with dead code. If you insist, I'll add
your code as long as it helps us to progress here, should I.
Thanks