Skip to content

Bluetooth: host: Remove cancel sync from database hash commit #35403

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 25, 2021
Merged

Bluetooth: host: Remove cancel sync from database hash commit #35403

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 47 additions & 35 deletions subsys/bluetooth/host/gatt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ enum {

#if defined(CONFIG_BT_GATT_CACHING)
DB_HASH_VALID, /* Database hash needs to be calculated */
DB_HASH_LOAD, /* Database hash loaded from settings. */
#endif
/* Total number of flags - must be at the end of the enum */
SC_NUM_FLAGS,
Expand All @@ -248,6 +249,9 @@ static struct gatt_sc {
#if defined(CONFIG_BT_GATT_CACHING)
static struct db_hash {
uint8_t hash[16];
#if defined(CONFIG_BT_SETTINGS)
uint8_t stored_hash[16];
Comment on lines 251 to +253
Copy link
Contributor

@Thalley Thalley May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a GATT_DB_HASH_SIZE or similar macro?

#endif
struct k_work_delayable work;
struct k_work_sync sync;
} db_hash;
Expand Down Expand Up @@ -719,8 +723,44 @@ static void db_hash_gen(bool store)
atomic_set_bit(gatt_sc.flags, DB_HASH_VALID);
}

#if defined(CONFIG_BT_SETTINGS)
static void sc_indicate(uint16_t start, uint16_t end);
#endif

static void db_hash_process(struct k_work *work)
{
#if defined(CONFIG_BT_SETTINGS)
if (atomic_test_and_clear_bit(gatt_sc.flags, DB_HASH_LOAD)) {
if (!atomic_test_bit(gatt_sc.flags, DB_HASH_VALID)) {
db_hash_gen(false);
}

/* Check if hash matches then skip SC update */
if (!memcmp(db_hash.stored_hash, db_hash.hash,
sizeof(db_hash.stored_hash))) {
BT_DBG("Database Hash matches");
k_work_cancel_delayable(&gatt_sc.work);
Copy link
Contributor

@pabigot pabigot May 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the cancel fail? If so what happens?

Copy link
Contributor Author

@joerchan joerchan May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Thalley Thalley May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pabigot pabigot May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

atomic_clear_bit(gatt_sc.flags, SC_RANGE_CHANGED);
return;
}

BT_HEXDUMP_DBG(db_hash.hash, sizeof(db_hash.hash),
"New Hash: ");

/* GATT database has been modified since last boot, likely due
* to a firmware update or a dynamic service that was not
* re-registered on boot.
* Indicate Service Changed to all bonded devices for the full
* database range to invalidate client-side cache and force
* discovery on reconnect.
*/
sc_indicate(0x0001, 0xffff);

/* Hash did not match, overwrite with current hash */
db_hash_store();
return;
}
#endif /* defined(CONFIG_BT_SETTINGS) */
db_hash_gen(true);
}

Expand Down Expand Up @@ -5118,58 +5158,30 @@ static int cf_set(const char *name, size_t len_rd, settings_read_cb read_cb,

SETTINGS_STATIC_HANDLER_DEFINE(bt_cf, "bt/cf", NULL, cf_set, NULL, NULL);

static uint8_t stored_hash[16];

static int db_hash_set(const char *name, size_t len_rd,
settings_read_cb read_cb, void *cb_arg)
{
ssize_t len;

len = read_cb(cb_arg, stored_hash, sizeof(stored_hash));
len = read_cb(cb_arg, db_hash.stored_hash, sizeof(db_hash.stored_hash));
if (len < 0) {
BT_ERR("Failed to decode value (err %zd)", len);
return len;
}

BT_HEXDUMP_DBG(stored_hash, sizeof(stored_hash), "Stored Hash: ");
BT_HEXDUMP_DBG(db_hash.stored_hash, sizeof(db_hash.stored_hash),
"Stored Hash: ");

return 0;
}

static int db_hash_commit(void)
{

k_sched_lock();

/* Stop work and generate the hash */
(void)k_work_cancel_delayable_sync(&db_hash.work, &db_hash.sync);
if (!atomic_test_bit(gatt_sc.flags, DB_HASH_VALID)) {
db_hash_gen(false);
}

k_sched_unlock();

/* Check if hash matches then skip SC update */
if (!memcmp(stored_hash, db_hash.hash, sizeof(stored_hash))) {
BT_DBG("Database Hash matches");
k_work_cancel_delayable(&gatt_sc.work);
atomic_clear_bit(gatt_sc.flags, SC_RANGE_CHANGED);
return 0;
}

BT_HEXDUMP_DBG(db_hash.hash, sizeof(db_hash.hash), "New Hash: ");

/**
* GATT database has been modified since last boot, likely due to
* a firmware update or a dynamic service that was not re-registered on
* boot. Indicate Service Changed to all bonded devices for the full
* database range to invalidate client-side cache and force discovery on
* reconnect.
atomic_set_bit(gatt_sc.flags, DB_HASH_LOAD);
/* Reschedule work to calculate and compare against the Hash value
* loaded from flash.
*/
sc_indicate(0x0001, 0xffff);

/* Hash did not match overwrite with current hash */
db_hash_store();
k_work_reschedule(&db_hash.work, K_NO_WAIT);
Copy link
Contributor

@pabigot pabigot May 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@joerchan joerchan May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@pabigot pabigot May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@joerchan joerchan May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pabigot pabigot May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@joerchan joerchan May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Settings load should only happen once.


return 0;
}
Expand Down