Clear memory in a predictable and atomic manner when changing type.

Clear memory in a predictable and atomic manner when changing type.

There may be existing pointers to the memory block that are being actively
accessed while the zeroing of memory is taking place. While this change
doesn't fix that, it does make it possible for those other threads to recognize
that memory is changing under it and be able to react appropriately.

Also, restore missing kTypeIdAny that was somehow lost.

BUG=620813
Review-Url: https://codereview.chromium.org/2709113003
Cr-Commit-Position: refs/heads/master@{#456121}
Committed: https://chromium.googlesource.com/chromium/src/+/bee49a23fbe4ce847798aa87dff47440b07c097b

[bcwhite, 2017-02-22 22:57:10]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 22:58:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-02-22 23:30:00]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org

[commit-bot: I haz the power, 2017-02-23 01:02:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 01:02:20]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-02-24 16:16:22]

Can you provide more context for why it's needed? The associated bug is just the
main persistent histograms bug and so it's not clear to me what this is for. Is
it to support some upcoming feature from breadcrumbs? If so, it should probably
be associated with that bug.

[bcwhite, 2017-02-24 20:22:28]

Description was changed from

==========
Clear memory in an atomic manner when changing type.

Also, restore missing kTypeIdAny that was somehow lost.

BUG=546019
==========

to

==========
Clear memory in a predictable and atomic manner when changing type.

There may be existing pointers to the memory block that are being actively
accessed while the zeroing of memory is taking place. While this change
doesn't fix that, it does make it possible for those other threads to recognize
that memory is changing under it and be able to react appropriately.

Also, restore missing kTypeIdAny that was somehow lost.

BUG=620813
==========

[bcwhite, 2017-02-24 20:22:44]

On 2017/02/24 16:16:22, Alexei Svitkine (slow) wrote:
> Can you provide more context for why it's needed? The associated bug is just
the
> main persistent histograms bug and so it's not clear to me what this is for.
Is
> it to support some upcoming feature from breadcrumbs? If so, it should
probably
> be associated with that bug.

Description and bug# updated.

[bcwhite, 2017-03-06 14:43:23]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 14:43:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-03-06 16:03:58]

On 2017/02/24 20:22:44, bcwhite wrote:
> On 2017/02/24 16:16:22, Alexei Svitkine (slow) wrote:
> > Can you provide more context for why it's needed? The associated bug is just
> the
> > main persistent histograms bug and so it's not clear to me what this is for.
> Is
> > it to support some upcoming feature from breadcrumbs? If so, it should
> probably
> > be associated with that bug.
> 
> Description and bug# updated.

Ping?  Context was added to the description.

[commit-bot: I haz the power, 2017-03-06 16:12:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 16:12:30]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-03-06 22:10:28]

I don't see how this is helping anything.

If the API is indeed unsafe, which sounds like it is - should we provide this
API at all?

Also, if the goal is to just make it easier to diagnose problems when this
happens - e.g. in crash reports - I think it would be better to instead clear
the memory with a well define bogus pattern - e.g. DEADBEEF or similar to make
it easier to spot when debugging.

[bcwhite, 2017-03-06 23:03:10]

On 2017/03/06 22:10:28, Alexei Svitkine (very slow) wrote:
> I don't see how this is helping anything.
> 
> If the API is indeed unsafe, which sounds like it is - should we provide this
> API at all?

The call isn't unsafe but clearing memory in an unpredictable pattern (as memset
may do) limits some cross-thread concurrency options.

Here's the situation:

Thread A is managing memory segment X and writes three numbers to the first
three words of that segment: X1, X2, and X3.  X1 is known to be non-zero.

Thread B is watching that same memory segment and wants to take a snapshot for
analysis purposes.  It atomically reads X1 and verifies it's non-zero.  It then
reads X2 and X3 which are known not to change as long as X1 hasn't changed, then
goes back and atomically re-reads X1.  As long as X1 matches what was read from
it previously then the values read from X2 and X3 are also valid.

