On 11/16/20 6:03 PM, Maximilian Luz wrote:
On 11/16/20 2:33 PM, Andy Shevchenko wrote:
On Sun, Nov 15, 2020 at 9:25 PM Maximilian Luz <luzmaximilian@xxxxxxxxx> wrote:
[...]
READ_ONCE and WRITE_ONCE are used to ensure proper access to state that
can be changed outside of the queue/pending locks (or under any one of
them). In general, I have tried to document all critical access of such
state with an explanation of why it is safe to do so.
I've looked at this again and noticed that I can guard the packet
timestamp by the pending lock and the packet priority by the queue lock
(after first submission). This makes reasoning about access to them
significantly easier and removes the need for WRITE_ONCE / READ_ONCE.
After that, READ_ONCE is used
- to access the controller state for smoke-testing to (hopefully) detect
invalid request function usage (note that all other access to this
state happens under the controller state lock)
- for the "safe counters", where access to the shared value is, after
initialization, restricted to the increment function
- to update the timeout reaper, where access to the shared value
(rtx_timeout.expires) is, after initialization, restricted to its
modification function (ssh_ptl_timeout_reaper_mod() /
ssh_rtl_timeout_reaper_mod()) and the timer function
- to access the request timestamp, which is, after initialization, only
set once in the lifetime of a request (all other access is read-only)
- to access the 'ptl' reference of the packet, which, after
initialization, is only set once, either at packet or request
submission (all other access is read-only). Note due to this,
READ_ONCE is only required for functions that can run concurrently
with ssh_ptl_submit() and ssh_rtl_submit(), i.e. ssh_ptl_cancel() and
ssh_rtl_cancel().
- to access request state outside of bit-ops when canceling
I'd argue that all of these cases can be checked and verified with a
reasonable amount of effort. Cancellation (last two points) is probably
the most complex one. Unfortunately, I don't see any way to simplify
that part without disallowing cancellation to run concurrently to
submission, which is something I'd like to support as this makes
implementing asynchronous requests in the future easier.
Regards,
Max
