* [PATCH mlx5-next 1/4] net/mlx5: Expose bits for querying special mkeys
2023-01-04 8:11 [PATCH rdma-next 0/4] Rely on firmware to get special mkeys Leon Romanovsky
@ 2023-01-04 8:11 ` Leon Romanovsky
2023-01-04 8:11 ` [PATCH mlx5-next 2/4] net/mlx5: Change define name for 0x100 lkey value Leon Romanovsky
` (2 subsequent siblings)
3 siblings, 0 replies; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04 8:11 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Or Har-Toov, Eric Dumazet, Jakub Kicinski, linux-rdma,
Michael Guralnik, netdev, Paolo Abeni, Saeed Mahameed
From: Or Har-Toov <ohartoov@nvidia.com>
Add needed HW bits to query the values of all special mkeys.
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
include/linux/mlx5/mlx5_ifc.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index f3d1c62c98dd..a2ed927c8f9f 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -1479,7 +1479,9 @@ struct mlx5_ifc_cmd_hca_cap_bits {
u8 relaxed_ordering_write[0x1];
u8 relaxed_ordering_read[0x1];
u8 log_max_mkey[0x6];
- u8 reserved_at_f0[0x8];
+ u8 reserved_at_f0[0x6];
+ u8 terminate_scatter_list_mkey[0x1];
+ u8 repeated_mkey[0x1];
u8 dump_fill_mkey[0x1];
u8 reserved_at_f9[0x2];
u8 fast_teardown[0x1];
@@ -5197,7 +5199,11 @@ struct mlx5_ifc_query_special_contexts_out_bits {
u8 null_mkey[0x20];
- u8 reserved_at_a0[0x60];
+ u8 terminate_scatter_list_mkey[0x20];
+
+ u8 repeated_mkey[0x20];
+
+ u8 reserved_at_a0[0x20];
};
struct mlx5_ifc_query_special_contexts_in_bits {
--
2.38.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH mlx5-next 2/4] net/mlx5: Change define name for 0x100 lkey value
2023-01-04 8:11 [PATCH rdma-next 0/4] Rely on firmware to get special mkeys Leon Romanovsky
2023-01-04 8:11 ` [PATCH mlx5-next 1/4] net/mlx5: Expose bits for querying " Leon Romanovsky
@ 2023-01-04 8:11 ` Leon Romanovsky
2023-01-04 8:11 ` [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys Leon Romanovsky
2023-01-04 8:11 ` [PATCH rdma-next 4/4] RDMA/mlx5: " Leon Romanovsky
3 siblings, 0 replies; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04 8:11 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Or Har-Toov, Eric Dumazet, Jakub Kicinski, linux-rdma,
Michael Guralnik, netdev, Paolo Abeni, Saeed Mahameed
From: Or Har-Toov <ohartoov@nvidia.com>
Change define of 0x100 lkey value from MLX5_INVALID_LKEY to be
MLX5_TERMINATE_SCATTER_LIST_LKEY as 0x100 is the value of
terminate_scatter_list_mkey.
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/infiniband/hw/mlx5/odp.c | 10 +++++-----
drivers/infiniband/hw/mlx5/srq.c | 2 +-
drivers/infiniband/hw/mlx5/wr.c | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 ++-
include/linux/mlx5/qp.h | 2 +-
5 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
index e6e021af6aa9..b4ebeadce67c 100644
--- a/drivers/infiniband/hw/mlx5/odp.c
+++ b/drivers/infiniband/hw/mlx5/odp.c
@@ -986,7 +986,7 @@ static int pagefault_data_segments(struct mlx5_ib_dev *dev,
{
int ret = 0, npages = 0;
u64 io_virt;
- u32 key;
+ __be32 key;
u32 byte_count;
size_t bcnt;
int inline_segment;
@@ -1000,7 +1000,7 @@ static int pagefault_data_segments(struct mlx5_ib_dev *dev,
struct mlx5_wqe_data_seg *dseg = wqe;
io_virt = be64_to_cpu(dseg->addr);
- key = be32_to_cpu(dseg->lkey);
+ key = dseg->lkey;
byte_count = be32_to_cpu(dseg->byte_count);
inline_segment = !!(byte_count & MLX5_INLINE_SEG);
bcnt = byte_count & ~MLX5_INLINE_SEG;
@@ -1014,8 +1014,8 @@ static int pagefault_data_segments(struct mlx5_ib_dev *dev,
}
/* receive WQE end of sg list. */
- if (receive_queue && bcnt == 0 && key == MLX5_INVALID_LKEY &&
- io_virt == 0)
+ if (receive_queue && bcnt == 0 &&
+ key == MLX5_TERMINATE_SCATTER_LIST_LKEY && io_virt == 0)
break;
if (!inline_segment && total_wqe_bytes) {
@@ -1034,7 +1034,7 @@ static int pagefault_data_segments(struct mlx5_ib_dev *dev,
continue;
}
- ret = pagefault_single_data_segment(dev, NULL, key,
+ ret = pagefault_single_data_segment(dev, NULL, be32_to_cpu(key),
io_virt, bcnt,
&pfault->bytes_committed,
bytes_mapped);
diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
index 757756c50cc6..bcceb14a07f9 100644
--- a/drivers/infiniband/hw/mlx5/srq.c
+++ b/drivers/infiniband/hw/mlx5/srq.c
@@ -447,7 +447,7 @@ int mlx5_ib_post_srq_recv(struct ib_srq *ibsrq, const struct ib_recv_wr *wr,
if (i < srq->msrq.max_avail_gather) {
scat[i].byte_count = 0;
- scat[i].lkey = cpu_to_be32(MLX5_INVALID_LKEY);
+ scat[i].lkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
scat[i].addr = 0;
}
}
diff --git a/drivers/infiniband/hw/mlx5/wr.c b/drivers/infiniband/hw/mlx5/wr.c
index 855f3f4fefad..bc44551493e2 100644
--- a/drivers/infiniband/hw/mlx5/wr.c
+++ b/drivers/infiniband/hw/mlx5/wr.c
@@ -1252,7 +1252,7 @@ int mlx5_ib_post_recv(struct ib_qp *ibqp, const struct ib_recv_wr *wr,
if (i < qp->rq.max_gs) {
scat[i].byte_count = 0;
- scat[i].lkey = cpu_to_be32(MLX5_INVALID_LKEY);
+ scat[i].lkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
scat[i].addr = 0;
}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 8d36e2de53a9..c76f15505a76 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -825,7 +825,8 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
/* check if num_frags is not a pow of two */
if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
wqe->data[f].byte_count = 0;
- wqe->data[f].lkey = cpu_to_be32(MLX5_INVALID_LKEY);
+ wqe->data[f].lkey =
+ MLX5_TERMINATE_SCATTER_LIST_LKEY;
wqe->data[f].addr = 0;
}
}
diff --git a/include/linux/mlx5/qp.h b/include/linux/mlx5/qp.h
index 4657d5c54abe..df55fbb65717 100644
--- a/include/linux/mlx5/qp.h
+++ b/include/linux/mlx5/qp.h
@@ -36,7 +36,7 @@
#include <linux/mlx5/device.h>
#include <linux/mlx5/driver.h>
-#define MLX5_INVALID_LKEY 0x100
+#define MLX5_TERMINATE_SCATTER_LIST_LKEY cpu_to_be32(0x100)
/* UMR (3 WQE_BB's) + SIG (3 WQE_BB's) + PSV (mem) + PSV (wire) */
#define MLX5_SIG_WQE_SIZE (MLX5_SEND_WQE_BB * 8)
#define MLX5_DIF_SIZE 8
--
2.38.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys
2023-01-04 8:11 [PATCH rdma-next 0/4] Rely on firmware to get special mkeys Leon Romanovsky
2023-01-04 8:11 ` [PATCH mlx5-next 1/4] net/mlx5: Expose bits for querying " Leon Romanovsky
2023-01-04 8:11 ` [PATCH mlx5-next 2/4] net/mlx5: Change define name for 0x100 lkey value Leon Romanovsky
@ 2023-01-04 8:11 ` Leon Romanovsky
2023-01-06 0:58 ` Saeed Mahameed
2023-01-04 8:11 ` [PATCH rdma-next 4/4] RDMA/mlx5: " Leon Romanovsky
3 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04 8:11 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Or Har-Toov, Eric Dumazet, Jakub Kicinski, linux-rdma,
Michael Guralnik, netdev, Paolo Abeni, Saeed Mahameed
From: Or Har-Toov <ohartoov@nvidia.com>
Using query_sepcial_contexts in order to get the correct value of
terminate_scatter_list_mkey, as FW will change it in some configurations.
This is done one time when the device is loading and the value is being
saved in the device context.
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
.../net/ethernet/mellanox/mlx5/core/en_main.c | 2 +-
.../net/ethernet/mellanox/mlx5/core/main.c | 27 +++++++++++++++++++
include/linux/mlx5/driver.h | 1 +
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index c76f15505a76..33d7a7095988 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -826,7 +826,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
wqe->data[f].byte_count = 0;
wqe->data[f].lkey =
- MLX5_TERMINATE_SCATTER_LIST_LKEY;
+ mdev->terminate_scatter_list_mkey;
wqe->data[f].addr = 0;
}
}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 7f5db13e3550..d39d758744a0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1221,6 +1221,28 @@ static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot)
return 0;
}
+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+ u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
+ u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
+ int err;
+
+ MLX5_SET(query_special_contexts_in, in, opcode,
+ MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
+ err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
+ if (err)
+ return err;
+
+ if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
+ dev->terminate_scatter_list_mkey =
+ cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+ terminate_scatter_list_mkey));
+ return 0;
+ }
+ dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
+ return 0;
+}
+
static int mlx5_load(struct mlx5_core_dev *dev)
{
int err;
@@ -1235,6 +1257,11 @@ static int mlx5_load(struct mlx5_core_dev *dev)
mlx5_events_start(dev);
mlx5_pagealloc_start(dev);
+ err = mlx5_get_terminate_scatter_list_mkey(dev);
+ if (err) {
+ mlx5_core_err(dev, "Failed to query special contexts\n");
+ goto err_irq_table;
+ }
err = mlx5_irq_table_create(dev);
if (err) {
mlx5_core_err(dev, "Failed to alloc IRQs\n");
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index d476255c9a3f..5f2c4038d638 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -801,6 +801,7 @@ struct mlx5_core_dev {
struct mlx5_rsc_dump *rsc_dump;
u32 vsc_addr;
struct mlx5_hv_vhca *hv_vhca;
+ __be32 terminate_scatter_list_mkey;
};
struct mlx5_db {
--
2.38.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys
2023-01-04 8:11 ` [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys Leon Romanovsky
@ 2023-01-06 0:58 ` Saeed Mahameed
2023-01-08 10:32 ` Leon Romanovsky
0 siblings, 1 reply; 18+ messages in thread
From: Saeed Mahameed @ 2023-01-06 0:58 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Jason Gunthorpe, Or Har-Toov, Eric Dumazet, Jakub Kicinski,
linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
Saeed Mahameed
On 04 Jan 10:11, Leon Romanovsky wrote:
>From: Or Har-Toov <ohartoov@nvidia.com>
>
>Using query_sepcial_contexts in order to get the correct value of
>terminate_scatter_list_mkey, as FW will change it in some configurations.
>This is done one time when the device is loading and the value is being
>saved in the device context.
>
>Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
>Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
>Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>---
> .../net/ethernet/mellanox/mlx5/core/en_main.c | 2 +-
> .../net/ethernet/mellanox/mlx5/core/main.c | 27 +++++++++++++++++++
> include/linux/mlx5/driver.h | 1 +
> 3 files changed, 29 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>index c76f15505a76..33d7a7095988 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>@@ -826,7 +826,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
> if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
> wqe->data[f].byte_count = 0;
> wqe->data[f].lkey =
>- MLX5_TERMINATE_SCATTER_LIST_LKEY;
>+ mdev->terminate_scatter_list_mkey;
> wqe->data[f].addr = 0;
> }
> }
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>index 7f5db13e3550..d39d758744a0 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>@@ -1221,6 +1221,28 @@ static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot)
> return 0;
> }
>
>+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
>+{
>+ u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
>+ u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
>+ int err;
>+
>+ MLX5_SET(query_special_contexts_in, in, opcode,
>+ MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
>+ err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
>+ if (err)
>+ return err;
>+
>+ if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
Why did you execute the command unconditionally if the output is only read
conditionally?
early exit before executing the command, older FW might fail and driver will
be unusable ..
>+ dev->terminate_scatter_list_mkey =
>+ cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>+ terminate_scatter_list_mkey));
>+ return 0;
>+ }
>+ dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
Another concern, what happens with old driver that is using the hardcoded
value with newer fw ? will it fail ? will it be accepted ?
The commit message doesn't explain what's going on here very well,
Let's discuss internally before you submit V2.
>+ return 0;
>+}
>+
> static int mlx5_load(struct mlx5_core_dev *dev)
> {
> int err;
>@@ -1235,6 +1257,11 @@ static int mlx5_load(struct mlx5_core_dev *dev)
> mlx5_events_start(dev);
> mlx5_pagealloc_start(dev);
>
>+ err = mlx5_get_terminate_scatter_list_mkey(dev);
>+ if (err) {
>+ mlx5_core_err(dev, "Failed to query special contexts\n");
print the err;
>+ goto err_irq_table;
>+ }
you are querying FW too soon, no issue here but better if you move it after
mlx5_eq_table_create() to use the command EQ rather than the primitive cmd
interface.
> err = mlx5_irq_table_create(dev);
> if (err) {
> mlx5_core_err(dev, "Failed to alloc IRQs\n");
>diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
>index d476255c9a3f..5f2c4038d638 100644
>--- a/include/linux/mlx5/driver.h
>+++ b/include/linux/mlx5/driver.h
>@@ -801,6 +801,7 @@ struct mlx5_core_dev {
> struct mlx5_rsc_dump *rsc_dump;
> u32 vsc_addr;
> struct mlx5_hv_vhca *hv_vhca;
>+ __be32 terminate_scatter_list_mkey;
> };
>
> struct mlx5_db {
>--
>2.38.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys
2023-01-06 0:58 ` Saeed Mahameed
@ 2023-01-08 10:32 ` Leon Romanovsky
2023-01-09 22:31 ` Saeed Mahameed
0 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-08 10:32 UTC (permalink / raw)
To: Saeed Mahameed
Cc: Jason Gunthorpe, Or Har-Toov, Eric Dumazet, Jakub Kicinski,
linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
Saeed Mahameed
On Thu, Jan 05, 2023 at 04:58:22PM -0800, Saeed Mahameed wrote:
> On 04 Jan 10:11, Leon Romanovsky wrote:
> > From: Or Har-Toov <ohartoov@nvidia.com>
> >
> > Using query_sepcial_contexts in order to get the correct value of
> > terminate_scatter_list_mkey, as FW will change it in some configurations.
> > This is done one time when the device is loading and the value is being
> > saved in the device context.
> >
> > Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
> > Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> > .../net/ethernet/mellanox/mlx5/core/en_main.c | 2 +-
> > .../net/ethernet/mellanox/mlx5/core/main.c | 27 +++++++++++++++++++
> > include/linux/mlx5/driver.h | 1 +
> > 3 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > index c76f15505a76..33d7a7095988 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > @@ -826,7 +826,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
> > if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
> > wqe->data[f].byte_count = 0;
> > wqe->data[f].lkey =
> > - MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > + mdev->terminate_scatter_list_mkey;
> > wqe->data[f].addr = 0;
> > }
> > }
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > index 7f5db13e3550..d39d758744a0 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > @@ -1221,6 +1221,28 @@ static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot)
> > return 0;
> > }
> >
> > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> > +{
> > + u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > + u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > + int err;
> > +
> > + MLX5_SET(query_special_contexts_in, in, opcode,
> > + MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > + err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > + if (err)
> > + return err;
> > +
> > + if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
>
> Why did you execute the command unconditionally if the output is only read
> conditionally?
>
> early exit before executing the command, older FW might fail and driver will
> be unusable ..
According to the documentation, this functionality was forever, but you
are presenting valid concern.
>
> > + dev->terminate_scatter_list_mkey =
> > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > + terminate_scatter_list_mkey));
> > + return 0;
> > + }
> > + dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
>
> Another concern, what happens with old driver that is using the hardcoded
> value with newer fw ? will it fail ? will it be accepted ?
It will be accepted and everything will work without glitches.
FW keeps backward compatibility and use MLX5_TERMINATE_SCATTER_LIST_LKEY
internally as a default.
>
> The commit message doesn't explain what's going on here very well,
> Let's discuss internally before you submit V2.
NP.
>
> > + return 0;
> > +}
> > +
> > static int mlx5_load(struct mlx5_core_dev *dev)
> > {
> > int err;
> > @@ -1235,6 +1257,11 @@ static int mlx5_load(struct mlx5_core_dev *dev)
> > mlx5_events_start(dev);
> > mlx5_pagealloc_start(dev);
> >
> > + err = mlx5_get_terminate_scatter_list_mkey(dev);
> > + if (err) {
> > + mlx5_core_err(dev, "Failed to query special contexts\n");
> print the err;
> > + goto err_irq_table;
> > + }
>
> you are querying FW too soon, no issue here but better if you move it after
> mlx5_eq_table_create() to use the command EQ rather than the primitive cmd
> interface.
I'll take a look.
Thanks
>
> > err = mlx5_irq_table_create(dev);
> > if (err) {
> > mlx5_core_err(dev, "Failed to alloc IRQs\n");
> > diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
> > index d476255c9a3f..5f2c4038d638 100644
> > --- a/include/linux/mlx5/driver.h
> > +++ b/include/linux/mlx5/driver.h
> > @@ -801,6 +801,7 @@ struct mlx5_core_dev {
> > struct mlx5_rsc_dump *rsc_dump;
> > u32 vsc_addr;
> > struct mlx5_hv_vhca *hv_vhca;
> > + __be32 terminate_scatter_list_mkey;
> > };
> >
> > struct mlx5_db {
> > --
> > 2.38.1
> >
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys
2023-01-08 10:32 ` Leon Romanovsky
@ 2023-01-09 22:31 ` Saeed Mahameed
0 siblings, 0 replies; 18+ messages in thread
From: Saeed Mahameed @ 2023-01-09 22:31 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Jason Gunthorpe, Or Har-Toov, Eric Dumazet, Jakub Kicinski,
linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
Saeed Mahameed
On 08 Jan 12:32, Leon Romanovsky wrote:
>On Thu, Jan 05, 2023 at 04:58:22PM -0800, Saeed Mahameed wrote:
>> On 04 Jan 10:11, Leon Romanovsky wrote:
>> > From: Or Har-Toov <ohartoov@nvidia.com>
>> >
>> > Using query_sepcial_contexts in order to get the correct value of
>> > terminate_scatter_list_mkey, as FW will change it in some configurations.
>> > This is done one time when the device is loading and the value is being
>> > saved in the device context.
>> >
>> > Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
>> > Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
>> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>> > ---
>> > .../net/ethernet/mellanox/mlx5/core/en_main.c | 2 +-
>> > .../net/ethernet/mellanox/mlx5/core/main.c | 27 +++++++++++++++++++
>> > include/linux/mlx5/driver.h | 1 +
>> > 3 files changed, 29 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > index c76f15505a76..33d7a7095988 100644
>> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > @@ -826,7 +826,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
>> > if (rq->wqe.info.num_frags < (1 << rq->wqe.info.log_num_frags)) {
>> > wqe->data[f].byte_count = 0;
>> > wqe->data[f].lkey =
>> > - MLX5_TERMINATE_SCATTER_LIST_LKEY;
>> > + mdev->terminate_scatter_list_mkey;
>> > wqe->data[f].addr = 0;
>> > }
>> > }
>> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> > index 7f5db13e3550..d39d758744a0 100644
>> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> > @@ -1221,6 +1221,28 @@ static int mlx5_function_teardown(struct mlx5_core_dev *dev, bool boot)
>> > return 0;
>> > }
>> >
>> > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
>> > +{
>> > + u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
>> > + u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
>> > + int err;
>> > +
>> > + MLX5_SET(query_special_contexts_in, in, opcode,
>> > + MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
>> > + err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
>> > + if (err)
>> > + return err;
>> > +
>> > + if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
>>
>> Why did you execute the command unconditionally if the output is only read
>> conditionally?
>>
>> early exit before executing the command, older FW might fail and driver will
>> be unusable ..
>
>According to the documentation, this functionality was forever, but you
>are presenting valid concern.
>
>>
>> > + dev->terminate_scatter_list_mkey =
>> > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>> > + terminate_scatter_list_mkey));
>> > + return 0;
>> > + }
>> > + dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
>>
>> Another concern, what happens with old driver that is using the hardcoded
>> value with newer fw ? will it fail ? will it be accepted ?
>
>It will be accepted and everything will work without glitches.
>FW keeps backward compatibility and use MLX5_TERMINATE_SCATTER_LIST_LKEY
>internally as a default.
>
Then please don't change mlx5e to use mdev->terminate_scatter_list_mkey in
this patch, just keep it as is and have a separate patch to change mlx5e.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
2023-01-04 8:11 [PATCH rdma-next 0/4] Rely on firmware to get special mkeys Leon Romanovsky
` (2 preceding siblings ...)
2023-01-04 8:11 ` [PATCH mlx5-next 3/4] net/mlx5: Use query_special_contexts for mkeys Leon Romanovsky
@ 2023-01-04 8:11 ` Leon Romanovsky
2023-01-04 13:03 ` Jason Gunthorpe
3 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04 8:11 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
Saeed Mahameed
From: Or Har-Toov <ohartoov@nvidia.com>
Using query_sepcial_contexts to get the correct value of mkeys such as
null_mkey, terminate_scatter_list_mkey and dump_fill_mkey, as FW will
change them in some configurations.
Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/infiniband/hw/mlx5/cmd.c | 41 ++++++++++++++--------------
drivers/infiniband/hw/mlx5/cmd.h | 3 +-
drivers/infiniband/hw/mlx5/main.c | 10 +++----
drivers/infiniband/hw/mlx5/mlx5_ib.h | 9 +++++-
drivers/infiniband/hw/mlx5/odp.c | 21 ++++----------
drivers/infiniband/hw/mlx5/srq.c | 2 +-
drivers/infiniband/hw/mlx5/wr.c | 2 +-
7 files changed, 43 insertions(+), 45 deletions(-)
diff --git a/drivers/infiniband/hw/mlx5/cmd.c b/drivers/infiniband/hw/mlx5/cmd.c
index ff3742b0460a..f2e465aadd8e 100644
--- a/drivers/infiniband/hw/mlx5/cmd.c
+++ b/drivers/infiniband/hw/mlx5/cmd.c
@@ -5,7 +5,7 @@
#include "cmd.h"
-int mlx5_cmd_dump_fill_mkey(struct mlx5_core_dev *dev, u32 *mkey)
+int mlx5_cmd_query_special_mkeys(struct mlx5_ib_dev *dev)
{
u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
@@ -13,26 +13,27 @@ int mlx5_cmd_dump_fill_mkey(struct mlx5_core_dev *dev, u32 *mkey)
MLX5_SET(query_special_contexts_in, in, opcode,
MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
- err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
- if (!err)
- *mkey = MLX5_GET(query_special_contexts_out, out,
- dump_fill_mkey);
- return err;
-}
-
-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
-{
- u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
- u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
- int err;
+ err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
+ if (err)
+ return err;
- MLX5_SET(query_special_contexts_in, in, opcode,
- MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
- err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
- if (!err)
- *null_mkey = MLX5_GET(query_special_contexts_out, out,
- null_mkey);
- return err;
+ if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
+ dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
+ out, dump_fill_mkey);
+
+ if (MLX5_CAP_GEN(dev->mdev, null_mkey))
+ dev->mkeys.null_mkey = cpu_to_be32(
+ MLX5_GET(query_special_contexts_out, out, null_mkey));
+
+ if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
+ dev->mkeys.terminate_scatter_list_mkey =
+ cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+ terminate_scatter_list_mkey));
+ return 0;
+ }
+ dev->mkeys.terminate_scatter_list_mkey =
+ MLX5_TERMINATE_SCATTER_LIST_LKEY;
+ return 0;
}
int mlx5_cmd_query_cong_params(struct mlx5_core_dev *dev, int cong_point,
diff --git a/drivers/infiniband/hw/mlx5/cmd.h b/drivers/infiniband/hw/mlx5/cmd.h
index ee46638db5de..79ccd7dfa67a 100644
--- a/drivers/infiniband/hw/mlx5/cmd.h
+++ b/drivers/infiniband/hw/mlx5/cmd.h
@@ -37,8 +37,7 @@
#include <linux/kernel.h>
#include <linux/mlx5/driver.h>
-int mlx5_cmd_dump_fill_mkey(struct mlx5_core_dev *dev, u32 *mkey);
-int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey);
+int mlx5_cmd_query_special_mkeys(struct mlx5_ib_dev *dev);
int mlx5_cmd_query_cong_params(struct mlx5_core_dev *dev, int cong_point,
void *out);
int mlx5_cmd_dealloc_pd(struct mlx5_core_dev *dev, u32 pdn, u16 uid);
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index c669ef6e47e7..12e8bf99a40e 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -1756,13 +1756,9 @@ static int set_ucontext_resp(struct ib_ucontext *uctx,
struct mlx5_ib_dev *dev = to_mdev(ibdev);
struct mlx5_ib_ucontext *context = to_mucontext(uctx);
struct mlx5_bfreg_info *bfregi = &context->bfregi;
- int err;
if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey)) {
- err = mlx5_cmd_dump_fill_mkey(dev->mdev,
- &resp->dump_fill_mkey);
- if (err)
- return err;
+ resp->dump_fill_mkey = dev->mkeys.dump_fill_mkey;
resp->comp_mask |=
MLX5_IB_ALLOC_UCONTEXT_RESP_MASK_DUMP_FILL_MKEY;
}
@@ -3634,6 +3630,10 @@ static int mlx5_ib_stage_init_init(struct mlx5_ib_dev *dev)
dev->port[i].roce.last_port_state = IB_PORT_DOWN;
}
+ err = mlx5_cmd_query_special_mkeys(dev);
+ if (err)
+ return err;
+
err = mlx5_ib_init_multiport_master(dev);
if (err)
return err;
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index 093aa69af0ef..42a0d1c8d85c 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -1051,6 +1051,13 @@ struct mlx5_port_caps {
u8 ext_port_cap;
};
+
+struct mlx5_special_mkeys {
+ u32 dump_fill_mkey;
+ __be32 null_mkey;
+ __be32 terminate_scatter_list_mkey;
+};
+
struct mlx5_ib_dev {
struct ib_device ib_dev;
struct mlx5_core_dev *mdev;
@@ -1081,7 +1088,6 @@ struct mlx5_ib_dev {
struct xarray odp_mkeys;
- u32 null_mkey;
struct mlx5_ib_flow_db *flow_db;
/* protect resources needed as part of reset flow */
spinlock_t reset_flow_resource_lock;
@@ -1110,6 +1116,7 @@ struct mlx5_ib_dev {
struct mlx5_port_caps port_caps[MLX5_MAX_PORTS];
u16 pkey_table_len;
u8 lag_ports;
+ struct mlx5_special_mkeys mkeys;
};
static inline struct mlx5_ib_cq *to_mibcq(struct mlx5_core_cq *mcq)
diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
index b4ebeadce67c..4998eaeadcbb 100644
--- a/drivers/infiniband/hw/mlx5/odp.c
+++ b/drivers/infiniband/hw/mlx5/odp.c
@@ -104,7 +104,7 @@ static void populate_klm(struct mlx5_klm *pklm, size_t idx, size_t nentries,
if (flags & MLX5_IB_UPD_XLT_ZAP) {
for (; pklm != end; pklm++, idx++) {
pklm->bcount = cpu_to_be32(MLX5_IMR_MTT_SIZE);
- pklm->key = cpu_to_be32(mr_to_mdev(imr)->null_mkey);
+ pklm->key = mr_to_mdev(imr)->mkeys.null_mkey;
pklm->va = 0;
}
return;
@@ -137,7 +137,7 @@ static void populate_klm(struct mlx5_klm *pklm, size_t idx, size_t nentries,
pklm->key = cpu_to_be32(mtt->ibmr.lkey);
pklm->va = cpu_to_be64(idx * MLX5_IMR_MTT_SIZE);
} else {
- pklm->key = cpu_to_be32(mr_to_mdev(imr)->null_mkey);
+ pklm->key = mr_to_mdev(imr)->mkeys.null_mkey;
pklm->va = 0;
}
}
@@ -1015,7 +1015,8 @@ static int pagefault_data_segments(struct mlx5_ib_dev *dev,
/* receive WQE end of sg list. */
if (receive_queue && bcnt == 0 &&
- key == MLX5_TERMINATE_SCATTER_LIST_LKEY && io_virt == 0)
+ key == dev->mkeys.terminate_scatter_list_mkey &&
+ io_virt == 0)
break;
if (!inline_segment && total_wqe_bytes) {
@@ -1615,25 +1616,15 @@ static const struct ib_device_ops mlx5_ib_dev_odp_ops = {
int mlx5_ib_odp_init_one(struct mlx5_ib_dev *dev)
{
- int ret = 0;
-
internal_fill_odp_caps(dev);
if (!(dev->odp_caps.general_caps & IB_ODP_SUPPORT))
- return ret;
+ return 0;
ib_set_device_ops(&dev->ib_dev, &mlx5_ib_dev_odp_ops);
- if (dev->odp_caps.general_caps & IB_ODP_SUPPORT_IMPLICIT) {
- ret = mlx5_cmd_null_mkey(dev->mdev, &dev->null_mkey);
- if (ret) {
- mlx5_ib_err(dev, "Error getting null_mkey %d\n", ret);
- return ret;
- }
- }
-
mutex_init(&dev->odp_eq_mutex);
- return ret;
+ return 0;
}
void mlx5_ib_odp_cleanup_one(struct mlx5_ib_dev *dev)
diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
index bcceb14a07f9..32c6643d0f7a 100644
--- a/drivers/infiniband/hw/mlx5/srq.c
+++ b/drivers/infiniband/hw/mlx5/srq.c
@@ -447,7 +447,7 @@ int mlx5_ib_post_srq_recv(struct ib_srq *ibsrq, const struct ib_recv_wr *wr,
if (i < srq->msrq.max_avail_gather) {
scat[i].byte_count = 0;
- scat[i].lkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
+ scat[i].lkey = dev->mkeys.terminate_scatter_list_mkey;
scat[i].addr = 0;
}
}
diff --git a/drivers/infiniband/hw/mlx5/wr.c b/drivers/infiniband/hw/mlx5/wr.c
index bc44551493e2..df1d1b0a3ef7 100644
--- a/drivers/infiniband/hw/mlx5/wr.c
+++ b/drivers/infiniband/hw/mlx5/wr.c
@@ -1252,7 +1252,7 @@ int mlx5_ib_post_recv(struct ib_qp *ibqp, const struct ib_recv_wr *wr,
if (i < qp->rq.max_gs) {
scat[i].byte_count = 0;
- scat[i].lkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
+ scat[i].lkey = dev->mkeys.terminate_scatter_list_mkey;
scat[i].addr = 0;
}
--
2.38.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
2023-01-04 8:11 ` [PATCH rdma-next 4/4] RDMA/mlx5: " Leon Romanovsky
@ 2023-01-04 13:03 ` Jason Gunthorpe
2023-01-04 13:09 ` Leon Romanovsky
0 siblings, 1 reply; 18+ messages in thread
From: Jason Gunthorpe @ 2023-01-04 13:03 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
Saeed Mahameed
On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> -{
> - u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> - u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> - int err;
> + err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> + if (err)
> + return err;
>
> - MLX5_SET(query_special_contexts_in, in, opcode,
> - MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> - err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> - if (!err)
> - *null_mkey = MLX5_GET(query_special_contexts_out, out,
> - null_mkey);
> - return err;
> + if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> + dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> + out, dump_fill_mkey);
> +
> + if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> + dev->mkeys.null_mkey = cpu_to_be32(
> + MLX5_GET(query_special_contexts_out, out, null_mkey));
> +
> + if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> + dev->mkeys.terminate_scatter_list_mkey =
> + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> + terminate_scatter_list_mkey));
> + return 0;
> + }
> + dev->mkeys.terminate_scatter_list_mkey =
> + MLX5_TERMINATE_SCATTER_LIST_LKEY;
This is already stored in the core dev, why are you recalculating it
here?
Jason
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
2023-01-04 13:03 ` Jason Gunthorpe
@ 2023-01-04 13:09 ` Leon Romanovsky
2023-01-04 13:13 ` Jason Gunthorpe
0 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04 13:09 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
Saeed Mahameed
On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > -{
> > - u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > - u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > - int err;
> > + err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > + if (err)
> > + return err;
> >
> > - MLX5_SET(query_special_contexts_in, in, opcode,
> > - MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > - err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > - if (!err)
> > - *null_mkey = MLX5_GET(query_special_contexts_out, out,
> > - null_mkey);
> > - return err;
> > + if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > + dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > + out, dump_fill_mkey);
> > +
> > + if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > + dev->mkeys.null_mkey = cpu_to_be32(
> > + MLX5_GET(query_special_contexts_out, out, null_mkey));
> > +
> > + if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > + dev->mkeys.terminate_scatter_list_mkey =
> > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > + terminate_scatter_list_mkey));
> > + return 0;
> > + }
> > + dev->mkeys.terminate_scatter_list_mkey =
> > + MLX5_TERMINATE_SCATTER_LIST_LKEY;
>
> This is already stored in the core dev, why are you recalculating it
> here?
It is not recalculating but setting default value. In core dev, we will
have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
is true.
Thanks
>
> Jason
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
2023-01-04 13:09 ` Leon Romanovsky
@ 2023-01-04 13:13 ` Jason Gunthorpe
2023-01-04 13:55 ` Leon Romanovsky
0 siblings, 1 reply; 18+ messages in thread
From: Jason Gunthorpe @ 2023-01-04 13:13 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
Saeed Mahameed
On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > -{
> > > - u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > - u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > - int err;
> > > + err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > + if (err)
> > > + return err;
> > >
> > > - MLX5_SET(query_special_contexts_in, in, opcode,
> > > - MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > - err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > - if (!err)
> > > - *null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > - null_mkey);
> > > - return err;
> > > + if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > + dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > + out, dump_fill_mkey);
> > > +
> > > + if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > + dev->mkeys.null_mkey = cpu_to_be32(
> > > + MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > +
> > > + if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > + dev->mkeys.terminate_scatter_list_mkey =
> > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > + terminate_scatter_list_mkey));
> > > + return 0;
> > > + }
> > > + dev->mkeys.terminate_scatter_list_mkey =
> > > + MLX5_TERMINATE_SCATTER_LIST_LKEY;
> >
> > This is already stored in the core dev, why are you recalculating it
> > here?
>
> It is not recalculating but setting default value. In core dev, we will
> have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> is true.
No, it has the identical code:
+static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
+{
+ if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
+ dev->terminate_scatter_list_mkey =
+ cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
+ terminate_scatter_list_mkey));
+ return 0;
+ }
+ dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
Jason
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
2023-01-04 13:13 ` Jason Gunthorpe
@ 2023-01-04 13:55 ` Leon Romanovsky
2023-01-04 13:56 ` Jason Gunthorpe
0 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04 13:55 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
Saeed Mahameed
On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > > -{
> > > > - u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > > - u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > > - int err;
> > > > + err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > > + if (err)
> > > > + return err;
> > > >
> > > > - MLX5_SET(query_special_contexts_in, in, opcode,
> > > > - MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > > - err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > > - if (!err)
> > > > - *null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > > - null_mkey);
> > > > - return err;
> > > > + if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > > + dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > > + out, dump_fill_mkey);
> > > > +
> > > > + if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > > + dev->mkeys.null_mkey = cpu_to_be32(
> > > > + MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > > +
> > > > + if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > > + dev->mkeys.terminate_scatter_list_mkey =
> > > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > + terminate_scatter_list_mkey));
> > > > + return 0;
> > > > + }
> > > > + dev->mkeys.terminate_scatter_list_mkey =
> > > > + MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > >
> > > This is already stored in the core dev, why are you recalculating it
> > > here?
> >
> > It is not recalculating but setting default value. In core dev, we will
> > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> > is true.
>
> No, it has the identical code:
>
> +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> +{
> + if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
> + dev->terminate_scatter_list_mkey =
> + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> + terminate_scatter_list_mkey));
> + return 0;
> + }
> + dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
Ahh, you are talking about that.
terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
which is needed to get other mkeys. So instead of doing special logic
for the terminate_scatter_list_mkey, we decided to use same pattern as
for other mkeys, which don't belong to core.
Thanks
>
> Jason
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
2023-01-04 13:55 ` Leon Romanovsky
@ 2023-01-04 13:56 ` Jason Gunthorpe
2023-01-04 14:03 ` Leon Romanovsky
0 siblings, 1 reply; 18+ messages in thread
From: Jason Gunthorpe @ 2023-01-04 13:56 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
Saeed Mahameed
On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
> On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
> > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > > > -{
> > > > > - u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > > > - u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > > > - int err;
> > > > > + err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > > > + if (err)
> > > > > + return err;
> > > > >
> > > > > - MLX5_SET(query_special_contexts_in, in, opcode,
> > > > > - MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > > > - err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > > > - if (!err)
> > > > > - *null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > > > - null_mkey);
> > > > > - return err;
> > > > > + if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > > > + dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > > > + out, dump_fill_mkey);
> > > > > +
> > > > > + if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > > > + dev->mkeys.null_mkey = cpu_to_be32(
> > > > > + MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > > > +
> > > > > + if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > > > + dev->mkeys.terminate_scatter_list_mkey =
> > > > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > + terminate_scatter_list_mkey));
> > > > > + return 0;
> > > > > + }
> > > > > + dev->mkeys.terminate_scatter_list_mkey =
> > > > > + MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > >
> > > > This is already stored in the core dev, why are you recalculating it
> > > > here?
> > >
> > > It is not recalculating but setting default value. In core dev, we will
> > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> > > is true.
> >
> > No, it has the identical code:
> >
> > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> > +{
> > + if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
> > + dev->terminate_scatter_list_mkey =
> > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > + terminate_scatter_list_mkey));
> > + return 0;
> > + }
> > + dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
>
> Ahh, you are talking about that.
> terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
> which is needed to get other mkeys. So instead of doing special logic
> for the terminate_scatter_list_mkey, we decided to use same pattern as
> for other mkeys, which don't belong to core.
Regardless, don't duplicate the code and maybe don't even duplicate
the storage of the terminate_scatter_list_mkey
Jason
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
2023-01-04 13:56 ` Jason Gunthorpe
@ 2023-01-04 14:03 ` Leon Romanovsky
2023-01-06 1:04 ` Saeed Mahameed
0 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-04 14:03 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Or Har-Toov, David S. Miller, Eric Dumazet, Jakub Kicinski,
linux-rdma, Michael Guralnik, netdev, Paolo Abeni,
Saeed Mahameed
On Wed, Jan 04, 2023 at 09:56:21AM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
> > On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
> > > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> > > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > > > > -{
> > > > > > - u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > > > > - u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > > > > - int err;
> > > > > > + err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > > > > + if (err)
> > > > > > + return err;
> > > > > >
> > > > > > - MLX5_SET(query_special_contexts_in, in, opcode,
> > > > > > - MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > > > > - err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > > > > - if (!err)
> > > > > > - *null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > > > > - null_mkey);
> > > > > > - return err;
> > > > > > + if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > > > > + dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > > > > + out, dump_fill_mkey);
> > > > > > +
> > > > > > + if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > > > > + dev->mkeys.null_mkey = cpu_to_be32(
> > > > > > + MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > > > > +
> > > > > > + if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > > > > + dev->mkeys.terminate_scatter_list_mkey =
> > > > > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > > + terminate_scatter_list_mkey));
> > > > > > + return 0;
> > > > > > + }
> > > > > > + dev->mkeys.terminate_scatter_list_mkey =
> > > > > > + MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > > >
> > > > > This is already stored in the core dev, why are you recalculating it
> > > > > here?
> > > >
> > > > It is not recalculating but setting default value. In core dev, we will
> > > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> > > > is true.
> > >
> > > No, it has the identical code:
> > >
> > > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> > > +{
> > > + if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
> > > + dev->terminate_scatter_list_mkey =
> > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > + terminate_scatter_list_mkey));
> > > + return 0;
> > > + }
> > > + dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
> >
> > Ahh, you are talking about that.
> > terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
> > which is needed to get other mkeys. So instead of doing special logic
> > for the terminate_scatter_list_mkey, we decided to use same pattern as
> > for other mkeys, which don't belong to core.
>
> Regardless, don't duplicate the code and maybe don't even duplicate
> the storage of the terminate_scatter_list_mkey
ok, will update and resend.
Thanks
>
> Jason
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
2023-01-04 14:03 ` Leon Romanovsky
@ 2023-01-06 1:04 ` Saeed Mahameed
2023-01-08 10:21 ` Leon Romanovsky
0 siblings, 1 reply; 18+ messages in thread
From: Saeed Mahameed @ 2023-01-06 1:04 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Jason Gunthorpe, Or Har-Toov, David S. Miller, Eric Dumazet,
Jakub Kicinski, linux-rdma, Michael Guralnik, netdev,
Paolo Abeni, Saeed Mahameed
On 04 Jan 16:03, Leon Romanovsky wrote:
>On Wed, Jan 04, 2023 at 09:56:21AM -0400, Jason Gunthorpe wrote:
>> On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
>> > On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
>> > > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
>> > > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
>> > > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
>> > > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
>> > > > > > -{
>> > > > > > - u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
>> > > > > > - u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
>> > > > > > - int err;
>> > > > > > + err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
>> > > > > > + if (err)
>> > > > > > + return err;
>> > > > > >
>> > > > > > - MLX5_SET(query_special_contexts_in, in, opcode,
>> > > > > > - MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
>> > > > > > - err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
>> > > > > > - if (!err)
>> > > > > > - *null_mkey = MLX5_GET(query_special_contexts_out, out,
>> > > > > > - null_mkey);
>> > > > > > - return err;
>> > > > > > + if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
>> > > > > > + dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
>> > > > > > + out, dump_fill_mkey);
>> > > > > > +
>> > > > > > + if (MLX5_CAP_GEN(dev->mdev, null_mkey))
>> > > > > > + dev->mkeys.null_mkey = cpu_to_be32(
>> > > > > > + MLX5_GET(query_special_contexts_out, out, null_mkey));
>> > > > > > +
>> > > > > > + if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
>> > > > > > + dev->mkeys.terminate_scatter_list_mkey =
>> > > > > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>> > > > > > + terminate_scatter_list_mkey));
>> > > > > > + return 0;
>> > > > > > + }
>> > > > > > + dev->mkeys.terminate_scatter_list_mkey =
>> > > > > > + MLX5_TERMINATE_SCATTER_LIST_LKEY;
>> > > > >
>> > > > > This is already stored in the core dev, why are you recalculating it
>> > > > > here?
>> > > >
>> > > > It is not recalculating but setting default value. In core dev, we will
>> > > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
>> > > > is true.
>> > >
>> > > No, it has the identical code:
>> > >
>> > > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
>> > > +{
>> > > + if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
>> > > + dev->terminate_scatter_list_mkey =
>> > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>> > > + terminate_scatter_list_mkey));
>> > > + return 0;
>> > > + }
>> > > + dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
>> >
>> > Ahh, you are talking about that.
>> > terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
>> > which is needed to get other mkeys. So instead of doing special logic
>> > for the terminate_scatter_list_mkey, we decided to use same pattern as
>> > for other mkeys, which don't belong to core.
>>
>> Regardless, don't duplicate the code and maybe don't even duplicate
>> the storage of the terminate_scatter_list_mkey
>
>ok, will update and resend.
>
Please provide a helper mlx5_get_xyz function, avoid assuming mlx5_core will store
it in dev->xyz.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
2023-01-06 1:04 ` Saeed Mahameed
@ 2023-01-08 10:21 ` Leon Romanovsky
2023-01-09 22:24 ` Saeed Mahameed
0 siblings, 1 reply; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-08 10:21 UTC (permalink / raw)
To: Saeed Mahameed
Cc: Jason Gunthorpe, Or Har-Toov, David S. Miller, Eric Dumazet,
Jakub Kicinski, linux-rdma, Michael Guralnik, netdev,
Paolo Abeni, Saeed Mahameed
On Thu, Jan 05, 2023 at 05:04:03PM -0800, Saeed Mahameed wrote:
> On 04 Jan 16:03, Leon Romanovsky wrote:
> > On Wed, Jan 04, 2023 at 09:56:21AM -0400, Jason Gunthorpe wrote:
> > > On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
> > > > On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
> > > > > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> > > > > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > > > > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > > > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > > > > > > -{
> > > > > > > > - u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > > > > > > - u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > > > > > > - int err;
> > > > > > > > + err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > > > > > > + if (err)
> > > > > > > > + return err;
> > > > > > > >
> > > > > > > > - MLX5_SET(query_special_contexts_in, in, opcode,
> > > > > > > > - MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > > > > > > - err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > > > > > > - if (!err)
> > > > > > > > - *null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > > > > > > - null_mkey);
> > > > > > > > - return err;
> > > > > > > > + if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > > > > > > + dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > > > > > > + out, dump_fill_mkey);
> > > > > > > > +
> > > > > > > > + if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > > > > > > + dev->mkeys.null_mkey = cpu_to_be32(
> > > > > > > > + MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > > > > > > +
> > > > > > > > + if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > > > > > > + dev->mkeys.terminate_scatter_list_mkey =
> > > > > > > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > > > > + terminate_scatter_list_mkey));
> > > > > > > > + return 0;
> > > > > > > > + }
> > > > > > > > + dev->mkeys.terminate_scatter_list_mkey =
> > > > > > > > + MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > > > > >
> > > > > > > This is already stored in the core dev, why are you recalculating it
> > > > > > > here?
> > > > > >
> > > > > > It is not recalculating but setting default value. In core dev, we will
> > > > > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> > > > > > is true.
> > > > >
> > > > > No, it has the identical code:
> > > > >
> > > > > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> > > > > +{
> > > > > + if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
> > > > > + dev->terminate_scatter_list_mkey =
> > > > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > + terminate_scatter_list_mkey));
> > > > > + return 0;
> > > > > + }
> > > > > + dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > >
> > > > Ahh, you are talking about that.
> > > > terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
> > > > which is needed to get other mkeys. So instead of doing special logic
> > > > for the terminate_scatter_list_mkey, we decided to use same pattern as
> > > > for other mkeys, which don't belong to core.
> > >
> > > Regardless, don't duplicate the code and maybe don't even duplicate
> > > the storage of the terminate_scatter_list_mkey
> >
> > ok, will update and resend.
> >
>
> Please provide a helper mlx5_get_xyz function, avoid assuming mlx5_core will store
> it in dev->xyz.
This helper was used in this version of patch and it is:
MLX5_GET(query_special_contexts_out, out, terminate_scatter_list_mkey))
Which was dropped in favor of Jason's request to rely on mlx5_core_dev
as this value already known:
https://lore.kernel.org/all/cover.1672917578.git.leonro@nvidia.com
I don't like the idea to add obfuscation function just for the sake of
obfuscation. If you don't want access to mlx5_core_dev, please argue with
Jason to accept first variant.
Thanks
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
2023-01-08 10:21 ` Leon Romanovsky
@ 2023-01-09 22:24 ` Saeed Mahameed
2023-01-10 8:45 ` Leon Romanovsky
0 siblings, 1 reply; 18+ messages in thread
From: Saeed Mahameed @ 2023-01-09 22:24 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Jason Gunthorpe, Or Har-Toov, David S. Miller, Eric Dumazet,
Jakub Kicinski, linux-rdma, Michael Guralnik, netdev,
Paolo Abeni, Saeed Mahameed
On 08 Jan 12:21, Leon Romanovsky wrote:
>On Thu, Jan 05, 2023 at 05:04:03PM -0800, Saeed Mahameed wrote:
>> On 04 Jan 16:03, Leon Romanovsky wrote:
>> > On Wed, Jan 04, 2023 at 09:56:21AM -0400, Jason Gunthorpe wrote:
>> > > On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
>> > > > On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
>> > > > > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
>> > > > > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
>> > > > > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
>> > > > > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
>> > > > > > > > -{
>> > > > > > > > - u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
>> > > > > > > > - u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
>> > > > > > > > - int err;
>> > > > > > > > + err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
>> > > > > > > > + if (err)
>> > > > > > > > + return err;
>> > > > > > > >
>> > > > > > > > - MLX5_SET(query_special_contexts_in, in, opcode,
>> > > > > > > > - MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
>> > > > > > > > - err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
>> > > > > > > > - if (!err)
>> > > > > > > > - *null_mkey = MLX5_GET(query_special_contexts_out, out,
>> > > > > > > > - null_mkey);
>> > > > > > > > - return err;
>> > > > > > > > + if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
>> > > > > > > > + dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
>> > > > > > > > + out, dump_fill_mkey);
>> > > > > > > > +
>> > > > > > > > + if (MLX5_CAP_GEN(dev->mdev, null_mkey))
>> > > > > > > > + dev->mkeys.null_mkey = cpu_to_be32(
>> > > > > > > > + MLX5_GET(query_special_contexts_out, out, null_mkey));
>> > > > > > > > +
>> > > > > > > > + if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
>> > > > > > > > + dev->mkeys.terminate_scatter_list_mkey =
>> > > > > > > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>> > > > > > > > + terminate_scatter_list_mkey));
>> > > > > > > > + return 0;
>> > > > > > > > + }
>> > > > > > > > + dev->mkeys.terminate_scatter_list_mkey =
>> > > > > > > > + MLX5_TERMINATE_SCATTER_LIST_LKEY;
>> > > > > > >
>> > > > > > > This is already stored in the core dev, why are you recalculating it
>> > > > > > > here?
>> > > > > >
>> > > > > > It is not recalculating but setting default value. In core dev, we will
>> > > > > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
>> > > > > > is true.
>> > > > >
>> > > > > No, it has the identical code:
>> > > > >
>> > > > > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
>> > > > > +{
>> > > > > + if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
>> > > > > + dev->terminate_scatter_list_mkey =
>> > > > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
>> > > > > + terminate_scatter_list_mkey));
>> > > > > + return 0;
>> > > > > + }
>> > > > > + dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
>> > > >
>> > > > Ahh, you are talking about that.
>> > > > terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
>> > > > which is needed to get other mkeys. So instead of doing special logic
>> > > > for the terminate_scatter_list_mkey, we decided to use same pattern as
>> > > > for other mkeys, which don't belong to core.
>> > >
>> > > Regardless, don't duplicate the code and maybe don't even duplicate
>> > > the storage of the terminate_scatter_list_mkey
>> >
>> > ok, will update and resend.
>> >
>>
>> Please provide a helper mlx5_get_xyz function, avoid assuming mlx5_core will store
>> it in dev->xyz.
>
>This helper was used in this version of patch and it is:
> MLX5_GET(query_special_contexts_out, out, terminate_scatter_list_mkey))
>Which was dropped in favor of Jason's request to rely on mlx5_core_dev
>as this value already known:
>https://lore.kernel.org/all/cover.1672917578.git.leonro@nvidia.com
>
>I don't like the idea to add obfuscation function just for the sake of
>obfuscation. If you don't want access to mlx5_core_dev, please argue with
>Jason to accept first variant.
I think you are confused, 1st version duplicated the logic that already
existed in mlx5 core, didn't just access with MLX5_GET.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH rdma-next 4/4] RDMA/mlx5: Use query_special_contexts for mkeys
2023-01-09 22:24 ` Saeed Mahameed
@ 2023-01-10 8:45 ` Leon Romanovsky
0 siblings, 0 replies; 18+ messages in thread
From: Leon Romanovsky @ 2023-01-10 8:45 UTC (permalink / raw)
To: Saeed Mahameed
Cc: Jason Gunthorpe, Or Har-Toov, David S. Miller, Eric Dumazet,
Jakub Kicinski, linux-rdma, Michael Guralnik, netdev,
Paolo Abeni, Saeed Mahameed
On Mon, Jan 09, 2023 at 02:24:28PM -0800, Saeed Mahameed wrote:
> On 08 Jan 12:21, Leon Romanovsky wrote:
> > On Thu, Jan 05, 2023 at 05:04:03PM -0800, Saeed Mahameed wrote:
> > > On 04 Jan 16:03, Leon Romanovsky wrote:
> > > > On Wed, Jan 04, 2023 at 09:56:21AM -0400, Jason Gunthorpe wrote:
> > > > > On Wed, Jan 04, 2023 at 03:55:25PM +0200, Leon Romanovsky wrote:
> > > > > > On Wed, Jan 04, 2023 at 09:13:10AM -0400, Jason Gunthorpe wrote:
> > > > > > > On Wed, Jan 04, 2023 at 03:09:54PM +0200, Leon Romanovsky wrote:
> > > > > > > > On Wed, Jan 04, 2023 at 09:03:06AM -0400, Jason Gunthorpe wrote:
> > > > > > > > > On Wed, Jan 04, 2023 at 10:11:25AM +0200, Leon Romanovsky wrote:
> > > > > > > > > > -int mlx5_cmd_null_mkey(struct mlx5_core_dev *dev, u32 *null_mkey)
> > > > > > > > > > -{
> > > > > > > > > > - u32 out[MLX5_ST_SZ_DW(query_special_contexts_out)] = {};
> > > > > > > > > > - u32 in[MLX5_ST_SZ_DW(query_special_contexts_in)] = {};
> > > > > > > > > > - int err;
> > > > > > > > > > + err = mlx5_cmd_exec_inout(dev->mdev, query_special_contexts, in, out);
> > > > > > > > > > + if (err)
> > > > > > > > > > + return err;
> > > > > > > > > >
> > > > > > > > > > - MLX5_SET(query_special_contexts_in, in, opcode,
> > > > > > > > > > - MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS);
> > > > > > > > > > - err = mlx5_cmd_exec_inout(dev, query_special_contexts, in, out);
> > > > > > > > > > - if (!err)
> > > > > > > > > > - *null_mkey = MLX5_GET(query_special_contexts_out, out,
> > > > > > > > > > - null_mkey);
> > > > > > > > > > - return err;
> > > > > > > > > > + if (MLX5_CAP_GEN(dev->mdev, dump_fill_mkey))
> > > > > > > > > > + dev->mkeys.dump_fill_mkey = MLX5_GET(query_special_contexts_out,
> > > > > > > > > > + out, dump_fill_mkey);
> > > > > > > > > > +
> > > > > > > > > > + if (MLX5_CAP_GEN(dev->mdev, null_mkey))
> > > > > > > > > > + dev->mkeys.null_mkey = cpu_to_be32(
> > > > > > > > > > + MLX5_GET(query_special_contexts_out, out, null_mkey));
> > > > > > > > > > +
> > > > > > > > > > + if (MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)) {
> > > > > > > > > > + dev->mkeys.terminate_scatter_list_mkey =
> > > > > > > > > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > > > > > > + terminate_scatter_list_mkey));
> > > > > > > > > > + return 0;
> > > > > > > > > > + }
> > > > > > > > > > + dev->mkeys.terminate_scatter_list_mkey =
> > > > > > > > > > + MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > > > > > > >
> > > > > > > > > This is already stored in the core dev, why are you recalculating it
> > > > > > > > > here?
> > > > > > > >
> > > > > > > > It is not recalculating but setting default value. In core dev, we will
> > > > > > > > have value only if MLX5_CAP_GEN(dev->mdev, terminate_scatter_list_mkey)
> > > > > > > > is true.
> > > > > > >
> > > > > > > No, it has the identical code:
> > > > > > >
> > > > > > > +static int mlx5_get_terminate_scatter_list_mkey(struct mlx5_core_dev *dev)
> > > > > > > +{
> > > > > > > + if (MLX5_CAP_GEN(dev, terminate_scatter_list_mkey)) {
> > > > > > > + dev->terminate_scatter_list_mkey =
> > > > > > > + cpu_to_be32(MLX5_GET(query_special_contexts_out, out,
> > > > > > > + terminate_scatter_list_mkey));
> > > > > > > + return 0;
> > > > > > > + }
> > > > > > > + dev->terminate_scatter_list_mkey = MLX5_TERMINATE_SCATTER_LIST_LKEY;
> > > > > >
> > > > > > Ahh, you are talking about that.
> > > > > > terminate_scatter_list_mkey is part of an output from MLX5_CMD_OP_QUERY_SPECIAL_CONTEXTS,
> > > > > > which is needed to get other mkeys. So instead of doing special logic
> > > > > > for the terminate_scatter_list_mkey, we decided to use same pattern as
> > > > > > for other mkeys, which don't belong to core.
> > > > >
> > > > > Regardless, don't duplicate the code and maybe don't even duplicate
> > > > > the storage of the terminate_scatter_list_mkey
> > > >
> > > > ok, will update and resend.
> > > >
> > >
> > > Please provide a helper mlx5_get_xyz function, avoid assuming mlx5_core will store
> > > it in dev->xyz.
> >
> > This helper was used in this version of patch and it is:
> > MLX5_GET(query_special_contexts_out, out, terminate_scatter_list_mkey))
> > Which was dropped in favor of Jason's request to rely on mlx5_core_dev
> > as this value already known:
> > https://lore.kernel.org/all/cover.1672917578.git.leonro@nvidia.com
> >
> > I don't like the idea to add obfuscation function just for the sake of
> > obfuscation. If you don't want access to mlx5_core_dev, please argue with
> > Jason to accept first variant.
>
> I think you are confused, 1st version duplicated the logic that already
> existed in mlx5 core, didn't just access with MLX5_GET.
No, the end result is the same: call to the memory to get terminate_scatter_list_mkey
value. MLX5_GET(..) will return exactly the same value as dev->terminate_scatter_list_mkey
or some other function call:
int mlx5_get_terminate_scatter_list_mkey() {
return dev->terminate_scatter_list_mkey;
}
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread