Re: [PATCH] cifs: smbdirect: use the max_sge the hw reports

[Tom Talpey <tom@talpey.com>]

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.

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@talpey.com>
>> Cc: David Howells <dhowells@redhat.com>
>> Cc: Hyunchul Lee <hyc.lee@gmail.com>
>> Cc: Long Li <longli@microsoft.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   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.
>