2022-08-03 23:34 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>:
Hi Tom,
> Oops, I typed "ksmbd" below, I meant "smbdirect client".
> However, fewer sge's are always better, and ksmbd may
> well have different requirements than "cifs", making a
> hardcoded value even less appropriate.
Yes. Agreed. but I don't know if I can properly answer your questions.
I have not looked deeply into the cifs smbdirect code, so I requested
Long who is an author of smbdirect in cifs for a proper fix before.
And it's still there. I just wrote a stopgap fix patch as on David's
request. I expect Long or someone else with a deep look into cifs and
smbdirect will give the right solution later.
Thanks!
>
> On 8/3/2022 10:26 AM, Tom Talpey wrote:
>> 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.
>>
>
