| Message ID | 20220803022042.10543-1-linkinjeon@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: smbdirect: use the max_sge the hw reports | expand |
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.
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. >
2022-08-03 23:34 GMT+09:00, Tom Talpey <tom@talpey.com>: 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@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. >> >
On 8/4/2022 1:58 AM, Namjae Jeon wrote: > 2022-08-03 23:34 GMT+09:00, Tom Talpey <tom@talpey.com>: > 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. Ok, well, I'll do it. The patch is a NAK from me, because it adds new, and incorrect, logic. If enabling SoftiWARP is the main goal, then simply one line would fix this: #define SMBDIRECT_MAX_SGE 5 > 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@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. >>> >> >
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; } + 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); + 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; 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 #define SMBDIRECT_MAX_SGE 16 /* The context for a SMBD request */ struct smbd_request {
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. 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(-)