zenichev left a comment (kamailio/kamailio#4539)
@miconda
> on reload, you reuse the hash table: first you destroy the items on the
> buckets, keeping the first one as dummy, then insert new items with data
> loaded from db?
> It is no new hash table built in parallel and swapping the pointer to the
> active hash table.
Current commit(s) keep the same logic as it's been before. There is no
functional change in this regard.
>From what I can see, permissions module implements the trusted table reload in
>the following way:
- it uses the global pointer to the current (so actual) tursted table
- two table versions are kept for the parallelism reasons (two or more
processes updating the table at a time)
- a temporary pointer (in `reload_trusted_table()`) takes the one which is less
actual (not the latest)
- and re-initializes that (so cleans the content fully)
- now it goes to the db backend, grabs actual entries, and inserts the actual
list of rows one by one, this one will be the most actual table from now
- then the global pointer gets updated with the latest pointed table (within
`reload_trusted_table()`) and hence starts using the very last updated linked
list
This is the previously existing implementation, which is now in all of the
kamailio branches including the master.
The newer implementation that I offered, does exactly the same in this regard,
with the exception that it operates on the hash tables (new abstraction
object), which are covered with the locks and doesn't allow any other 2d or 3d
process in parallel accidentally to take the very same table and update it in
parallel.
This is so far was the problem we've been experiencing since a while. And this
a lot depends on the size of the table (in our case tables are quite big), and
on the amount of RPC processes trying to trigger the reload, in our case it can
be up to 3 processes trying to update the trusted table at a time.
> What is supposed to happen in between 'destroying the items on all buckets'
> and 'inserting the new items'?
The reload worker takes non-actual table when he destroys and then inserts new
items.
It's the one which is not used at the moment by the module, that's why the
already existing implementation keeps two tables for this purpose.
After it's updated it gets pointed by the global pointer, in other words
becomes a new actual table. And the previously actual gets non-actual, and will
be used next time in the same way to prepare a new table version.
> If a Kamailio worker needs to match on trusted table, that is empty for some
> short time and it fails?
This cannot happen, see my explanation above.
Again, I didn't introduce any functional/algorithm change. It is in all
kamailio branches already, and it works fine.
What I do, is that I introduce safer way covered with locks, for cases when 2-3
or even more processes trying to reload table(s).
Moreover it's more elegant, because it uses an abstraction layer for
representing a hash table, and not purely working with linked lists, what makes
it more efficient and easy to work with (as well when coding).
> What is the role of the first dummy item per bucket?
The hash buckets can be implemented with a dummy head or without it.
On the heavy loaded things, like the permissions module is, it should be more
efficient to operate with a dummy head, because:
- we are always sure that the bucket is initialized and ready to use (fast)
- the pointer to the bucket never changes, it stays the same, so only the
`->next` part is an actual beginning
- this gives more stability (locks granularity, concurrent readers/writers,
re-initialization)
- partial cleanup. We do not need to reallocate the bucket when doing a re-init
(like during the reload): no allocation, no pointer churn, no locks re-init etc.
- not the last thing is that dummy heads based implementation simplifies the
code (less code branching, more predictable iteration etc.)
For example, when doing the re-init, having no dummy head, I would have to free
the buckets pointer, allocate it again, reassign.
What all in common gives a little bit more race windows increase and processing
time. Even being covered with locks.
To summarize, I can re-write the code to have no dummy head in the bucket(s),
but i suggest to keep it for the reasons mentioned above.
Please let me know if that explains your questions.
---
@henningw
> If there is no need for having duplicate entries, I think they are not
> necessary, we just need to document it.
Yes, I agree. I will update the doc then.
> For me its fine, maybe other developers want to add something as well. Just
> to understand it correctly, the mentioned rework would be a smaller change
> for the address, just adding some locking improvements? Or using basically
> the same approach as for the trusted? For me the using the same approach
> would be fine for both, as mentioned.
Yes, exactly, I use the same approach for the address table, hence it's going
to be within the same PR.
I would say the approach should be kept as much similar as possible, so that
it's simpler for the code maintenance.
The address table rework is in progress, I haven't had much time
yesterday/today, but it should be ready this/next week.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4539#issuecomment-3745031105
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4539/[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!
