Messages in this thread | | | Date | Tue, 29 Aug 2023 23:25:46 +0200 | From | Michał Mirosław <> | Subject | Re: [PATCH 6/6] regulator: core: simplify lock_two() |
| |
On Tue, Aug 29, 2023 at 03:52:19PM -0500, Stephen Boyd wrote: > Quoting Michał Mirosław (2023-08-28 13:26:54) > > Indeed they are quite similar. I did remove a bit more code than that, > > though: in this case there is no early success return before the loop. > > > > Instead of saying: > > > > lock A > > lock B > > if ok return > > if that failed, loop: > > unlock A > > lock B harder > > lock A > > if ok return > > swap A <-> B > > lock B > > > > Now it's: > > > > lock A > > loop forever: > > lock B > > if ok, return > > unlock A > > swap them > > lock A harder > > > > With the same condition 'A held' at the start of an iteration. > > > > Removing duplicate code is great! I'm primarily concerned with > readability. The terms 'A' and 'B' doesn't make it easy for me. Can you > maintain the 'held' and 'contended' names for the variables? > > That would be > > 1. lock 'held' > 2. loop forever: > 3. lock 'contended' > 4. if ok, return > 5. unlock 'held' > 6. swap them > 7. lock 'held' harder
Doesn't this make it more confusing? The lock is 'held' only in lines 2-5 and looses this trait (but not the name) on the other lines. 'contended' is more problematic: the contended lock is called 'held' before locking it at line 7.
The algorithm is basically: Take the locks in sequence. If that failed, swap the order and try again.
Would a comment like the sentence above help with readability?
Or we could wrap the final lines of the iteration in a 'regulator_lock_contended()' to make it self-documenting?
Best Regards Michał Mirosław
| |