Re: [PATCH for-next v3 00/13] Implement work queues for rdma_rxe

[Bob Pearson <rpearsonhpe@gmail.com>]

On 11/2/22 05:17, Daisuke Matsuda (Fujitsu) wrote:
> On Sat, Oct 29, 2022 12:10 PM Bob Pearson wrote:
>> This patch series implements work queues as an alternative for
>> the main tasklets in the rdma_rxe driver. The patch series starts
>> with a patch that makes the internal API for task execution pluggable
>> and implements an inline and a tasklet based set of functions.
>> The remaining patches cleanup the qp reset and error code in the
>> three tasklets and modify the locking logic to prevent making
>> multiple calls to the tasklet scheduling routine. After
>> this preparation the work queue equivalent set of functions is
>> added and the tasklet version is dropped.
> 
> Thank you for posting the 3rd series.
> It looks fine at a glance, but now I am concerned about problems
> that can be potentially caused by concurrency.
> 
>>
>> The advantages of the work queue version of deferred task execution
>> is mainly that the work queue variant has much better scalability
>> and overall performance than the tasklet variant.  The perftest
>> microbenchmarks in local loopback mode (not a very realistic test
>> case) can reach approximately 100Gb/sec with work queues compared to
>> about 16Gb/sec for tasklets.
> 
> As you wrote, the advantage of work queue version is that the number works
> that can run parallelly scales with the number of logical CPUs. However, the
> dispatched works (rxe_requester, rxe_responder, and rxe_completer) are
> designed for serial execution on tasklet, so we must not rely on them functioning
> properly on parallel execution.

Work queues are serial for each separate work task just like tasklets. There isn't
a problem here. The tasklets for different tasks can run in parallel but tend to
do so less than work queue tasks. The reason is that tasklets are scheduled by
default on the same cpu as the thread that scheduled it while work queues are scheduled
by the kernel scheduler and get spread around.
> 
> There could be 3 problems, which stem from the fact that works are not necessarily
> executed in the same order the packets are received. Works are enqueued to worker
> pools on each CPU, and each CPU respectively schedules the works, so the ordering
> of works among CPUs is not guaranteed.
> 
> [1]
> On UC/UD connections, responder does not check the psn of inbound packets,
> so the payloads can be copied to MRs without checking the order. If there are
> works that write to overlapping memory locations, they can potentially cause
> data corruption depending the order.
> 
> [2]
> On RC connections, responder checks the psn, and drops the packet if it is not
> the expected one. Requester can retransmit the request in this case, so the order
> seems to be guaranteed for RC.
> 
> However, responder updates the next expected psn (qp->resp.psn) BEFORE
> replying an ACK packet. If the work is preempted soon after storing the next psn,
> another work on another CPU can potentially reply another ACK packet earlier.
> This behaviour is against the spec.
> Cf. IB Specification Vol 1-Release-1.5 " 9.5 TRANSACTION ORDERING"
> 
> [3]
> Again on RC connections, the next expected psn (qp->resp.psn) can be
> loaded and stored at the same time from different threads. It seems we
> have to use a synchronization method, perhaps like READ_ONCE() and
> WRITE_ONCE() macros, to prevent loading an old value. This one is just an
> example; there can be other variables that need similar consideration.
> 
> 
> All the problems above can be solved by making the work queue single-
> threaded. We can do it by using flags=WQ_UNBOUND and max_active=1
> for alloc_workqueue(), but this should be the last resort since this spoils
> the performance benefit of work queue.
> 
> I am not sure what we can do with [1] right now.
> For [2] and [3], we could just move the update of psn later than the ack reply,
> and use *_ONCE() macros for shared variables.
> 
> Thanks,
> Daisuke
> 
>>
>> This version of the patch series drops the tasklet version as an option
>> but keeps the option of switching between the workqueue and inline
>> versions.
>>
>> This patch series is derived from an earlier patch set developed by
>> Ian Ziemba at HPE which is used in some Lustre storage clients attached
>> to Lustre servers with hard RoCE v2 NICs.
>>
>> It is based on the current version of wip/jgg-for-next.
>>
>> v3:
>> Link: https://lore.kernel.org/linux-rdma/202210220559.f7taTL8S-lkp@intel.com/
>> The v3 version drops the first few patches which have already been accepted
>> in for-next. It also drops the last patch of the v2 version which
>> introduced module parameters to select between the task interfaces. It also
>> drops the tasklet version entirely. It fixes a minor error caught by
>> the kernel test robot <lkp@intel.com> with a missing static declaration.
>>
>> v2:
>> The v2 version of the patch set has some minor changes that address
>> comments from Leon Romanovsky regarding locking of the valid parameter
>> and the setup parameters for alloc_workqueue. It also has one
>> additional cleanup patch.
>>
>> Bob Pearson (13):
>>   RDMA/rxe: Make task interface pluggable
>>   RDMA/rxe: Split rxe_drain_resp_pkts()
>>   RDMA/rxe: Simplify reset state handling in rxe_resp.c
>>   RDMA/rxe: Handle qp error in rxe_resp.c
>>   RDMA/rxe: Cleanup comp tasks in rxe_qp.c
>>   RDMA/rxe: Remove __rxe_do_task()
>>   RDMA/rxe: Make tasks schedule each other
>>   RDMA/rxe: Implement disable/enable_task()
>>   RDMA/rxe: Replace TASK_STATE_START by TASK_STATE_IDLE
>>   RDMA/rxe: Replace task->destroyed by task state INVALID.
>>   RDMA/rxe: Add workqueue support for tasks
>>   RDMA/rxe: Make WORKQUEUE default for RC tasks
>>   RDMA/rxe: Remove tasklets from rxe_task.c
>>
>>  drivers/infiniband/sw/rxe/rxe.c      |   9 +-
>>  drivers/infiniband/sw/rxe/rxe_comp.c |  24 ++-
>>  drivers/infiniband/sw/rxe/rxe_qp.c   |  80 ++++-----
>>  drivers/infiniband/sw/rxe/rxe_req.c  |   4 +-
>>  drivers/infiniband/sw/rxe/rxe_resp.c |  70 +++++---
>>  drivers/infiniband/sw/rxe/rxe_task.c | 258 +++++++++++++++++++--------
>>  drivers/infiniband/sw/rxe/rxe_task.h |  56 +++---
>>  7 files changed, 329 insertions(+), 172 deletions(-)
>>
>>
>> base-commit: 692373d186205dfb1b56f35f22702412d94d9420
>> --
>> 2.34.1
>