On 6/22/22 17:26, Emanuele Giuseppe Esposito wrote:
Am 21/06/2022 um 19:26 schrieb Vladimir Sementsov-Ogievskiy:
On 6/16/22 16:18, Emanuele Giuseppe Esposito wrote:
With the*nop* job_lock/unlock placed, rename the static
functions that are always under job_mutex, adding "_locked" suffix.
List of functions that get this suffix:
job_txn_ref job_txn_del_job
job_txn_apply job_state_transition
job_should_pause job_event_cancelled
job_event_completed job_event_pending
job_event_ready job_event_idle
job_do_yield job_timer_not_pending
job_do_dismiss job_conclude
job_update_rc job_commit
job_abort job_clean
job_finalize_single job_cancel_async
job_completed_txn_abort job_prepare
job_needs_finalize job_do_finalize
job_transition_to_pending job_completed_txn_success
job_completed job_cancel_err
job_force_cancel_err
Note that "locked" refers to the*nop* job_lock/unlock, and not
real_job_lock/unlock.
No functional change intended.
Signed-off-by: Emanuele Giuseppe Esposito<eespo...@redhat.com>
Hmm. Maybe it was already discussed.. But for me it seems, that it would
be simpler to review previous patches, that fix job_ API users to use
locking properly, if this renaming go earlier.
Anyway, in this series, we can't update everything at once. So patch to
patch, we make the code more and more correct. (yes I remember that
lock() is a noop, but I should review thinking that it real, otherwise,
how to review?)
So, I'm saying about formal correctness of using lock() unlock()
function in connection with introduced _locked prifixes and in
connection with how it should finally work.
You do:
05. introduce some _locked functions, that just duplicates, and
job_pause_point_locked() is formally inconsistent, as I said.
06. Update a lot of places, to give them their final form (but not
final, as some functions will be renamed to _locked, some not, hard to
imagine)
07,08,09. Update some more, and even more places. very hard to track
formal correctness of using locks
10-...: rename APIs.
What do you think about the following:
1. Introduce noop lock, and some internal _locked() versions, and keep
formal consistency inside job.c, considering all public interfaces as
unlocked:
at this point:
- everything correct inside job.c
- no public interfaces with _locked prefix
- all public interfaces take mutex internally
- no external user take mutex by hand
We can rename all internal static functions at this step too.
2. Introduce some public _locked APIs, that we'll use in next patches
3. Now start fixing external users in several patches:
- protect by mutex direct use of job fields
- make wider locks and move to _locked APIs inside them where needed
In this scenario, every updated unit becomes formally correct after
update, and after all steps everything is formally correct, and we can
move to turning-on the mutex.
I don't understand your logic also here, sorry :(
I assume you want to keep patch 1-4, then the problem is assing job_lock
and renaming functions in _locked.
So I would say the problem is in patch 5-6-10-11-12-13. All the others
should be self contained.
I understand patch 5 is a little hard to follow.
Now, I am not sure what you propose here but it seems that the end goal
is to just have the same result, but with additional intermediate steps
that are just "do this just because in the next patch will be useful".
I think the problem is that we are going to miss the "why we need the
lock" logic in the patches if we do so.
The logic I tried to convey in this order is the following:
- job.h: add _locked duplicates for job API functions called with and
without job_mutex
Just create duplicates of functions
- jobs: protect jobs with job_lock/unlock
QMP and monitor functions call APIs that assume lock is taken,
drivers must take explicitly the lock
- jobs: rename static functions called with job_mutex held
- job.h: rename job API functions called with job_mutex held
- block_job: rename block_job functions called with job_mutex held
*given* that some functions are always under lock, transform
them in _locked. Requires the job_lock/unlock patch
- job.h: define unlocked functions
Comments on the public functions that are not _locked
@Kevin, since you also had some feedbacks on the patch ordering, do you
agree with this ordering or you have some other ideas?
Following your suggestion, we could move patches 10-11-12-13 before
patch 6 "jobs: protect jobs with job_lock/unlock".
(Apologies for changing my mind, but being the second complain I am
starting to reconsider reordering the patches).
In two words, what I mean: let's keep the following invariant from patch to
patch:
1. Function that has _locked() prefix is always called with lock held
2. Function that has _locked() prefix never calls functions that take lock by
themselves so that would dead-lock
3. Function that is documented as "called with lock not held" is never called
with lock held
That what I mean by "formal correctness": yes, we know that lock is noop, but
still let's keep code logic to correspond function naming and comments that we add.
--
Best regards,
Vladimir
