* [PATCH 0/5] SRP kernel patches for kernel v5.14
@ 2021-05-12 3:27 Bart Van Assche
2021-05-12 3:27 ` [PATCH 1/5] RDMA/ib_hdrs.h: Remove a superfluous cast Bart Van Assche
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Bart Van Assche @ 2021-05-12 3:27 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Bart Van Assche
Hi Jason,
Please consider the five patches in this series for kernel v5.14.
Thanks,
Bart.
Bart Van Assche (5):
RDMA/ib_hdrs.h: Remove a superfluous cast
RDMA/srp: Add more structure size checks
RDMA/srp: Apply the __packed attribute to members instead of
structures
RDMA/srp: Fix a recently introduced memory leak
RDMA/srp: Make struct scsi_cmnd and struct srp_request adjacent
drivers/infiniband/ulp/srp/ib_srp.c | 154 ++++++++++++----------------
drivers/infiniband/ulp/srp/ib_srp.h | 2 -
include/rdma/ib_hdrs.h | 2 +-
include/scsi/srp.h | 26 ++---
4 files changed, 76 insertions(+), 108 deletions(-)
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/5] RDMA/ib_hdrs.h: Remove a superfluous cast
2021-05-12 3:27 [PATCH 0/5] SRP kernel patches for kernel v5.14 Bart Van Assche
@ 2021-05-12 3:27 ` Bart Van Assche
2021-05-19 9:00 ` Leon Romanovsky
2021-05-12 3:27 ` [PATCH 2/5] RDMA/srp: Add more structure size checks Bart Van Assche
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2021-05-12 3:27 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Bart Van Assche,
Don Hiatt, Dennis Dalessandro
be16_to_cpu() returns a u16 value. Remove the superfluous u16 cast. That
cast was introduced by commit 7dafbab3753f ("IB/hfi1: Add functions to
parse BTH/IB headers").
Cc: Don Hiatt <don.hiatt@intel.com>
Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
include/rdma/ib_hdrs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/rdma/ib_hdrs.h b/include/rdma/ib_hdrs.h
index 57c1ac881d08..82483120539f 100644
--- a/include/rdma/ib_hdrs.h
+++ b/include/rdma/ib_hdrs.h
@@ -208,7 +208,7 @@ static inline u8 ib_get_lver(struct ib_header *hdr)
static inline u16 ib_get_len(struct ib_header *hdr)
{
- return (u16)(be16_to_cpu(hdr->lrh[2]));
+ return be16_to_cpu(hdr->lrh[2]);
}
static inline u32 ib_get_qkey(struct ib_other_headers *ohdr)
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/5] RDMA/srp: Add more structure size checks
2021-05-12 3:27 [PATCH 0/5] SRP kernel patches for kernel v5.14 Bart Van Assche
2021-05-12 3:27 ` [PATCH 1/5] RDMA/ib_hdrs.h: Remove a superfluous cast Bart Van Assche
@ 2021-05-12 3:27 ` Bart Van Assche
2021-05-19 9:01 ` Leon Romanovsky
2021-05-12 3:27 ` [PATCH 3/5] RDMA/srp: Apply the __packed attribute to members instead of structures Bart Van Assche
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2021-05-12 3:27 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Bart Van Assche,
Nicolas Morey-Chaisemartin
Before modifying how the __packed attribute is used, add compile time
size checks for the structures that will be modified.
Cc: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/infiniband/ulp/srp/ib_srp.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 31f8aa2c40ed..0f66bf015db6 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -4078,10 +4078,13 @@ static int __init srp_init_module(void)
{
int ret;
+ BUILD_BUG_ON(sizeof(struct srp_aer_req) != 36);
+ BUILD_BUG_ON(sizeof(struct srp_cmd) != 48);
BUILD_BUG_ON(sizeof(struct srp_imm_buf) != 4);
+ BUILD_BUG_ON(sizeof(struct srp_indirect_buf) != 20);
BUILD_BUG_ON(sizeof(struct srp_login_req) != 64);
BUILD_BUG_ON(sizeof(struct srp_login_req_rdma) != 56);
- BUILD_BUG_ON(sizeof(struct srp_cmd) != 48);
+ BUILD_BUG_ON(sizeof(struct srp_rsp) != 36);
if (srp_sg_tablesize) {
pr_warn("srp_sg_tablesize is deprecated, please use cmd_sg_entries\n");
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/5] RDMA/srp: Apply the __packed attribute to members instead of structures
2021-05-12 3:27 [PATCH 0/5] SRP kernel patches for kernel v5.14 Bart Van Assche
2021-05-12 3:27 ` [PATCH 1/5] RDMA/ib_hdrs.h: Remove a superfluous cast Bart Van Assche
2021-05-12 3:27 ` [PATCH 2/5] RDMA/srp: Add more structure size checks Bart Van Assche
@ 2021-05-12 3:27 ` Bart Van Assche
2021-05-20 14:48 ` Jason Gunthorpe
2021-05-12 3:27 ` [PATCH 4/5] RDMA/srp: Fix a recently introduced memory leak Bart Van Assche
2021-05-12 3:27 ` [PATCH 5/5] RDMA/srp: Make struct scsi_cmnd and struct srp_request adjacent Bart Van Assche
4 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2021-05-12 3:27 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Bart Van Assche,
Nicolas Morey-Chaisemartin
Applying the __packed attribute to an entire data structure results in
suboptimal code on architectures that do not support unaligned accesses.
Hence apply the __packed attribute only to those data members that are
not naturally aligned.
Cc: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
include/scsi/srp.h | 26 +++++++++-----------------
1 file changed, 9 insertions(+), 17 deletions(-)
diff --git a/include/scsi/srp.h b/include/scsi/srp.h
index 177d8026e96f..84d2b5fc1409 100644
--- a/include/scsi/srp.h
+++ b/include/scsi/srp.h
@@ -107,10 +107,10 @@ struct srp_direct_buf {
* having the 20-byte structure padded to 24 bytes on 64-bit architectures.
*/
struct srp_indirect_buf {
- struct srp_direct_buf table_desc;
+ struct srp_direct_buf table_desc __packed;
__be32 len;
- struct srp_direct_buf desc_list[];
-} __attribute__((packed));
+ struct srp_direct_buf desc_list[] __packed;
+};
/* Immediate data buffer descriptor as defined in SRP2. */
struct srp_imm_buf {
@@ -175,13 +175,13 @@ struct srp_login_rsp {
u8 opcode;
u8 reserved1[3];
__be32 req_lim_delta;
- u64 tag;
+ u64 tag __packed;
__be32 max_it_iu_len;
__be32 max_ti_iu_len;
__be16 buf_fmt;
u8 rsp_flags;
u8 reserved2[25];
-} __attribute__((packed));
+};
struct srp_login_rej {
u8 opcode;
@@ -207,10 +207,6 @@ struct srp_t_logout {
u64 tag;
};
-/*
- * We need the packed attribute because the SRP spec only aligns the
- * 8-byte LUN field to 4 bytes.
- */
struct srp_tsk_mgmt {
u8 opcode;
u8 sol_not;
@@ -225,10 +221,6 @@ struct srp_tsk_mgmt {
u8 reserved5[8];
};
-/*
- * We need the packed attribute because the SRP spec only aligns the
- * 8-byte LUN field to 4 bytes.
- */
struct srp_cmd {
u8 opcode;
u8 sol_not;
@@ -266,7 +258,7 @@ struct srp_rsp {
u8 sol_not;
u8 reserved1[2];
__be32 req_lim_delta;
- u64 tag;
+ u64 tag __packed;
u8 reserved2[2];
u8 flags;
u8 status;
@@ -275,7 +267,7 @@ struct srp_rsp {
__be32 sense_data_len;
__be32 resp_data_len;
u8 data[];
-} __attribute__((packed));
+};
struct srp_cred_req {
u8 opcode;
@@ -301,13 +293,13 @@ struct srp_aer_req {
u8 sol_not;
u8 reserved[2];
__be32 req_lim_delta;
- u64 tag;
+ u64 tag __packed;
u32 reserved2;
struct scsi_lun lun;
__be32 sense_data_len;
u32 reserved3;
u8 sense_data[];
-} __attribute__((packed));
+};
struct srp_aer_rsp {
u8 opcode;
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/5] RDMA/srp: Fix a recently introduced memory leak
2021-05-12 3:27 [PATCH 0/5] SRP kernel patches for kernel v5.14 Bart Van Assche
` (2 preceding siblings ...)
2021-05-12 3:27 ` [PATCH 3/5] RDMA/srp: Apply the __packed attribute to members instead of structures Bart Van Assche
@ 2021-05-12 3:27 ` Bart Van Assche
2021-05-12 8:15 ` Max Gurtovoy
2021-05-12 3:27 ` [PATCH 5/5] RDMA/srp: Make struct scsi_cmnd and struct srp_request adjacent Bart Van Assche
4 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2021-05-12 3:27 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Bart Van Assche,
Max Gurtovoy, Nicolas Morey-Chaisemartin
Only allocate an mr_list if it will be used and if it will be freed.
Cc: Max Gurtovoy <maxg@mellanox.com>
Cc: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
Fixes: f273ad4f8d90 ("RDMA/srp: Remove support for FMR memory registration")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/infiniband/ulp/srp/ib_srp.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 0f66bf015db6..52db42af421b 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1009,12 +1009,13 @@ static int srp_alloc_req_data(struct srp_rdma_ch *ch)
for (i = 0; i < target->req_ring_size; ++i) {
req = &ch->req_ring[i];
- mr_list = kmalloc_array(target->mr_per_cmd, sizeof(void *),
- GFP_KERNEL);
- if (!mr_list)
- goto out;
- if (srp_dev->use_fast_reg)
+ if (srp_dev->use_fast_reg) {
+ mr_list = kmalloc_array(target->mr_per_cmd,
+ sizeof(void *), GFP_KERNEL);
+ if (!mr_list)
+ goto out;
req->fr_list = mr_list;
+ }
req->indirect_desc = kmalloc(target->indirect_size, GFP_KERNEL);
if (!req->indirect_desc)
goto out;
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 5/5] RDMA/srp: Make struct scsi_cmnd and struct srp_request adjacent
2021-05-12 3:27 [PATCH 0/5] SRP kernel patches for kernel v5.14 Bart Van Assche
` (3 preceding siblings ...)
2021-05-12 3:27 ` [PATCH 4/5] RDMA/srp: Fix a recently introduced memory leak Bart Van Assche
@ 2021-05-12 3:27 ` Bart Van Assche
2021-05-19 9:09 ` Leon Romanovsky
4 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2021-05-12 3:27 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Bart Van Assche,
Nicolas Morey-Chaisemartin
Define .init_cmd_priv and .exit_cmd_priv callback functions in struct
scsi_host_template. Set .cmd_size such that the SCSI core allocates
per-command private data. Use scsi_cmd_priv() to access that private
data. Remove the req_ring pointer from struct srp_rdma_ch since it is
no longer necessary. Convert srp_alloc_req_data() and srp_free_req_data()
into functions that initialize one instance of the SRP-private command
data. This is a micro-optimization since this patch removes several
pointer dereferences from the hot path.
Note: due to commit e73a5e8e8003 ("scsi: core: Only return started requests
from scsi_host_find_tag()"), it is no longer necessary to protect the
completion path against duplicate responses.
Cc: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/infiniband/ulp/srp/ib_srp.c | 156 ++++++++++++----------------
drivers/infiniband/ulp/srp/ib_srp.h | 2 -
2 files changed, 65 insertions(+), 93 deletions(-)
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 52db42af421b..773ac5929082 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -965,69 +965,55 @@ static void srp_disconnect_target(struct srp_target_port *target)
}
}
-static void srp_free_req_data(struct srp_target_port *target,
- struct srp_rdma_ch *ch)
+static int srp_exit_cmd_priv(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
{
+ struct srp_target_port *target = host_to_target(shost);
struct srp_device *dev = target->srp_host->srp_dev;
struct ib_device *ibdev = dev->dev;
- struct srp_request *req;
- int i;
+ struct srp_request *req = scsi_cmd_priv(cmd);
- if (!ch->req_ring)
- return;
-
- for (i = 0; i < target->req_ring_size; ++i) {
- req = &ch->req_ring[i];
- if (dev->use_fast_reg)
- kfree(req->fr_list);
- if (req->indirect_dma_addr) {
- ib_dma_unmap_single(ibdev, req->indirect_dma_addr,
- target->indirect_size,
- DMA_TO_DEVICE);
- }
- kfree(req->indirect_desc);
+ if (dev->use_fast_reg)
+ kfree(req->fr_list);
+ if (req->indirect_dma_addr) {
+ ib_dma_unmap_single(ibdev, req->indirect_dma_addr,
+ target->indirect_size,
+ DMA_TO_DEVICE);
}
+ kfree(req->indirect_desc);
- kfree(ch->req_ring);
- ch->req_ring = NULL;
+ return 0;
}
-static int srp_alloc_req_data(struct srp_rdma_ch *ch)
+static int srp_init_cmd_priv(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
{
- struct srp_target_port *target = ch->target;
+ struct srp_target_port *target = host_to_target(shost);
struct srp_device *srp_dev = target->srp_host->srp_dev;
struct ib_device *ibdev = srp_dev->dev;
- struct srp_request *req;
+ struct srp_request *req = scsi_cmd_priv(cmd);
void *mr_list;
dma_addr_t dma_addr;
- int i, ret = -ENOMEM;
-
- ch->req_ring = kcalloc(target->req_ring_size, sizeof(*ch->req_ring),
- GFP_KERNEL);
- if (!ch->req_ring)
- goto out;
+ int ret = -ENOMEM;
- for (i = 0; i < target->req_ring_size; ++i) {
- req = &ch->req_ring[i];
- if (srp_dev->use_fast_reg) {
- mr_list = kmalloc_array(target->mr_per_cmd,
- sizeof(void *), GFP_KERNEL);
- if (!mr_list)
- goto out;
- req->fr_list = mr_list;
- }
- req->indirect_desc = kmalloc(target->indirect_size, GFP_KERNEL);
- if (!req->indirect_desc)
- goto out;
-
- dma_addr = ib_dma_map_single(ibdev, req->indirect_desc,
- target->indirect_size,
- DMA_TO_DEVICE);
- if (ib_dma_mapping_error(ibdev, dma_addr))
+ if (srp_dev->use_fast_reg) {
+ mr_list = kmalloc_array(target->mr_per_cmd, sizeof(void *),
+ GFP_KERNEL);
+ if (!mr_list)
goto out;
+ req->fr_list = mr_list;
+ }
+ req->indirect_desc = kmalloc(target->indirect_size, GFP_KERNEL);
+ if (!req->indirect_desc)
+ goto out;
- req->indirect_dma_addr = dma_addr;
+ dma_addr = ib_dma_map_single(ibdev, req->indirect_desc,
+ target->indirect_size,
+ DMA_TO_DEVICE);
+ if (ib_dma_mapping_error(ibdev, dma_addr)) {
+ srp_exit_cmd_priv(shost, cmd);
+ goto out;
}
+
+ req->indirect_dma_addr = dma_addr;
ret = 0;
out:
@@ -1069,10 +1055,6 @@ static void srp_remove_target(struct srp_target_port *target)
}
cancel_work_sync(&target->tl_err_work);
srp_rport_put(target->rport);
- for (i = 0; i < target->ch_count; i++) {
- ch = &target->ch[i];
- srp_free_req_data(target, ch);
- }
kfree(target->ch);
target->ch = NULL;
@@ -1291,22 +1273,32 @@ static void srp_finish_req(struct srp_rdma_ch *ch, struct srp_request *req,
}
}
-static void srp_terminate_io(struct srp_rport *rport)
+struct srp_terminate_context {
+ struct srp_target_port *srp_target;
+ int scsi_result;
+};
+
+static bool srp_terminate_cmd(struct scsi_cmnd *scmnd, void *context_ptr,
+ bool reserved)
{
- struct srp_target_port *target = rport->lld_data;
- struct srp_rdma_ch *ch;
- int i, j;
+ struct srp_terminate_context *context = context_ptr;
+ struct srp_target_port *target = context->srp_target;
+ u32 tag = blk_mq_unique_tag(scmnd->request);
+ struct srp_rdma_ch *ch = &target->ch[blk_mq_unique_tag_to_hwq(tag)];
+ struct srp_request *req = scsi_cmd_priv(scmnd);
- for (i = 0; i < target->ch_count; i++) {
- ch = &target->ch[i];
+ srp_finish_req(ch, req, NULL, context->scsi_result);
- for (j = 0; j < target->req_ring_size; ++j) {
- struct srp_request *req = &ch->req_ring[j];
+ return true;
+}
- srp_finish_req(ch, req, NULL,
- DID_TRANSPORT_FAILFAST << 16);
- }
- }
+static void srp_terminate_io(struct srp_rport *rport)
+{
+ struct srp_target_port *target = rport->lld_data;
+ struct srp_terminate_context context = { .srp_target = target,
+ .scsi_result = DID_TRANSPORT_FAILFAST << 16 };
+
+ scsi_host_busy_iter(target->scsi_host, srp_terminate_cmd, &context);
}
/* Calculate maximum initiator to target information unit length. */
@@ -1362,13 +1354,12 @@ static int srp_rport_reconnect(struct srp_rport *rport)
ch = &target->ch[i];
ret += srp_new_cm_id(ch);
}
- for (i = 0; i < target->ch_count; i++) {
- ch = &target->ch[i];
- for (j = 0; j < target->req_ring_size; ++j) {
- struct srp_request *req = &ch->req_ring[j];
+ {
+ struct srp_terminate_context context = {
+ .srp_target = target, .scsi_result = DID_RESET << 16};
- srp_finish_req(ch, req, NULL, DID_RESET << 16);
- }
+ scsi_host_busy_iter(target->scsi_host, srp_terminate_cmd,
+ &context);
}
for (i = 0; i < target->ch_count; i++) {
ch = &target->ch[i];
@@ -1964,13 +1955,10 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp)
spin_unlock_irqrestore(&ch->lock, flags);
} else {
scmnd = scsi_host_find_tag(target->scsi_host, rsp->tag);
- if (scmnd && scmnd->host_scribble) {
- req = (void *)scmnd->host_scribble;
+ if (scmnd) {
+ req = scsi_cmd_priv(scmnd);
scmnd = srp_claim_req(ch, req, NULL, scmnd);
} else {
- scmnd = NULL;
- }
- if (!scmnd) {
shost_printk(KERN_ERR, target->scsi_host,
"Null scmnd for RSP w/tag %#016llx received on ch %td / QP %#x\n",
rsp->tag, ch - target->ch, ch->qp->qp_num);
@@ -2002,7 +1990,6 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp)
srp_free_req(ch, req, scmnd,
be32_to_cpu(rsp->req_lim_delta));
- scmnd->host_scribble = NULL;
scmnd->scsi_done(scmnd);
}
}
@@ -2170,13 +2157,12 @@ static int srp_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *scmnd)
{
struct srp_target_port *target = host_to_target(shost);
struct srp_rdma_ch *ch;
- struct srp_request *req;
+ struct srp_request *req = scsi_cmd_priv(scmnd);
struct srp_iu *iu;
struct srp_cmd *cmd;
struct ib_device *dev;
unsigned long flags;
u32 tag;
- u16 idx;
int len, ret;
scmnd->result = srp_chkready(target->rport);
@@ -2186,10 +2172,6 @@ static int srp_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *scmnd)
WARN_ON_ONCE(scmnd->request->tag < 0);
tag = blk_mq_unique_tag(scmnd->request);
ch = &target->ch[blk_mq_unique_tag_to_hwq(tag)];
- idx = blk_mq_unique_tag_to_tag(tag);
- WARN_ONCE(idx >= target->req_ring_size, "%s: tag %#x: idx %d >= %d\n",
- dev_name(&shost->shost_gendev), tag, idx,
- target->req_ring_size);
spin_lock_irqsave(&ch->lock, flags);
iu = __srp_get_tx_iu(ch, SRP_IU_CMD);
@@ -2198,13 +2180,10 @@ static int srp_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *scmnd)
if (!iu)
goto err;
- req = &ch->req_ring[idx];
dev = target->srp_host->srp_dev->dev;
ib_dma_sync_single_for_cpu(dev, iu->dma, ch->max_it_iu_len,
DMA_TO_DEVICE);
- scmnd->host_scribble = (void *) req;
-
cmd = iu->buf;
memset(cmd, 0, sizeof *cmd);
@@ -3085,6 +3064,8 @@ static struct scsi_host_template srp_template = {
.target_alloc = srp_target_alloc,
.slave_configure = srp_slave_configure,
.info = srp_target_info,
+ .init_cmd_priv = srp_init_cmd_priv,
+ .exit_cmd_priv = srp_exit_cmd_priv,
.queuecommand = srp_queuecommand,
.change_queue_depth = srp_change_queue_depth,
.eh_timed_out = srp_timed_out,
@@ -3098,6 +3079,7 @@ static struct scsi_host_template srp_template = {
.cmd_per_lun = SRP_DEFAULT_CMD_SQ_SIZE,
.shost_attrs = srp_host_attrs,
.track_queue_depth = 1,
+ .cmd_size = sizeof(struct srp_request),
};
static int srp_sdev_count(struct Scsi_Host *host)
@@ -3677,8 +3659,6 @@ static ssize_t srp_create_target(struct device *dev,
if (ret)
goto out;
- target->req_ring_size = target->queue_size - SRP_TSK_MGMT_SQ_SIZE;
-
if (!srp_conn_unique(target->srp_host, target)) {
if (target->using_rdma_cm) {
shost_printk(KERN_INFO, target->scsi_host,
@@ -3781,10 +3761,6 @@ static ssize_t srp_create_target(struct device *dev,
if (ret)
goto err_disconnect;
- ret = srp_alloc_req_data(ch);
- if (ret)
- goto err_disconnect;
-
ret = srp_connect_ch(ch, max_iu_len, multich);
if (ret) {
char dst[64];
@@ -3803,7 +3779,6 @@ static ssize_t srp_create_target(struct device *dev,
goto free_ch;
} else {
srp_free_ch_ib(target, ch);
- srp_free_req_data(target, ch);
target->ch_count = ch - target->ch;
goto connected;
}
@@ -3864,7 +3839,6 @@ static ssize_t srp_create_target(struct device *dev,
for (i = 0; i < target->ch_count; i++) {
ch = &target->ch[i];
srp_free_ch_ib(target, ch);
- srp_free_req_data(target, ch);
}
kfree(target->ch);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 6818cac0a3b7..abccddeea1e3 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -174,7 +174,6 @@ struct srp_rdma_ch {
struct srp_iu **tx_ring;
struct srp_iu **rx_ring;
- struct srp_request *req_ring;
int comp_vector;
u64 tsk_mgmt_tag;
@@ -220,7 +219,6 @@ struct srp_target_port {
int mr_pool_size;
int mr_per_cmd;
int queue_size;
- int req_ring_size;
int comp_vector;
int tl_retry_count;
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] RDMA/srp: Fix a recently introduced memory leak
2021-05-12 3:27 ` [PATCH 4/5] RDMA/srp: Fix a recently introduced memory leak Bart Van Assche
@ 2021-05-12 8:15 ` Max Gurtovoy
2021-05-12 16:14 ` Bart Van Assche
0 siblings, 1 reply; 17+ messages in thread
From: Max Gurtovoy @ 2021-05-12 8:15 UTC (permalink / raw)
To: Bart Van Assche, Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Max Gurtovoy,
Nicolas Morey-Chaisemartin
On 5/12/2021 6:27 AM, Bart Van Assche wrote:
> Only allocate an mr_list if it will be used and if it will be freed.
>
> Cc: Max Gurtovoy <maxg@mellanox.com>
> Cc: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
> Fixes: f273ad4f8d90 ("RDMA/srp: Remove support for FMR memory registration")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/infiniband/ulp/srp/ib_srp.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 0f66bf015db6..52db42af421b 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1009,12 +1009,13 @@ static int srp_alloc_req_data(struct srp_rdma_ch *ch)
>
> for (i = 0; i < target->req_ring_size; ++i) {
> req = &ch->req_ring[i];
> - mr_list = kmalloc_array(target->mr_per_cmd, sizeof(void *),
> - GFP_KERNEL);
> - if (!mr_list)
> - goto out;
> - if (srp_dev->use_fast_reg)
> + if (srp_dev->use_fast_reg) {
> + mr_list = kmalloc_array(target->mr_per_cmd,
> + sizeof(void *), GFP_KERNEL);
> + if (!mr_list)
> + goto out;
> req->fr_list = mr_list;
> + }
> req->indirect_desc = kmalloc(target->indirect_size, GFP_KERNEL);
> if (!req->indirect_desc)
> goto out;
Looks good,
maybe we can remove the local mr_list variable while we're here ?
otherwise,
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] RDMA/srp: Fix a recently introduced memory leak
2021-05-12 8:15 ` Max Gurtovoy
@ 2021-05-12 16:14 ` Bart Van Assche
0 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2021-05-12 16:14 UTC (permalink / raw)
To: Max Gurtovoy, Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Max Gurtovoy,
Nicolas Morey-Chaisemartin
On 5/12/21 1:15 AM, Max Gurtovoy wrote:
> maybe we can remove the local mr_list variable while we're here ?
>
> otherwise,
>
> Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Hi Max,
I will remove the mr_list variable if I have to repost this patch series.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/5] RDMA/ib_hdrs.h: Remove a superfluous cast
2021-05-12 3:27 ` [PATCH 1/5] RDMA/ib_hdrs.h: Remove a superfluous cast Bart Van Assche
@ 2021-05-19 9:00 ` Leon Romanovsky
2021-05-24 3:12 ` Bart Van Assche
0 siblings, 1 reply; 17+ messages in thread
From: Leon Romanovsky @ 2021-05-19 9:00 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jason Gunthorpe, Doug Ledford, linux-rdma, Don Hiatt, Dennis Dalessandro
On Tue, May 11, 2021 at 08:27:48PM -0700, Bart Van Assche wrote:
> be16_to_cpu() returns a u16 value. Remove the superfluous u16 cast. That
> cast was introduced by commit 7dafbab3753f ("IB/hfi1: Add functions to
> parse BTH/IB headers").
>
> Cc: Don Hiatt <don.hiatt@intel.com>
> Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> include/rdma/ib_hdrs.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/rdma/ib_hdrs.h b/include/rdma/ib_hdrs.h
> index 57c1ac881d08..82483120539f 100644
> --- a/include/rdma/ib_hdrs.h
> +++ b/include/rdma/ib_hdrs.h
> @@ -208,7 +208,7 @@ static inline u8 ib_get_lver(struct ib_header *hdr)
>
> static inline u16 ib_get_len(struct ib_header *hdr)
> {
> - return (u16)(be16_to_cpu(hdr->lrh[2]));
> + return be16_to_cpu(hdr->lrh[2]);
> }
>
> static inline u32 ib_get_qkey(struct ib_other_headers *ohdr)
It is unclear why this function in the header. It is called only once.
Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] RDMA/srp: Add more structure size checks
2021-05-12 3:27 ` [PATCH 2/5] RDMA/srp: Add more structure size checks Bart Van Assche
@ 2021-05-19 9:01 ` Leon Romanovsky
0 siblings, 0 replies; 17+ messages in thread
From: Leon Romanovsky @ 2021-05-19 9:01 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jason Gunthorpe, Doug Ledford, linux-rdma, Nicolas Morey-Chaisemartin
On Tue, May 11, 2021 at 08:27:49PM -0700, Bart Van Assche wrote:
> Before modifying how the __packed attribute is used, add compile time
> size checks for the structures that will be modified.
>
> Cc: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/infiniband/ulp/srp/ib_srp.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] RDMA/srp: Make struct scsi_cmnd and struct srp_request adjacent
2021-05-12 3:27 ` [PATCH 5/5] RDMA/srp: Make struct scsi_cmnd and struct srp_request adjacent Bart Van Assche
@ 2021-05-19 9:09 ` Leon Romanovsky
2021-05-19 15:13 ` Bart Van Assche
0 siblings, 1 reply; 17+ messages in thread
From: Leon Romanovsky @ 2021-05-19 9:09 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jason Gunthorpe, Doug Ledford, linux-rdma, Nicolas Morey-Chaisemartin
On Tue, May 11, 2021 at 08:27:52PM -0700, Bart Van Assche wrote:
> Define .init_cmd_priv and .exit_cmd_priv callback functions in struct
> scsi_host_template. Set .cmd_size such that the SCSI core allocates
> per-command private data. Use scsi_cmd_priv() to access that private
> data. Remove the req_ring pointer from struct srp_rdma_ch since it is
> no longer necessary. Convert srp_alloc_req_data() and srp_free_req_data()
> into functions that initialize one instance of the SRP-private command
> data. This is a micro-optimization since this patch removes several
> pointer dereferences from the hot path.
>
> Note: due to commit e73a5e8e8003 ("scsi: core: Only return started requests
> from scsi_host_find_tag()"), it is no longer necessary to protect the
> completion path against duplicate responses.
>
> Cc: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/infiniband/ulp/srp/ib_srp.c | 156 ++++++++++++----------------
> drivers/infiniband/ulp/srp/ib_srp.h | 2 -
> 2 files changed, 65 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 52db42af421b..773ac5929082 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -965,69 +965,55 @@ static void srp_disconnect_target(struct srp_target_port *target)
> }
> }
>
> -static void srp_free_req_data(struct srp_target_port *target,
> - struct srp_rdma_ch *ch)
> +static int srp_exit_cmd_priv(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
> {
> + struct srp_target_port *target = host_to_target(shost);
> struct srp_device *dev = target->srp_host->srp_dev;
> struct ib_device *ibdev = dev->dev;
> - struct srp_request *req;
> - int i;
> + struct srp_request *req = scsi_cmd_priv(cmd);
>
> - if (!ch->req_ring)
> - return;
> -
> - for (i = 0; i < target->req_ring_size; ++i) {
> - req = &ch->req_ring[i];
> - if (dev->use_fast_reg)
> - kfree(req->fr_list);
> - if (req->indirect_dma_addr) {
> - ib_dma_unmap_single(ibdev, req->indirect_dma_addr,
> - target->indirect_size,
> - DMA_TO_DEVICE);
> - }
> - kfree(req->indirect_desc);
> + if (dev->use_fast_reg)
> + kfree(req->fr_list);
Isn't cleaner will be to ensure that fr_list is NULL for !dev->use_fast_reg path?
In patch #4 https://lore.kernel.org/linux-rdma/20210512032752.16611-5-bvanassche@acm.org
Thanks
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] RDMA/srp: Make struct scsi_cmnd and struct srp_request adjacent
2021-05-19 9:09 ` Leon Romanovsky
@ 2021-05-19 15:13 ` Bart Van Assche
2021-05-19 15:32 ` Leon Romanovsky
0 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2021-05-19 15:13 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Jason Gunthorpe, Doug Ledford, linux-rdma, Nicolas Morey-Chaisemartin
On 5/19/21 2:09 AM, Leon Romanovsky wrote:
> On Tue, May 11, 2021 at 08:27:52PM -0700, Bart Van Assche wrote:
>> -static void srp_free_req_data(struct srp_target_port *target,
>> - struct srp_rdma_ch *ch)
>> +static int srp_exit_cmd_priv(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
>> {
>> + struct srp_target_port *target = host_to_target(shost);
>> struct srp_device *dev = target->srp_host->srp_dev;
>> struct ib_device *ibdev = dev->dev;
>> - struct srp_request *req;
>> - int i;
>> + struct srp_request *req = scsi_cmd_priv(cmd);
>>
>> - if (!ch->req_ring)
>> - return;
>> -
>> - for (i = 0; i < target->req_ring_size; ++i) {
>> - req = &ch->req_ring[i];
>> - if (dev->use_fast_reg)
>> - kfree(req->fr_list);
>> - if (req->indirect_dma_addr) {
>> - ib_dma_unmap_single(ibdev, req->indirect_dma_addr,
>> - target->indirect_size,
>> - DMA_TO_DEVICE);
>> - }
>> - kfree(req->indirect_desc);
>> + if (dev->use_fast_reg)
>> + kfree(req->fr_list);
>
> Isn't cleaner will be to ensure that fr_list is NULL for !dev->use_fast_reg path?
> In patch #4 https://lore.kernel.org/linux-rdma/20210512032752.16611-5-bvanassche@acm.org
Hi Leon,
I think that per-request private data is zero-initialized and hence that
it is not necessary to clear req->fr_list explicitly. blk_mq_alloc_rqs()
passes __GFP_ZERO to alloc_pages_node(). blk_mq_alloc_rqs() does not
only allocate block layer requests (struct request) but also per-request
private data (set->cmd_size).
Thanks,
Bart.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] RDMA/srp: Make struct scsi_cmnd and struct srp_request adjacent
2021-05-19 15:13 ` Bart Van Assche
@ 2021-05-19 15:32 ` Leon Romanovsky
0 siblings, 0 replies; 17+ messages in thread
From: Leon Romanovsky @ 2021-05-19 15:32 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jason Gunthorpe, Doug Ledford, linux-rdma, Nicolas Morey-Chaisemartin
On Wed, May 19, 2021 at 08:13:09AM -0700, Bart Van Assche wrote:
> On 5/19/21 2:09 AM, Leon Romanovsky wrote:
> > On Tue, May 11, 2021 at 08:27:52PM -0700, Bart Van Assche wrote:
> >> -static void srp_free_req_data(struct srp_target_port *target,
> >> - struct srp_rdma_ch *ch)
> >> +static int srp_exit_cmd_priv(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
> >> {
> >> + struct srp_target_port *target = host_to_target(shost);
> >> struct srp_device *dev = target->srp_host->srp_dev;
> >> struct ib_device *ibdev = dev->dev;
> >> - struct srp_request *req;
> >> - int i;
> >> + struct srp_request *req = scsi_cmd_priv(cmd);
> >>
> >> - if (!ch->req_ring)
> >> - return;
> >> -
> >> - for (i = 0; i < target->req_ring_size; ++i) {
> >> - req = &ch->req_ring[i];
> >> - if (dev->use_fast_reg)
> >> - kfree(req->fr_list);
> >> - if (req->indirect_dma_addr) {
> >> - ib_dma_unmap_single(ibdev, req->indirect_dma_addr,
> >> - target->indirect_size,
> >> - DMA_TO_DEVICE);
> >> - }
> >> - kfree(req->indirect_desc);
> >> + if (dev->use_fast_reg)
> >> + kfree(req->fr_list);
> >
> > Isn't cleaner will be to ensure that fr_list is NULL for !dev->use_fast_reg path?
> > In patch #4 https://lore.kernel.org/linux-rdma/20210512032752.16611-5-bvanassche@acm.org
>
> Hi Leon,
>
> I think that per-request private data is zero-initialized and hence that
> it is not necessary to clear req->fr_list explicitly. blk_mq_alloc_rqs()
> passes __GFP_ZERO to alloc_pages_node(). blk_mq_alloc_rqs() does not
> only allocate block layer requests (struct request) but also per-request
> private data (set->cmd_size).
So you don't need this "if (dev->use_fast_reg)" check.
Thanks
>
> Thanks,
>
> Bart.
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/5] RDMA/srp: Apply the __packed attribute to members instead of structures
2021-05-12 3:27 ` [PATCH 3/5] RDMA/srp: Apply the __packed attribute to members instead of structures Bart Van Assche
@ 2021-05-20 14:48 ` Jason Gunthorpe
2021-05-24 3:41 ` Bart Van Assche
0 siblings, 1 reply; 17+ messages in thread
From: Jason Gunthorpe @ 2021-05-20 14:48 UTC (permalink / raw)
To: Bart Van Assche
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Nicolas Morey-Chaisemartin
On Tue, May 11, 2021 at 08:27:50PM -0700, Bart Van Assche wrote:
> @@ -107,10 +107,10 @@ struct srp_direct_buf {
> * having the 20-byte structure padded to 24 bytes on 64-bit architectures.
> */
> struct srp_indirect_buf {
> - struct srp_direct_buf table_desc;
> + struct srp_direct_buf table_desc __packed;
> __be32 len;
> - struct srp_direct_buf desc_list[];
> -} __attribute__((packed));
> + struct srp_direct_buf desc_list[] __packed;
> +};
>
> /* Immediate data buffer descriptor as defined in SRP2. */
> struct srp_imm_buf {
> @@ -175,13 +175,13 @@ struct srp_login_rsp {
> u8 opcode;
> u8 reserved1[3];
> __be32 req_lim_delta;
> - u64 tag;
> + u64 tag __packed;
What you really want is just something like this:
typedef u64 __attribute__((aligned(4))) compat_u64;
And then use that every place you have the u64 and forget about packed
entirely.
Except for a couple exceptions IBA mads are always aligned to 4 bytes,
only the 64 bit quantities are unaligned.
But really this whole thing should be replaced with the IBA_FIELD
macros like include/rdma/ibta_vol1_c12.h demos.
Then it would be sparse safe and obviously endian correct as well.
I suppose you are respinning this due to the other comments?
Jason
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/5] RDMA/ib_hdrs.h: Remove a superfluous cast
2021-05-19 9:00 ` Leon Romanovsky
@ 2021-05-24 3:12 ` Bart Van Assche
0 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2021-05-24 3:12 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Jason Gunthorpe, Doug Ledford, linux-rdma, Don Hiatt, Dennis Dalessandro
On 5/19/21 2:00 AM, Leon Romanovsky wrote:
> On Tue, May 11, 2021 at 08:27:48PM -0700, Bart Van Assche wrote:
>> diff --git a/include/rdma/ib_hdrs.h b/include/rdma/ib_hdrs.h
>> index 57c1ac881d08..82483120539f 100644
>> --- a/include/rdma/ib_hdrs.h
>> +++ b/include/rdma/ib_hdrs.h
>> @@ -208,7 +208,7 @@ static inline u8 ib_get_lver(struct ib_header *hdr)
>>
>> static inline u16 ib_get_len(struct ib_header *hdr)
>> {
>> - return (u16)(be16_to_cpu(hdr->lrh[2]));
>> + return be16_to_cpu(hdr->lrh[2]);
>> }
>>
>> static inline u32 ib_get_qkey(struct ib_other_headers *ohdr)
>
> It is unclear why this function in the header. It is called only once.
That's a good point. I will move it into the .c file that uses this
function.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/5] RDMA/srp: Apply the __packed attribute to members instead of structures
2021-05-20 14:48 ` Jason Gunthorpe
@ 2021-05-24 3:41 ` Bart Van Assche
2021-05-24 23:00 ` Jason Gunthorpe
0 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2021-05-24 3:41 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Nicolas Morey-Chaisemartin
On 5/20/21 7:48 AM, Jason Gunthorpe wrote:
> On Tue, May 11, 2021 at 08:27:50PM -0700, Bart Van Assche wrote:
>> @@ -107,10 +107,10 @@ struct srp_direct_buf {
>> * having the 20-byte structure padded to 24 bytes on 64-bit architectures.
>> */
>> struct srp_indirect_buf {
>> - struct srp_direct_buf table_desc;
>> + struct srp_direct_buf table_desc __packed;
>> __be32 len;
>> - struct srp_direct_buf desc_list[];
>> -} __attribute__((packed));
>> + struct srp_direct_buf desc_list[] __packed;
>> +};
>>
>> /* Immediate data buffer descriptor as defined in SRP2. */
>> struct srp_imm_buf {
>> @@ -175,13 +175,13 @@ struct srp_login_rsp {
>> u8 opcode;
>> u8 reserved1[3];
>> __be32 req_lim_delta;
>> - u64 tag;
>> + u64 tag __packed;
>
> What you really want is just something like this:
>
> typedef u64 __attribute__((aligned(4))) compat_u64;
>
> And then use that every place you have the u64 and forget about packed
> entirely.
Really? My understanding is that the aligned attribute can be used to
increase alignment of a structure member but not to decrease it. From
https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes:
"When used on a struct, or struct member, the aligned attribute can only
increase the alignment; in order to decrease it, the packed attribute
must be specified as well."
Additionally, there is a recommendation in
Documentation/process/coding-style.rst not to introduce new typedefs.
> Except for a couple exceptions IBA mads are always aligned to 4 bytes,
> only the 64 bit quantities are unaligned.
>
> But really this whole thing should be replaced with the IBA_FIELD
> macros like include/rdma/ibta_vol1_c12.h demos.
>
> Then it would be sparse safe and obviously endian correct as well.
I prefer a structure over the IBA_FIELD macros so I will change __packed
into __packed __aligned(4).
Thanks,
Bart.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/5] RDMA/srp: Apply the __packed attribute to members instead of structures
2021-05-24 3:41 ` Bart Van Assche
@ 2021-05-24 23:00 ` Jason Gunthorpe
0 siblings, 0 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2021-05-24 23:00 UTC (permalink / raw)
To: Bart Van Assche
Cc: Leon Romanovsky, Doug Ledford, linux-rdma, Nicolas Morey-Chaisemartin
On Sun, May 23, 2021 at 08:41:25PM -0700, Bart Van Assche wrote:
> On 5/20/21 7:48 AM, Jason Gunthorpe wrote:
> > On Tue, May 11, 2021 at 08:27:50PM -0700, Bart Van Assche wrote:
> >> @@ -107,10 +107,10 @@ struct srp_direct_buf {
> >> * having the 20-byte structure padded to 24 bytes on 64-bit architectures.
> >> */
> >> struct srp_indirect_buf {
> >> - struct srp_direct_buf table_desc;
> >> + struct srp_direct_buf table_desc __packed;
> >> __be32 len;
> >> - struct srp_direct_buf desc_list[];
> >> -} __attribute__((packed));
> >> + struct srp_direct_buf desc_list[] __packed;
> >> +};
> >>
> >> /* Immediate data buffer descriptor as defined in SRP2. */
> >> struct srp_imm_buf {
> >> @@ -175,13 +175,13 @@ struct srp_login_rsp {
> >> u8 opcode;
> >> u8 reserved1[3];
> >> __be32 req_lim_delta;
> >> - u64 tag;
> >> + u64 tag __packed;
> >
> > What you really want is just something like this:
> >
> > typedef u64 __attribute__((aligned(4))) compat_u64;
> >
> > And then use that every place you have the u64 and forget about packed
> > entirely.
>
> Really? My understanding is that the aligned attribute can be used to
> increase alignment of a structure member but not to decrease it.
I didn't test it, but that is pre-existing code in Linux.. Maybe it
doesn't work and/or needs packed too
> Additionally, there is a recommendation in
> Documentation/process/coding-style.rst not to introduce new typedefs.
That we tend to selective ignore <shrug>
> > Except for a couple exceptions IBA mads are always aligned to 4 bytes,
> > only the 64 bit quantities are unaligned.
> >
> > But really this whole thing should be replaced with the IBA_FIELD
> > macros like include/rdma/ibta_vol1_c12.h demos.
> >
> > Then it would be sparse safe and obviously endian correct as well.
>
> I prefer a structure over the IBA_FIELD macros so I will change __packed
> into __packed __aligned(4).
IMHO the struct is alot worse due to lack of endian safety and
complexity in establishing the layout.
Jason
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-05-24 23:00 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-12 3:27 [PATCH 0/5] SRP kernel patches for kernel v5.14 Bart Van Assche
2021-05-12 3:27 ` [PATCH 1/5] RDMA/ib_hdrs.h: Remove a superfluous cast Bart Van Assche
2021-05-19 9:00 ` Leon Romanovsky
2021-05-24 3:12 ` Bart Van Assche
2021-05-12 3:27 ` [PATCH 2/5] RDMA/srp: Add more structure size checks Bart Van Assche
2021-05-19 9:01 ` Leon Romanovsky
2021-05-12 3:27 ` [PATCH 3/5] RDMA/srp: Apply the __packed attribute to members instead of structures Bart Van Assche
2021-05-20 14:48 ` Jason Gunthorpe
2021-05-24 3:41 ` Bart Van Assche
2021-05-24 23:00 ` Jason Gunthorpe
2021-05-12 3:27 ` [PATCH 4/5] RDMA/srp: Fix a recently introduced memory leak Bart Van Assche
2021-05-12 8:15 ` Max Gurtovoy
2021-05-12 16:14 ` Bart Van Assche
2021-05-12 3:27 ` [PATCH 5/5] RDMA/srp: Make struct scsi_cmnd and struct srp_request adjacent Bart Van Assche
2021-05-19 9:09 ` Leon Romanovsky
2021-05-19 15:13 ` Bart Van Assche
2021-05-19 15:32 ` Leon Romanovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.