On 8/2/2022 10:20 PM, Namjae Jeon wrote:
In Soft-iWARP, smbdirect does not work in cifs client.
The hardcoding max_sge is large in cifs, but need smaller value for
soft-iWARP. Add SMBDIRECT_MIN_SGE macro as 6 and use the max_sge
the hw reports instead of hardcoding 16 sge's.
There is no issue in SoftiWARP, the bug is in ksmbd, so I think
the message is incorrect. May I suggest:
"Use a more appropriate max_sge, and ensure it does not exceed the
RDMA provider's maximum. This enables ksmbd to function on
SoftiWARP, among potentially others."
More comments below.
Cc: Tom Talpey <tom@xxxxxxxxxx>
Cc: David Howells <dhowells@xxxxxxxxxx>
Cc: Hyunchul Lee <hyc.lee@xxxxxxxxx>
Cc: Long Li <longli@xxxxxxxxxxxxx>
Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
---
fs/cifs/smbdirect.c | 15 ++++++++++-----
fs/cifs/smbdirect.h | 3 ++-
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index 5fbbec22bcc8..bb68702362f7 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -1518,7 +1518,7 @@ static int allocate_caches_and_workqueue(struct smbd_connection *info)
static struct smbd_connection *_smbd_get_connection(
struct TCP_Server_Info *server, struct sockaddr *dstaddr, int port)
{
- int rc;
+ int rc, max_sge;
struct smbd_connection *info;
struct rdma_conn_param conn_param;
struct ib_qp_init_attr qp_attr;
@@ -1562,13 +1562,13 @@ static struct smbd_connection *_smbd_get_connection(
info->max_receive_size = smbd_max_receive_size;
info->keep_alive_interval = smbd_keep_alive_interval;
- if (info->id->device->attrs.max_send_sge < SMBDIRECT_MAX_SGE) {
+ if (info->id->device->attrs.max_send_sge < SMBDIRECT_MIN_SGE) {
log_rdma_event(ERR,
"warning: device max_send_sge = %d too small\n",
info->id->device->attrs.max_send_sge);
log_rdma_event(ERR, "Queue Pair creation may fail\n");
}
- if (info->id->device->attrs.max_recv_sge < SMBDIRECT_MAX_SGE) {
+ if (info->id->device->attrs.max_recv_sge < SMBDIRECT_MIN_SGE) {
log_rdma_event(ERR,
"warning: device max_recv_sge = %d too small\n",
info->id->device->attrs.max_recv_sge);
@@ -1593,13 +1593,18 @@ static struct smbd_connection *_smbd_get_connection(
goto alloc_cq_failed;
}
Why are the two conditions treated differently? It prints a rather
vague warning if the send sge is exceeded, but it fails if the
receive sge is too large.
I suggest failing fast in both cases, but the code gives no way
for the user to correct the situation, SMBDIRECT_MIN_SGE is a
hardcoded constant. That's the bug
IIRC, the ksmbd code requires 3 send sge's for send, and it needs
5 sge's when SMB3_TRANSFORM is needed. Why not provide a variable
sge limit, depending on the session's requirement?
+ max_sge = min3(info->id->device->attrs.max_send_sge,
+ info->id->device->attrs.max_recv_sge,
+ SMBDIRECT_MAX_SGE);
+ max_sge = max(max_sge, SMBDIRECT_MIN_SGE);
This is inaccurate. ksmbd's send sge requirement is not necessarily
the same as its receive sge, likewise the RDMA provider's limit.
There is no reason to limit one by the other, and they should be
calculated independently.
What is the ksmbd receive sge requirement? Is it variable, like
the send, depending on what protocol features are needed?
+
memset(&qp_attr, 0, sizeof(qp_attr));
qp_attr.event_handler = smbd_qp_async_error_upcall;
qp_attr.qp_context = info;
qp_attr.cap.max_send_wr = info->send_credit_target;
qp_attr.cap.max_recv_wr = info->receive_credit_max;
- qp_attr.cap.max_send_sge = SMBDIRECT_MAX_SGE;
- qp_attr.cap.max_recv_sge = SMBDIRECT_MAX_SGE;
+ qp_attr.cap.max_send_sge = max_sge;
+ qp_attr.cap.max_recv_sge = max_sge;
See previous comment.
qp_attr.cap.max_inline_data = 0;
qp_attr.sq_sig_type = IB_SIGNAL_REQ_WR;
qp_attr.qp_type = IB_QPT_RC;
diff --git a/fs/cifs/smbdirect.h b/fs/cifs/smbdirect.h
index a87fca82a796..8b81301e4d4c 100644
--- a/fs/cifs/smbdirect.h
+++ b/fs/cifs/smbdirect.h
@@ -225,7 +225,8 @@ struct smbd_buffer_descriptor_v1 {
__le32 length;
} __packed;
-/* Default maximum number of SGEs in a RDMA send/recv */
+/* Default maximum/minimum number of SGEs in a RDMA send/recv */
+#define SMBDIRECT_MIN_SGE 6
See previous comment, and also, please justify the "6".
David Howells commented it appears to be "5", at least for send.
I think with a small refactoring to allocate a more flexible header
buffer, it could be even smaller.
I would hope the value for receive is "2", or less. But I haven't
looked very deeply yet.
With sge's and an RDMA provider, the smaller the better. The adapter
will always be more efficient in processing work requests. So doing
this right is beneficial in many ways.
#define SMBDIRECT_MAX_SGE 16
While we're at it, please justify "16". Will ksmbd ever need so many?
/* The context for a SMBD request */
struct smbd_request {
Tom.
