Avoid race condition in kernel launch and kernel finish (Windows Only)

[sherry-yuan]

A follow-up to #37

Kernel interrupt are sent when kernel changed status from init to running, however this case was not checked in the signal handler

fpga-runtime-for-opencl/src/acl_kernel_if.cpp

Line 1257 in df4be19

void acl_kernel_if_update_status(acl_kernel_if *kern) {

And previously it was assumed that the finish bit must be set after checking "all other possible cases" (i.e buffer full, profiler interrupt). This assumption was enforced by an assert statement which was removed in #37. It was temporarily removed because the "all other possible case" did not cover all the possible case, namely it missed the "kernel running" status. This means it is possible that no kernel finishes when reaching the check for finish count, and assertion of finish_count > 0.

The full solution is to add additional checking on status register's values so that we don't reach the final "kernel finish" case checking without checking for kernel running status.

[sherry-yuan]

Confirmed the assert should be removed, there are two flow that will call on acl_kernel_if_update_status: polling and interrupt flow . For polling, there is no guarantee that >0 kernel will finish when runtime polls for status. This assert will likely conflict with the printf feature Zibai is implementing using the polling flow.

[sherry-yuan]

Realized anything that is not done, stall or profiler update will not reach assert.

fpga-runtime-for-opencl/src/acl_kernel_if.cpp

Lines 1335 to 1338 in df4be19

	if (ACL_KERNEL_READ_BIT(csr, KERNEL_CSR_DONE) == 0 &&
	ACL_KERNEL_READ_BIT(csr, KERNEL_CSR_STALLED) == 0 &&
	ACL_KERNEL_READ_BIT(csr, KERNEL_CSR_PROFILE_TEMPORAL_STATUS) == 0)
	continue;

However there are still cases where some kernel in queue is done but finish count is 0, log below;

:: Accelerator 11 reporting status 2.
:: Accelerator 11 is done.
HAL Reading from Kernel: 4 bytes 1628 -> 91c1bff2d8
108360416261 acl_kernel_if_write_32b 00000020 0
108360416264 acl_kernel_if_read_32b 00001628 0
HAL Reading from Kernel: 4 bytes 20 -> 91c14f9fd0
:: Accelerator 11 has 0 finishes.

Normally it should look like (always like this with no other writes in between):

:: Accelerator 11 reporting status 2.
:: Accelerator 11 is done.
HAL Reading from Kernel: 4 bytes 1628 -> 91c1bff2d8
108360062459 acl_kernel_if_read_32b 00001628 1
:: Accelerator 11 has 1 finishes.

acl_launch_kernel
108077750927 acl_kernel_if_read_32b 00001600 262146
:: Accelerator 11 reporting status 2.
:: Accelerator 11 is done.
:: Launching kernel 6 on accelerator 5, device 0.
HAL Reading from Kernel: 4 bytes 1628 -> 91c1bff2d8
:: Set the shared control for the profile counters:: Accelerator 5.
:: Not setting shared control value - shared counters not enabled 5.
HAL Writing to Kernel: 4 bytes 91c14f9fc0 -> 20
108077751948 acl_kernel_if_write_32b 00000020 0
HAL Reading from Kernel: 4 bytes 20 -> 91c14f9fd0
108077750950 acl_kernel_if_read_32b 00001628 1
108077751963 acl_kernel_if_read_32b 00000020 0
:: Accelerator 11 has 1 finishes.
HAL Writing to Kernel: 72 bytes 1c5d1784060 -> 1830
HAL Writing to Kernel: 4 bytes 91c1bff2b0 -> 20

This suggests writing 0 at CSR address 00000020 has overwritten values on 00001628.

The writing 0 at CSR address 00000020 only happens closely after launching a new kernel.

x20 address corresponds to OFFSET_KERNEL_CRA_SEGMENT

fpga-runtime-for-opencl/src/acl_kernel_if.cpp

Line 53 in cfac76e

#define OFFSET_KERNEL_CRA_SEGMENT ((dev_addr_t)0x0020)

Further investigation will be updated later.
Since this issue does not seem to be related to missing case checking, the issue title will be updated to "Avoid race condition in kernel launch and kernel finish".

Can confirm the done status and finish count is read from the right csr address. This means race condition causing incorrect offset is not possible.

Nothing stops kernel launch and kernel interrupt to be called at the same time. The following is the proof that kernel launch and kernel finish can be called at the same time

:: Updating kernel status. (kernel interrupt)
HAL Reading from Kernel: 4 bytes 20 -> 91c1bff2f8 (kernel interrupt)
acl_launch_kernel (kernel launch)
:: Launching kernel 8 on accelerator 5, device 0. (kernel launch)
108077728773 acl_kernel_if_read_32b 00000020 4096 (kernel interrupt)
:: Set the shared control for the profile counters:: Accelerator 5. (kernel launch)
:: Not setting shared control value - shared counters not enabled 5. (kernel launch)
HAL Reading from Kernel: 4 bytes 1600 -> 91c1bff2c4 (kernel interrupt)
HAL Writing to Kernel: 4 bytes 91c14fa0f0 -> 20
108077728797 acl_kernel_if_read_32b 00001600 262146 (kernel interrupt)
:: Accelerator 11 reporting status 2. (kernel interrupt)
:: Accelerator 11 is done. (kernel interrupt)

[sherry-yuan]

Next steps:

• Find tools that detect race condition for windows (and linux)
• add CI/CD to avoid race conditions.
• Write up unit tests to reproduce such behavior.
• Put in simulation waveform and the read/write to CSR

[sherry-yuan]

This comment keep track of list of tools for detecting race conditions.

1. tsan. Other documentation can be found here
2. CHESS

A bunch of other concurrency testing tools are documented here. (Apparently intel has its own The Intel Thread Checker too)

[sherry-yuan]

Upon investigating the simulation waveform, it seems like the first kernel interrupt is stuck forever, and there were no simultaneous read/write from same address. On simulation, there is also no address change in the middle of read/write operation. (I doubt whether simulation have same behavior as hardware runs.)