Re: [PATCH for-next v2 18/18] RDMA/rxe: Add parameters to control task type

[Bob Pearson <rpearsonhpe@gmail.com>]

On 10/25/22 04:35, Daisuke Matsuda (Fujitsu) wrote:
> On Sat, Oct 22, 2022 5:01 AM Bob Pearson:
>>
>> Add modparams to control the task types for req, comp, and resp
>> tasks.
>>
>> It is expected that the work queue version will take the place of
>> the tasklet version once this patch series is accepted and moved
>> upstream. However, for now it is convenient to keep the option of
>> easily switching between the versions to help benchmarking and
>> debugging.
>>
>> Signed-off-by: Ian Ziemba <ian.ziemba@hpe.com>
>> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
>> ---
>>  drivers/infiniband/sw/rxe/rxe_qp.c   | 6 +++---
>>  drivers/infiniband/sw/rxe/rxe_task.c | 8 ++++++++
>>  drivers/infiniband/sw/rxe/rxe_task.h | 4 ++++
>>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> <...>
> 
>> diff --git a/drivers/infiniband/sw/rxe/rxe_task.c b/drivers/infiniband/sw/rxe/rxe_task.c
>> index 9b8c9d28ee46..4568c4a05e85 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_task.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_task.c
>> @@ -6,6 +6,14 @@
>>
>>  #include "rxe.h"
>>
>> +int rxe_req_task_type = RXE_TASK_TYPE_TASKLET;
>> +int rxe_comp_task_type = RXE_TASK_TYPE_TASKLET;
>> +int rxe_resp_task_type = RXE_TASK_TYPE_TASKLET;
> 
> As the tasklet version is to be eliminated in near future, why
> don't we make the workqueue version default now?
> 
> 
>> +
>> +module_param_named(req_task_type, rxe_req_task_type, int, 0664);
>> +module_param_named(comp_task_type, rxe_comp_task_type, int, 0664);
>> +module_param_named(resp_task_type, rxe_resp_task_type, int, 0664);
> 
> As I have commented to the 7th patch, users would not benefit from
> specifying the 'inline' type directly. It is OK to have the mode internally
> to keep the source code simple, but RXE_TASK_TYPE_INLINE should
> not be exposed to users.
> 
> I suggest that these module parameters be bool type and task types
> be like this:
> === rxe_task.h===
> enum rxe_task_type {
>         RXE_TASK_TYPE_WORKQUEUE = 0,
>         RXE_TASK_TYPE_TASKLET   = 1,
>         RXE_TASK_TYPE_INLINE    = 2,
> };
> =============
> In this case, while we can preserve the 'inline' type internally,
> we can prohibit users from specifying any value other than
> 'workqueue' or 'tasklet'; modprobe fails if non-boolean values
> are passed.

I don't know if you have noticed this but the tasks that handle incoming packets
already process them inline if the queues are empty which is most of the time.
The difference between this and inline always is not major. The NAPI thread is
already deferred once so we're not running at hw interrupt level.

What you say makes sense in a multi-user environment but that is not always the case.
HPC jobs typically have the node dedicated to a single user and it seems best to let
them figure out what works best. In any case it takes root to make this change.

Bob
> 
> If you still want to keep the parameters int type, then you need
> to add some code to perform value check. We can specify an 
> arbitrary int value with the current implementation.
> 
> Thanks,
> Daisuke
> 
>> +
>>  static struct workqueue_struct *rxe_wq;
>>
>>  int rxe_alloc_wq(void)
>> diff --git a/drivers/infiniband/sw/rxe/rxe_task.h b/drivers/infiniband/sw/rxe/rxe_task.h
>> index d1156b935635..5a2ac7ada05b 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_task.h
>> +++ b/drivers/infiniband/sw/rxe/rxe_task.h
>> @@ -7,6 +7,10 @@
>>  #ifndef RXE_TASK_H
>>  #define RXE_TASK_H
>>
>> +extern int rxe_req_task_type;
>> +extern int rxe_comp_task_type;
>> +extern int rxe_resp_task_type;
>> +
>>  struct rxe_task;
>>
>>  struct rxe_task_ops {
>> --
>> 2.34.1
>