Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 1 of 3 files reviewed, 11 unresolved discussions (waiting on @lukaszstolarczuk and @yjrobin)

-- commits, line 6 at r1:
2 commits with the same description? perhaps they should be just squashed

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 2 at r1 (raw file):

// SPDX-License-Identifier: BSD-3-Clause
/* Copyright 2021-2022, Intel Corporation */

2021 should be enough 😉

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 6 at r1 (raw file):

/**
 * @file
 * Atomic specialization for persistent-ware self_relative_ptr.

misspell - persistent-aware

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 55 at r1 (raw file):

		delete;

	/*

please make these comments a Doxygen style:

/**
 * ...
 */

this way they'll be properly documented.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 84 at r1 (raw file):

	/*
	 * Read-optimized load retries upon dirty ptr, relies on tge store

tge store = misspell

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 99 at r1 (raw file):

		return val;
	}
	/*

pls add an empty line before this comment

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 2 at r1 (raw file):

// SPDX-License-Identifier: BSD-3-Clause
/* Copyright 2020, Intel Corporation */

2021

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 28 at r1 (raw file):

	pmem::obj::experimental::atomic_persistent_aware_ptr<T, ReadOptimized>;

constexpr int ARR_SIZE = 10000;

this test timed out on our Windows CI - perhaps this number is too big...?

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 52 at r1 (raw file):

		});
	} catch (...) {
		UT_ASSERT(0);

please use ASSERT_UNREACHABLE instead of UT_ASSERT(0)

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 83 at r1 (raw file):

test(int argc, char *argv[])
{
	if (argc != 2)

you could check this once and just pass path to the function

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 87 at r1 (raw file):

	char path[100];
	strcpy(path, argv[1]);

you could use strncpy just to be safe (including length of the suffix you add later)

Reviewable status: 1 of 3 files reviewed, 17 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 29 at r1 (raw file):

template <typename T, typename ReadOptimized>
struct atomic_persistent_aware_ptr {

Could you please add some decsription of this pointer? It would be good to mention that it takes care of persisting itself (and also ensuring that data visibility == data persistence).

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 29 at r1 (raw file):

template <typename T, typename ReadOptimized>
struct atomic_persistent_aware_ptr {

Looking at this implementation, I don't see anything pointer-specific. Maybe you could make this structure a generic one (something like template <typename T, typename ReadOptimized> struct atomic_persistent_aware {}'.

and in skip list we would use atomic_persistent_aware<self_relative_ptr<T>> ?

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 94 at r1 (raw file):

		auto val = ptr.load(order);
		while (is_dirty(val)) {
			std::this_thread::yield();

Hm, I'm not sure if yielding right away is the best option, maybe we should try to spin for a while? Please take a look at: https://github.com/pmem/libpmemobj-cpp/blob/master/include/libpmemobj%2B%2B/detail/atomic_backoff.hpp which can be use in this scenario. (It uses pause instructions) Example usage is here: https://github.com/pmem/libpmemobj-cpp/blob/master/include/libpmemobj%2B%2B/container/concurrent_hash_map.hpp#L3049

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 122 at r1 (raw file):

		 std::memory_order order = std::memory_order_seq_cst) noexcept
	{
		return ptr.exchange(desired, order);

I think that all write functions (exchange, compare_exchange_weak/strong, fetch_add, fetch_sub) should use the same mechanism as store for guaranteeing data consistency.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 297 at r1 (raw file):

 */
template <typename T, typename ReadOptimized>
struct can_do_snapshot<

Actually I think we should not allow atomic_persistent_aware_ptr to be snapshotted (that is, it would be better to remove this can_do_snapshot specialization). Unlike self_relative_ptr, this type takes care of persisting itself.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 28 at r1 (raw file):

I belive the Windows timeout might be due to the use of yield. This problem might disappear if you use atomic_backoff as suggested in one of the comments above.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 105 at r1 (raw file):

int
main(int argc, char *argv[])

It would be good to have some test for veryfing data visibility.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 110 at r1 (raw file):

		run_test([&] { test<std::false_type>(argc, argv); });
	auto ret_readopt = run_test([&] { test<std::true_type>(argc, argv); });
	return (ret_writeopt && ret_readopt);

I think we might add a pmreorder test for checking data visibility. The idea would be to run something like this under pmreorder:

void run() {
ptr = nullptr; // (initial value)
ptr.store(0xABCD);
}

and in second application thread:

auto r = ptr.load();
pmem->read = r;

If, on check_consistency test the pmem->read is different from what is actually stored in the ptr this means there is some kind of data inconsistency.

I'll think about it and share with you later.

Reviewable status: 1 of 3 files reviewed, 17 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, and @yjrobin)

-- commits, line 6 at r1:

We can squash them when merged.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 2 at r1 (raw file):

Done.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 6 at r1 (raw file):

Done.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 29 at r1 (raw file):

Done.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 29 at r1 (raw file):

Yes, you are right it is not pointer-specific. Seems like a good idea to make it more generic. It can support any data which is 8-byte aligned and smaller than 8 bytes (to make sure the atomicity of the update and flush).

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 55 at r1 (raw file):

Done.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 84 at r1 (raw file):

Done.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 94 at r1 (raw file):

Done.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 99 at r1 (raw file):

Done.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 122 at r1 (raw file):

Actually, I don't expect this atomic_persistent_aware_ptr to be used in this way. I'll try to implement these functions. The logic of these functions are a bit more complicated than that of store/load. Working on it.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 297 at r1 (raw file):

Agree. I'll remove it.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 2 at r1 (raw file):

Done.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 28 at r1 (raw file):

I'll use atomic_backoff to see if the problem disappears.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 52 at r1 (raw file):

Done.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 83 at r1 (raw file):

Done.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 87 at r1 (raw file):

Done.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 105 at r1 (raw file):

Working on it. If you have any suggestions, please share with me. Thanks.

Reviewable status: 1 of 3 files reviewed, 17 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 29 at r1 (raw file):

Great. Would you like to do this on this PR or it should next step? I think it might be next step because we would probably need to change the DIRTY flag to be MSB instead of LSB (for integer values, LSB could change the value).

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 94 at r1 (raw file):

You should also call pause somewhere at the end of this loop.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 122 at r1 (raw file):

I see, in that case, our first step could be to just remove those functions from the class. I would like for all functions of this atomic to provide the same visibility/persistency rules. What do you think about this?

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 175 at r2 (raw file):

	 */
	bool
	compare_exchange_weak(value_type &expected, value_type desired,

Hm, what if the implementation would just return false whenever CAS fails (including when it's just because someone cleared the dirty flag). The code would be probably easier and from the user perspective, in most case it should not matter why the CAS failed I think - the solution is very often to just try again.

Reviewable status: 1 of 3 files reviewed, 17 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 29 at r1 (raw file):

I prefer to do this at next step. The flag bit needs to chosen based on the bit usage of the underlying data, cannot be easily generalized.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 94 at r1 (raw file):

Sorry about missing the most important one :) Done.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 122 at r1 (raw file):

I agree, the 'visibility == persistence' is the key of this atomic ptr.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 175 at r2 (raw file):

I think we also need two versions (read/write optimized) for the functions we removed for now. E.g., for read-optimized ptr, store will never clear the dirty flag, so if load is not called, CAS will never succeed. In this case, CAS should take care of the failure when it's due to the dirty flag.
We can discuss later if we decide to add exchange, CAS and fetch to this special atomic ptr.

Reviewable status: 1 of 3 files reviewed, 13 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 175 at r2 (raw file):

Agree, let's talk about this later.

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @igchor and @yjrobin)

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 28 at r1 (raw file):

Windows build went just fine, so it looks the issue disappeared.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 87 at r1 (raw file):

I can see you updated strcat, but what about the strcpy? it should copy max 100-5-1=94 chars, I guess (otherwise our static analysis will be throwing error here 😉 )

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, and @yjrobin)

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 87 at r1 (raw file):

You coud also probably just use C++ features: std::string(path) + BoolToString(ReadOptimized::value)

Reviewed 1 of 3 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @igchor, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 36 at r1 (raw file):

		std::atomic<std::ptrdiff_t>>;

	static constexpr uintptr_t IS_DIRTY = 1;

Why uintptr_t? It wasn't designed for "flags".

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 112 at r3 (raw file):

for (detail::atomic_backoff backoff(true);;)

in while(true) we trust:

detail::atomic_backoff backoff(true)
for (;;) {/*code*/}

->

detail::atomic_backoff backoff(true)
while (true) {/*code*/}

Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 36 at r1 (raw file):

This flag is used for bitwise operation on pointers (element */void *), so I think it should be fine.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 112 at r3 (raw file):

Done.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 87 at r1 (raw file):

Done.

Reviewable status: 1 of 3 files reviewed, 7 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 113 at r4 (raw file):

		if (is_dirty(val)) {
			detail::atomic_backoff backoff(true);
			while (true) {

Actually, with @lukaszstolarczuk we found that this code is not correct. If crash happens during store (after we issued store with dirty flag but before calling compare_exchange_strong to clear the flag), on restart the reader will spin forever (there will be no one to clear the flag). I think the implementation should be just:

auto val = ptr.load(order);
if (is_dirty(val)
    pool_by_vptr(this).persist(&ptr, sizeof(ptr));
return clear_dirty(val);

This will ensure we are returning value which is definitely persisted but avoid infinite loop. The cost of persist should be comparable to spinning anyway imho.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 135 at r4 (raw file):

	{
		auto val = ptr.load(order);
		while (is_dirty(val)) {

I'm not sure if we really need this loop here. Maybe it would be better to just do one iteration, and if compare_exchange_strong fails, we just return clear_dirty(val) and rely on subsequent reader to clear the flag?

Reviewable status: 1 of 3 files reviewed, 7 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 113 at r4 (raw file):

I've added the fix in my PR, but feel free to apply it already here - ref. #1217

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 69 at r4 (raw file):

template <typename ReadOptimized>
void
test_ptr_visibility(nvobj::pool<root<ReadOptimized>> &pop)

am I missing something or these tests are exactly the same...?

Reviewable status: 1 of 3 files reviewed, 7 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 113 at r4 (raw file):

Good point. To be honest, I have not thought thoroughly about this ReadOptimized version because my original design of this ptr is WriteOptimized only, where the reader is responsible for flushing and clearing the dirty flag to make sure the flush is necessary and executed only once. And it also needs the user's attention (e.g. the swmr-skiplist) to deal with dirty ptrs upon recovery.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 135 at r4 (raw file):

If CAS fails, either someone updated it with a new dirty one, or someone flushed it and clear the flag. Just return the clear_dirty(val) may be ok with the later, but it is incorrect with the former case where the returned ptr no longer exists. The while loop can cover both cases.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 105 at r1 (raw file):

I think we can use a simple list to mimic the swmr scenario, and add the recovery process before checking the data visibility/consistency.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 69 at r4 (raw file):

The implementation of test cases is incomplete.

Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 113 at r4 (raw file):

Yes, that's true. So, maybe the reader should be implemented in the same way for both ReadOptimized and WriteOptimized cases, we would just change the store() implementation? In general, if store() actually clears the dirty flag it should be rather uncommon for readers to see this dirty flag set.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 135 at r4 (raw file):

Why it's incorrect? I think it's basically the same as:

ptr.store(0);

thread 1                    | thread 2
auto val = ptr.load(order)  | 
                            | ptr.store(1);
return val; // will return 0|

The first thread will also return value which is no longer valid (but it was at some point in time). Whether the first thread will call compare_exchange_strong before returning should not make any difference I think. Imo we don't violate any ordering rules by returning value which is slightly overrated. The only difference to regular, non-persistent atomic pointers which we must provide is ensuring that what we return is actually persistent.

Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 113 at r4 (raw file):

For correctness, I think we can unify the implementation of load() now. We can evaluate the difference in performance test later.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 135 at r4 (raw file):

I'm thinking a case like follows:

auto val = ptr.load(order); 
while (is_dirty(val)) {
	pool_by_vptr(this).persist(&ptr, sizeof(ptr)); // if the writer update ptr to val2 before the flush here, we will be flushing val2.
	auto clean_val = clear_dirty(val);
	if (ptr.compare_exchange_strong(val, clean_val, order)) // then the CAS will fail, but if we return clear_dirty(val), 
//it will be a problem as we return an obsolete ptr (val) which is not the same as the persistent one (val2)
		return clean_val;
}

The while loop here makes sure we will not return the wrong ptr, if CAS failed, it will assign the value of the current ptr to val and start over again. This extreme case is like there are two consequent updates, before the first one being persistent, the second one has overwritten it. IMO, the first one should not be seen by any readers. What do you think?

Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, and @yjrobin)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 113 at r4 (raw file):

Ok, so let's do that for now. And for store() we can keep the 2 implementations as we have right now.

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 135 at r4 (raw file):

My understanding is as follows: modifications performed on the ptr create some sort of linear history, for example like this:

nullptr | val | val2 | // all values before val2 are part of committed 'history'

Now, we could think of this history as a log in a database. What we would like to achieve (I think) is something similar to "read committed" guarantee. This means that we cannot read uncommited (not-persisted) values. In your example, this is not violated because since val was overwritten with val2 and val2 was persisted, val can also be treated as part of this history of committed values.

Do you see any particular cases where this would be problematic?

Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

include/libpmemobj++/experimental/atomic_persistent_aware_ptr.hpp, line 135 at r4 (raw file):

I understand what you mean. After second thought, I think we can just use "if" here for now. We can discuss this later if we run into problems when we use it to implement SWMR skip-list. I'll merge these changes and push it.

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 105 at r1 (raw file):

This will done at #1217

Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

@yjrobin the code looks good, could you please just fix the test (see the comment for test) and squash all commits into one? After that, we can merge the PR.

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @KFilipek, @lukaszstolarczuk, and @yjrobin)

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 47 at r5 (raw file):

		nvobj::transaction::run(pop, [&] {
			UT_ASSERT(r->ptr.load() == nullptr);
			r->ptr = nvobj::make_persistent<int>();

The test fails on this line because this assignment does not add r->ptr to a tx. I think the best way to fix this would be to follow a recommendation from this blog post: https://pmem.io/2021/09/17/concurrency.html (part about Publishing allocations), i.e. to create an additional, non-atomic ptr to which we would assign the result of make_persistnt (inside the tx). And then, outside tx, we would call r->ptr.store(non_atomic_ptr); Something like this:

struct root {
    atomic_ptr<int, ReadOptimized> ptr;
    ptr<int> non_atomic_ptr;
};

...

nvobj::transaction::run(pop, [&] {
    ...
    r->non_atomic_ptr = nvobj::make_persistent<int>();
});

r->ptr.store(r->non_atomic_ptr);

...

I've fixed the test, please see the comments there. I've also merged the recent commits in pmem:master. If that's ok for you, please merge the PR. Squash commits can be done when merging.

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

tests/ptr/atomic_persistent_aware_ptr_pmem.cpp, line 47 at r5 (raw file):

Thanks for the tips. I've also modified the pptr deletion accordingly.