Without a predictable memory clear that is atomic and monotonically increasing,
it would be possible to clear X2 and X3 before X1 meaning there is no way to
detect that the read values are valid.

By doing an atomic clear that is in-order, it provides options for thread B to
detect if thread A has changed the type of the memory segment (typically marking
it as "free").

Having thread B check the type ID before and after isn't sufficient if thread A
could free and then "allocate" it again by changing the type back to the
original value (the "ABA" concurrency problem).  However, if thread A guarantees
that the X1 value will always be non-zero and different in successive
allocations then it's still possible to detect that the data has changed.

Most of this is outside the scope of the persistent memory allocator; it just
needs to behave in a guaranteed manner so that these types of constructs are
possible.


> Also, if the goal is to just make it easier to diagnose problems when this
> happens - e.g. in crash reports - I think it would be better to instead clear
> the memory with a well define bogus pattern - e.g. DEADBEEF or similar to make
> it easier to spot when debugging.

Nothing to do with that.

[Alexei Svitkine (slow), 2017-03-10 16:32:20]

On 2017/03/06 23:03:10, bcwhite wrote:
> On 2017/03/06 22:10:28, Alexei Svitkine (very slow) wrote:
> > I don't see how this is helping anything.
> > 
> > If the API is indeed unsafe, which sounds like it is - should we provide
this
> > API at all?
> 
> The call isn't unsafe but clearing memory in an unpredictable pattern (as
memset
> may do) limits some cross-thread concurrency options.
> 
> Here's the situation:
> 
> Thread A is managing memory segment X and writes three numbers to the first
> three words of that segment: X1, X2, and X3.  X1 is known to be non-zero.
> 
> Thread B is watching that same memory segment and wants to take a snapshot for
> analysis purposes.  It atomically reads X1 and verifies it's non-zero.  It
then
> reads X2 and X3 which are known not to change as long as X1 hasn't changed,
then
> goes back and atomically re-reads X1.  As long as X1 matches what was read
from
> it previously then the values read from X2 and X3 are also valid.

I don't see why this example wouldn't be susceptible to the ABA problem.

What's stopping a B happening between the first read of X1 and the latter read
and compare of it to the previous value?

> 
> Without a predictable memory clear that is atomic and monotonically
increasing,
> it would be possible to clear X2 and X3 before X1 meaning there is no way to
> detect that the read values are valid.
> 
> By doing an atomic clear that is in-order, it provides options for thread B to
> detect if thread A has changed the type of the memory segment (typically
marking
> it as "free").
> 
> Having thread B check the type ID before and after isn't sufficient if thread
A
> could free and then "allocate" it again by changing the type back to the
> original value (the "ABA" concurrency problem).  However, if thread A
guarantees
> that the X1 value will always be non-zero and different in successive
> allocations then it's still possible to detect that the data has changed.
> 
> Most of this is outside the scope of the persistent memory allocator; it just
> needs to behave in a guaranteed manner so that these types of constructs are
> possible.

But why do we need to support such constructs? What's the concrete use case this
enables? If there isn't enough, we shouldn't do it until we need it.

> 
> 
> > Also, if the goal is to just make it easier to diagnose problems when this
> > happens - e.g. in crash reports - I think it would be better to instead
clear
> > the memory with a well define bogus pattern - e.g. DEADBEEF or similar to
make
> > it easier to spot when debugging.
> 
> Nothing to do with that.

[Alexei Svitkine (slow), 2017-03-10 17:41:54]

