Messages in this thread | | | Subject | Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem | From | Maximilian Luz <> | Date | Tue, 17 Nov 2020 07:05:13 +0100 |
| |
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@gmail.com> 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
| |