Bluetooth: host: Remove cancel sync from database hash commit

[joerchan]

Fix deadlock when db_hash_commit has to wait for the delayed work to
finish. This creates a deadlock if the delayed work for database hash
calculation needs to store the hash since the settings API is locked
when calling the commit callback.
Remove call to k_work_cancel_delayable_sync from db_hash_commit in order
to avoid the deadlock. Instead move comparing of the stored hash to the
delayed work and reschedule the work with no wait.

Fixes: #35058

[pabigot]

The settings API's use of "load", "set", and "commit" seem to be non-intuitive. I don't understand what this code does.

But that's irrelevant. This appears to eliminate the deadlock. But it may violate user expectations that settings_commit() will block until the settings have actually been committed (whatever that means). The effect of the commit will be delayed until the work item is completed, no?

From the perspective of eliminating the deadlock this seems fine. I can't speak to whether the resulting behavior is acceptable.

I do believe there's a data race involving stored_hash but that wouldn't be new.

[pabigot]

Does the semantics of settings_commit allow this? The commit will not have completed by the time the call returns because it doesn't wait. Unless setting the LOAD flag ensures that any operation that depends on the commit will detect the incomplete operation and complete it before the work handler does.

If that makes any sense and is also correct perhaps a comment could be added.

[joerchan]

The settings API's use of "load", "set", and "commit" seem to be non-intuitive. I don't understand what this code does.

You get a "set" for each value stored, and then "commit" once all values have been loaded, i.e no more "set" callbacks.

The value has been "commited" as we have the stored_hash value. The next thing we wish to do is to compare it against the calculated hash value, and store the new value if they are different. That don't have to be part of the the commit here.

So we can push this to the db_hash work, since this was already the work responsible for the calculation.
As long as peers that read the hash characteristic value receive the correct value then this works as expected.

Can you elaborate on the data race on stored_hash?

[pabigot]

Can you elaborate on the data race on stored_hash?

I don't see anything that prevents one thread from accessing it via db_hash_set() while the work thread is accessing it via db_hash_process(). Only a problem when preemptable threads are involved or SMP is present.

[joerchan]

They should be strictly sequential, db_hash_set is only called before db_hash_commit, and db_hash_work would only read the stored_hash when it has been submitted from db_hash_commit due to the flag set.

[pabigot]

It's still a data race if a second set/commit occurs in a thread that preempts the work thread. Unlikely, but not impossible, unless all set/commits come from something that runs on the same work queue.

[joerchan]

Settings load should only happen once.

[pabigot]

Can the cancel fail? If so what happens?

[joerchan]

This would now be from a work handler so shouldn't fail in being canceled, right?

EDIT:
If it would fail then we would send out a service changed indication to all connected peers. That would be uneccesary as they would have to rediscover a database that hasn't changed. But having peers connected at this early in the startup phase is very unlikely.

[Thalley]

If it can fail, and we need to do something, we should check the return value and do whatever.

If you don't think we need to do anything, I think we should cast the return value to (void) to make it explicit.

[pabigot]

Yes, preemption of the work thread and SMP could both create a situation where the cancel could fail.

https://docs.zephyrproject.org/latest/reference/kernel/threads/workqueue.html#check-return-values shows recommended documentary practices. This comment provides more background on the recommendation and why a cast to (void) is not useful by itself.

[Thalley]

We don't have a GATT_DB_HASH_SIZE or similar macro?

[carlescufi]

@mniestroj could you please check if this solves the issue?

[joerchan]

mniestroj could you please check if this solves the issue?

@carlescufi FYI: We ran into the same issue here as well eventually, and @MarekPieta has confirmed that this fixed the issue for us.

[mniestroj]

I confirm it fixes #35058.