On 2017/03/10 16:32:20, Alexei Svitkine (very slow) wrote:
> On 2017/03/06 23:03:10, bcwhite wrote:
> > On 2017/03/06 22:10:28, Alexei Svitkine (very slow) wrote:
> > > I don't see how this is helping anything.
> > > 
> > > If the API is indeed unsafe, which sounds like it is - should we provide
> this
> > > API at all?
> > 
> > The call isn't unsafe but clearing memory in an unpredictable pattern (as
> memset
> > may do) limits some cross-thread concurrency options.
> > 
> > Here's the situation:
> > 
> > Thread A is managing memory segment X and writes three numbers to the first
> > three words of that segment: X1, X2, and X3.  X1 is known to be non-zero.
> > 
> > Thread B is watching that same memory segment and wants to take a snapshot
for
> > analysis purposes.  It atomically reads X1 and verifies it's non-zero.  It
> then
> > reads X2 and X3 which are known not to change as long as X1 hasn't changed,
> then
> > goes back and atomically re-reads X1.  As long as X1 matches what was read
> from
> > it previously then the values read from X2 and X3 are also valid.
> 
> I don't see why this example wouldn't be susceptible to the ABA problem.
> 
> What's stopping a B happening between the first read of X1 and the latter read
> and compare of it to the previous value?

Per offline discussion, this can only work if there's a constraint on how X1 is
assigned - i.e. user code must make sure not to re-use this - the more concrete
use case is an id field that's monotonically increasing.

> 
> > 
> > Without a predictable memory clear that is atomic and monotonically
> increasing,
> > it would be possible to clear X2 and X3 before X1 meaning there is no way to
> > detect that the read values are valid.
> > 
> > By doing an atomic clear that is in-order, it provides options for thread B
to
> > detect if thread A has changed the type of the memory segment (typically
> marking
> > it as "free").
> > 
> > Having thread B check the type ID before and after isn't sufficient if
thread
> A
> > could free and then "allocate" it again by changing the type back to the
> > original value (the "ABA" concurrency problem).  However, if thread A
> guarantees
> > that the X1 value will always be non-zero and different in successive
> > allocations then it's still possible to detect that the data has changed.
> > 
> > Most of this is outside the scope of the persistent memory allocator; it
just
> > needs to behave in a guaranteed manner so that these types of constructs are
> > possible.
> 
> But why do we need to support such constructs? What's the concrete use case
this
> enables? If there isn't enough, we shouldn't do it until we need it.

Per offline discussion, this is for breadcrumbs where entries get recycled a lot
and
we want to support live analysis.

> 
> > 
> > 
> > > Also, if the goal is to just make it easier to diagnose problems when this
> > > happens - e.g. in crash reports - I think it would be better to instead
> clear
> > > the memory with a well define bogus pattern - e.g. DEADBEEF or similar to
> make
> > > it easier to spot when debugging.
> > 
> > Nothing to do with that.

[Alexei Svitkine (slow), 2017-03-10 17:42:01]

lgtm

[bcwhite, 2017-03-10 17:45:37]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2017-03-10 17:46:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 18:58:53]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489167937040520,
"parent_rev": "9558316be2419589a7895300825e9b9494dd06eb", "commit_rev":
"bee49a23fbe4ce847798aa87dff47440b07c097b"}

[commit-bot: I haz the power, 2017-03-10 18:59:48]

Description was changed from

==========
Clear memory in a predictable and atomic manner when changing type.

There may be existing pointers to the memory block that are being actively
accessed while the zeroing of memory is taking place. While this change
doesn't fix that, it does make it possible for those other threads to recognize
that memory is changing under it and be able to react appropriately.

Also, restore missing kTypeIdAny that was somehow lost.

BUG=620813
==========

to

==========
Clear memory in a predictable and atomic manner when changing type.

There may be existing pointers to the memory block that are being actively
accessed while the zeroing of memory is taking place. While this change
doesn't fix that, it does make it possible for those other threads to recognize
that memory is changing under it and be able to react appropriately.

Also, restore missing kTypeIdAny that was somehow lost.

BUG=620813

Review-Url: https://codereview.chromium.org/2709113003
Cr-Commit-Position: refs/heads/master@{#456121}
Committed:
https://chromium.googlesource.com/chromium/src/+/bee49a23fbe4ce847798aa87dff4...
==========

[commit-bot: I haz the power, 2017-03-10 18:59:49]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/bee49a23fbe4ce847798aa87dff4...