examples: add missing VALGRIND_SET_CLEAN() to ringbuf libpmem2 example

[ldorau]

Add missing VALGRIND_SET_CLEAN() to ringbuf libpmem2 example (found by pmemcheck).

Ref: #5598

---

This change is 

[codecov]

Codecov Report

Merging #5604 (4262f88) into stable-1.13 (01eb891) will increase coverage by 1.57%.
The diff coverage is n/a.

❗ Current head 4262f88 differs from pull request most recent head 204229f. Consider uploading reports for the commit 204229f to get more accurate results

@@               Coverage Diff               @@
##           stable-1.13    #5604      +/-   ##
===============================================
+ Coverage        72.68%   74.25%   +1.57%     
===============================================
  Files              145      145              
  Lines            22634    22131     -503     
  Branches          3771     3705      -66     
===============================================
- Hits             16451    16433      -18     
+ Misses            6183     5698     -485     

[ldorau]

Rebased to pmem:stable-1.13.

[grom72]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ldorau)

[janekmi]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ldorau)

---

src/examples/libpmem2/ringbuf/ringbuf.c line 168 at r1 (raw file):

	ringbuf_data_force_page_allocation(rbuf->data, size);
	rbuf->persist(rbuf->data, sizeof(*rbuf->data));

As far as I can tell, the above is just a performance optimization, the persistent state remains intact.

It doesn't feel right to persist a block just to make Valgrind silent. I don't see any other argument for calling here persist. Am I missing something?

Do we have any recommended way of dealing with this kind of situation?

[janekmi]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ldorau)

---

src/examples/libpmem2/ringbuf/ringbuf.c line 168 at r1 (raw file):

	ringbuf_data_force_page_allocation(rbuf->data, size);
	rbuf->persist(rbuf->data, sizeof(*rbuf->data));

With the proposed fix I still got an issue:

Number of stores not made persistent: 2
Stores not made persistent properly:
[0]    at 0x401064: ringbuf_data_force_page_allocation (ringbuf.c:88)
   by 0x401308: ringbuf_new (ringbuf.c:167)
   by 0x401960: main (ringbuf.c:431)
	Address: 0x7cfe000	size: 1	state: DIRTY
[1]    at 0x401064: ringbuf_data_force_page_allocation (ringbuf.c:88)
   by 0x401308: ringbuf_new (ringbuf.c:167)
   by 0x401960: main (ringbuf.c:431)
	Address: 0x7cff000	size: 1	state: DIRTY
Total memory not made persistent: 2
ERROR SUMMARY: 2 errors

It looks like the following solves the issue definitely.

ringbuf_data_force_page_allocation(rbuf->data, size);
rbuf->persist(rbuf->data, size);

[ldorau]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @grom72 and @janekmi)

---

src/examples/libpmem2/ringbuf/ringbuf.c line 168 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

With the proposed fix I still got an issue:

Number of stores not made persistent: 2
Stores not made persistent properly:
[0]    at 0x401064: ringbuf_data_force_page_allocation (ringbuf.c:88)
   by 0x401308: ringbuf_new (ringbuf.c:167)
   by 0x401960: main (ringbuf.c:431)
	Address: 0x7cfe000	size: 1	state: DIRTY
[1]    at 0x401064: ringbuf_data_force_page_allocation (ringbuf.c:88)
   by 0x401308: ringbuf_new (ringbuf.c:167)
   by 0x401960: main (ringbuf.c:431)
	Address: 0x7cff000	size: 1	state: DIRTY
Total memory not made persistent: 2
ERROR SUMMARY: 2 errors

It looks like the following solves the issue definitely.

ringbuf_data_force_page_allocation(rbuf->data, size);
rbuf->persist(rbuf->data, size);

Right. Done.

[ldorau]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @grom72, @janekmi, and @pbalcer)

---

src/examples/libpmem2/ringbuf/ringbuf.c line 168 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

As far as I can tell, the above is just a performance optimization, the persistent state remains intact.

It doesn't feel right to persist a block just to make Valgrind silent. I don't see any other argument for calling here persist. Am I missing something?

Do we have any recommended way of dealing with this kind of situation?

This persist is needed only to silent Valgrind, because the writes in ringbuf_data_force_page_allocation() are made only to ensure that all the pages for the ring buffer data are allocated (by rewriting a byte from each page), so they do not need to be persisted at all. I do not know how make Valgrind know that ...

@pbalcer Do you know how to make Valgrind not require to persist a store?

[janekmi]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @grom72 and @pbalcer)

