Possible bugs in pmdk examples

62 views
Skip to first unread message

Benjamin Reidys

unread,
Nov 3, 2021, 8:23:15 PM11/3/21
to pmem
Hello all,

My research group is testing a tool working with pm programs. I wanted to verify a few things we found when working with the pmdk examples.
(1) tree_map/btree_map.c:437. The code uses a TX_ADD(node) which copies the entire node to the undo log. Is it necessary to copy the entire node to the log? It appears that only a subset of fields is modified and flushing extra data is inefficient.
(2) pminvaders/pminvaders.c:220. Same as before, the entire gstate object is persisted when only one field is updated. Is there a need to persist the whole object?
(3) pminvaders/pminvaders.c:237. The gstate object is persisted even though the update_score function ends with the exact same call to persist gstate.
(4) pminvaders/pminvaders.c:325. The player object is flushed, even in the default case. This would appear unnecessary.
(5) hashmap/hashmap_atomic.c:234/255/471. The count dirty variable is used to check if recalculating the count is necessary. Are there  possible issues with using this variable with a multi threaded application?  For example, thread 1 finishes recovery after thread 2 crashes during insertion. Thread 2 would not see count_dirty=1 when recovering after thread 1. Alternatively a successful insertion by one thread can nullify count_dirty before another thread recovers.

Please let me know if I have misunderstood something about the code!
Thanks in advance,
-Ben

ppbb...@gmail.com

unread,
Nov 5, 2021, 6:56:41 AM11/5/21
to pmem
Hi Ben,

This is my favorite type of bug reports ;) I really appreciate the effort researchers like you and your group are putting to validate our libraries! Please do let us know when you publish your work.
As for the bugs:
1. You are right. It's not necessary. However, like in this case, we sometimes prioritize clarity in examples over minuscule optimizations.
2. The entire game_state object is 8 bytes. That's why we are just persisting the entire thing instead of individual fields. It would make no difference.
3. Yup, good fine. That persist is redundant.
4. Again, this was likely made for clarity. However, in this case, I can imagine this actually making a non-insignificant performance different. I'll make that change ;)
5. This code is not thread-safe. The word "atomic" refers to the fact that this example uses the "atomic API" from libpmemobj. Sorry about that - I can see how this can be confusing.

I've created a PR to remove the unnecessary persists in pminvaders: https://github.com/pmem/pmdk/pull/5358
Who should I say this is Reported-by?

Hope this helps,
Piotr

Benjamin Reidys

unread,
Nov 5, 2021, 3:13:59 PM11/5/21
to pmem
Hi Piotr,

Thanks for your reply!

I am happy to see that most of what we are detecting are in fact bugs.
You can put me as the Reported-by.

Thanks,
Ben
Reply all
Reply to author
Forward
0 new messages