vsc: Always perform volatile access to VSC values

[dridi]

From @asadsa92's recent review of #3483:

Any side-effect to get rid off the volatile pointer?

There used to be two locations where pt->ptr was dereferenced as a volatile pointer and considering the effective volatility of VSCs we should probably generalize it in VSC_Value() where it was centralized.

[nigoroll]

Can anyone help me understand the motivation? In my mind, the time scale of outdated values for non volatile access should be irrelevant to vsc clients.

[asadsa92]

So, I only mentioned this as part of my review as I also wanted to understand whether the change was intended or simply a oversight.

Now looking:

• The declaration is volatile: https://github.com/varnishcache/varnish-cache/blob/master/include/vapi/vsc.h#L52
• The only place, which I could find, were it is updated: https://github.com/varnishcache/varnish-cache/blob/master/lib/libvarnishapi/vsc.c#L310 . This is updated through VSC_Iter -> vsc_add_seg -> vsc_fill_point

VSC_Iter is only called from VSC clients and the same goes for VSC_Value. So, I think @nigoroll might be correct that this is not needed. The compiler "sees" all modifications of pt->ptr and therefore should be free to optimize the access. Unless, we expect the libvarnishapi to change separately from the VSC clients in the installation. The point is that we have no external access to pt->ptr so volatile should no be needed.

EDIT: I was able to find the origin of the volatile member: https://github.com/varnishcache/varnish-cache/commit/f25134aa6eff8ea23a6c64cb2c0123b7ae853589 (11 years ago, AFAICT it has always been volatile) @bsdphk, maybe you got some input here that I might have overseen.

[nigoroll]

@asadsa92 thank you for the clarification and research.

Within varnishd (at the write end for statistics), volatile kind-of makes sense: Because we update values (e.g. increase/decrese), outdated values cause inaccurate statistics.

But volatile is not the solution, concurrent access can still happen and will cause wrong values. Thus, we already serialize access to the most important statistics, and for this case volatile is useless and wasteful.

So I think a proper solution would be to have stats with

• either a non-volatile declaration and locking or
• an atomic declaration.

[bsdphk]

"Within varnishd (at the write end for statistics), volatile kind-of makes sense: Because we update values (e.g. increase/decrese), outdated values cause inaccurate statistics."

That volatile in varnishd is to make sure the VSM is updated in at timely matter, as @nigoroll mentions, it is not enough to protect the read-modify-write operation itself, we use locks for that.

Similarly on the client side, we need volatile to explain to the compiler that the contents of the VSM changes under us.

To what extent they actually make any difference in the real world is a damn good question, which is impossible to find the answer to, except by disassembly of suspected code.

[nigoroll]

Does this PR still serve a purpose?

[bsdphk]

As far as I can tell, the proposed volatile makes no difference, so timing out this ticket.