[janekmi]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @grom72 and @pbalcer)

---

src/examples/libpmem2/ringbuf/ringbuf.c line 168 at r1 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

This persist is needed only to silent Valgrind, because the writes in ringbuf_data_force_page_allocation() are made only to ensure that all the pages for the ring buffer data are allocated (by rewriting a byte from each page), so they do not need to be persisted at all. I do not know how make Valgrind know that ...

@pbalcer Do you know how to make Valgrind not require to persist a store?

We are probably looking for VALGRIND_PMC_SET_CLEAN().

[janekmi]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @grom72 and @pbalcer)

---

src/examples/libpmem2/ringbuf/ringbuf.c line 168 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

We are probably looking for VALGRIND_PMC_SET_CLEAN().

Or even VALGRIND_SET_CLEAN().

[janekmi]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @grom72 and @pbalcer)

---

src/examples/libpmem2/ringbuf/ringbuf.c line 168 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Or even VALGRIND_SET_CLEAN().

https://github.com/pmem/pmdk/blob/master/src/common/set.c#L149

🤣

[ldorau]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @grom72 and @pbalcer)

---

src/examples/libpmem2/ringbuf/ringbuf.c line 168 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

https://github.com/pmem/pmdk/blob/master/src/common/set.c#L149

🤣

:-D Done.

[janekmi]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @grom72, @ldorau, and @pbalcer)

---

src/examples/libpmem2/ringbuf/ringbuf.c line 237 at r3 (raw file):

	if (rbuf->granularity == PMEM2_GRANULARITY_BYTE) {
		__atomic_store_n(pos, val, __ATOMIC_RELEASE);
		VALGRIND_SET_CLEAN(pos, sizeof(val));

Oh. Why do you think persist is not necessary here? 🤔

[janekmi]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72, @ldorau, and @pbalcer)

---

src/examples/libpmem2/ringbuf/ringbuf.c line 168 at r1 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

:-D Done.

😆

Alright. I think this would be the most satisfying solution but it has not been done till now. And I reckon it might be tricky to do it right. So, I propose to proceed as follows:

ringbuf_data_force_page_allocation(rbuf->data, size);
// The persist below is unnecessary for the persistency correctness.
// It is needed only to work around Valgrind's pmemcheck limitation.
// XXX the following could be potentially replaced with VALGRIND_SET_CLEAN().
rbuf->persist(rbuf->data, sizeof(*rbuf->data));

---

src/examples/libpmem2/ringbuf/ringbuf.c line 237 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Oh. Why do you think persist is not necessary here? 🤔

And here I am pretty sure a simple:

rbuf->persist(pos, sizeof(val));

is needed. As you have proposed initially.

[ldorau]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72, @janekmi, and @pbalcer)

---

src/examples/libpmem2/ringbuf/ringbuf.c line 168 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

😆

Alright. I think this would be the most satisfying solution but it has not been done till now. And I reckon it might be tricky to do it right. So, I propose to proceed as follows:

ringbuf_data_force_page_allocation(rbuf->data, size);
// The persist below is unnecessary for the persistency correctness.
// It is needed only to work around Valgrind's pmemcheck limitation.
// XXX the following could be potentially replaced with VALGRIND_SET_CLEAN().
rbuf->persist(rbuf->data, sizeof(*rbuf->data));

This persist is unnecessary, because ringbuf_data_force_page_allocation() performs 1 byte stores just to force allocation of pages and those stores do not need to be persisted. So the code is correct.

In order to be able to run this code under pmemcheck two VALGRIND_SET_CLEAN() calls would have to be added (as they are currently).
However, it seems that examples do not use any valgrind macros on purpose (to avoid unnecessary complication), so I believe this example just should not be run under pmemcheck and the correct fix is following: #5618 (and this pull request should be closed).

---

src/examples/libpmem2/ringbuf/ringbuf.c line 237 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

And here I am pretty sure a simple:

rbuf->persist(pos, sizeof(val));

is needed. As you have proposed initially.

It is explained above:

	 * However, if the map can be persistently written to with byte
	 * granularity (i.e., the system is eADR equipped), then data
	 * visibility is the same as data persistence. This eliminates the
	 * need for the persistent flag algorithm.

Code quote:

	 * However, if the map can be persistently written to with byte
	 * granularity (i.e., the system is eADR equipped), then data
	 * visibility is the same as data persistence. This eliminates the
	 * need for the persistent flag algorithm.
	 */

[ldorau]

Replaced with #5618