On Tue, Oct 25, 2022 11:50 PM Bob Pearson wrote:
> On 10/25/22 04:35, Daisuke Matsuda (Fujitsu) wrote:
> > On Sat, Oct 22, 2022 5:01 AM Bob Pearson:
> >>
<...>
> >> +
> >> +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.
I know rxe_resp_queue_pkt() and rxe_comp_queue_pkt() usually call the procedures
directly. That made me wonder why we need the additional type for users when
there are virtually no difference after all. I now understand how you use this mode for
HPC, but I am not sure we should add this new type for the very specific purpose.
>
> 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.
This sounds correct, but it seems we cannot delve into this issue further until the
discussion of whether to eliminate the tasklets or not settles. If the tasklets will be
removed, then I think we should give up the inline type along with the module
parameters in order to keep the things simple, which is much more friendly to
ordinary end-users.
Thanks,
Daisuke
>
> 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
> >
