zenichev left a comment (kamailio/kamailio#4787)
> In many cases I look at the diff of the PRs, because if there are many
> commits, one can amend another one and it is hard to track the final result
> by looking at subsequent commits. It is a significant diff and trust me I
> looked at it more than to many other PRs, but it is not humanly possible to
> cover all the cases there at such size.
Clear, thanks for the explanation.
> The races between match and reload are a matter of design and with the one I
> understood being in this PR, the races cannot be avoided.
I didn't cover the match part with the locks indeed (that would have been an
over-kill), but they aren't the problem.
The problem happens only when there is a change of the content, and this is
namely **only**: hr timer and reload (reload either with RPC or any other way
triggered).
> So, the match_hash_table() starts with *current_trusted_table, but uses only
> bucket lock, not table lock. A reload can swap the active pointer (table lock
> not being acquired by the match) and the timer routine starts freeing the
> table that can still be used by already-started match_hash_table(). Am I
> wrong here? If yes, how an already started match is protecting that a reload
> does not destroy the table it is using?
This is a good point.
The upper lock has a precedence over the buckets level lock, but that only
prevails in case of reloads and hr timer.
As soon as the match process starts, it uses the hash index to find the
required bucket, and locks it while the matching is being done. But it locks
only the bucket, not the whole table, then when by the hash index the bucket
(the list if to call it simpler) is selected, it's locked for write operations.
Hence any reload who wants to get to the buckets and change anything there will
comes through: 1. upper level lock (acquiring the whole table); 2. buckets
level lock (bucket specific lock) while trying to change anything and will be
waiting to acquire it until the match is done.
That just parallelizes the work of two competing things: write operations one
side (e.g. reload) and read operations other side (e.g. match). Since the most
what happens is match and RPC reloads or the hr timer are much rare thing in
comparison to the match, this isn't a hit to the processing time.
> Am I wrong here? If yes, how an already started match is protecting that a
> reload does not destroy the table it is using?
When the reload happens, the swap will trigger the `trusted_table_reinit()` on
the `new_trusted_table` whereas the `trusted_table_free_buckets()` is covered
with the bucket locks, and it will not touch the concerned bucket used for
matching. Please remember that the swap of the global cursor happens at the
point of real swap at the end of the reload.
So if P1 does the match based on the most actual table (let say taking the
table X), P2 tries to reload and takes the less actual table (let's say Y). P1
will lock the bucket of the X table and not allow any other process coming
ahead (P3) to touch the content, and such a new process will have to wait.
P2 will finish the reload (based on the older table Y content) and swap the
global cursor to it. X table gets stale.
Even if two competitive processes come to run the reload, the second process
will be blocked by the lock acquired previously, and hence not allow to overlap
in write operations (main issue in the architecture of the module right now).
That means that potentially only one single process get into the situation when
it tries to re-fill the content of the selected table, but there is a match
being done. As soon as the match is managed, the swap of tables is allowed.
**I want to underline,** now nothing is locks safe, the processes are
chaotically doing the read and write operations on tables and the users have
quite a good chance to fall into our situation with iterator invalidation
issues. This all depends on how huge the loading is on the system.
> Up to you if you don't want to go with this PR further, it is not that I
> didn't want to accept your changes, I suggested an alternative as well. It is
> about the fact that a module used for critical operations should be protected
> as much as possible and a massive change has to show its benefits. If you
> don't want to discuss and clarify the concerns, the variant with a different
> module stands as an option.
Tbh, I had a perception of things that the change had no go.
And most of the argumentation was not meeting the actual commits content
(especially what's been described), what made me surprised and leading to
understanding that invest time into something that won't have a green light.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4787#issuecomment-4865797355
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4787/[email protected]>
_______________________________________________
Kamailio - Development Mailing List -- [email protected]
To unsubscribe send an email to [email protected]
Important: keep the mailing list in the recipients, do not reply only to the
sender!
