Added a way for sync point clients to issue out of order waits.

Added a way for sync point clients to issue out of order waits.

This patch fixes a subtle bug that could occur if a sync point client
is waiting without a corresponding order number. This can occur for any
wait that is not done within the usual command buffer processing loop.
An example is the command buffer stub OnSignalSyncToken() function
which issues a wait directly on the IO thread.

Previously, a similar concept was introduced in SyncPointClientWaiter
which was used to wait out of order when a Gpu Memory Buffer was
destroyed. Because of the need for this functionality in the regular
SyncPointClient, I have folded the SyncPointClientWaiter functions into
SyncPointClient under WaitOutOfOrder() & WaitOutOfOrderNonThreadSafe().

Because of how subtle this bug is, further state tracking is now done
so we can test whether or not an order number is processing or not.
Using that test, DCHECKs have been added so Wait()/WaitNonThreadSafe
can only be called when processing an order number, and the the out of
order variants can only be called when not processing an order number.

BUG=514815
TEST=Added unit tests to test SignalSyncToken().

Committed: https://crrev.com/563fb212fda4ad5ea477b754d628527e3f20010f
Cr-Commit-Position: refs/heads/master@{#368441}

[David Yen, 2016-01-06 18:45:48]

dyen@chromium.org changed reviewers:
+ piman@chromium.org, sky@chromium.org

[David Yen, 2016-01-06 18:45:49]

Sorry for another big semi-complex change, but I found a subtle bug with
SignalSyncToken. Hopefully the CL description is clear enough, but it boils down
to waiting without an assigned global order number which may turn the wait into
a NOP if no commands are in the queue.

+piman@ for general review.

+sky@ for mojo changes, we simply need to call PauseProcessingOrderNumber() when
an order number is not completed.

CC penghuang@, I've added extra state tracking for order number processing so
you may need to add PauseProcessingOrderNumber() if you are pausing command
processing in your other CL.

[piman, 2016-01-07 01:16:22]

https://codereview.chromium.org/1568563002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1568563002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:1013: // within the global order.
I don't think that's true, this is processed on the main thread, just like every
other message (it would be a good idea to do on the IO thread, actually, but we
don't do that now).

https://codereview.chromium.org/1568563002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1568563002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:952:
sync_point_client_->WaitOutOfOrder(
ditto, how is this different than WaitFenceSyncOnGpuThread in terms of ordering?

[piman, 2016-01-07 01:25:59]

Actually, thinking a bit more, the difference between SignalSyncToken and
WaitSyncToken is that the former doesn't block execution, and so can't
cause deadlocks on the GPU process. This has a few consequences:
- it is somewhat safe to wait for an unverified/future sync point (the
callback may never fire, the client needs to be able to handle that)
- it *can* be done outside the message processing
(e.g. GpuChannelManager::OnDestroyGpuMemoryBuffer), but doesn't have to
(e.g. GpuCommandBufferStub::OnSignalSyncToken)
- it *could* be done from other threads (though SyncPointClient isn't
thread-safe).

So, I think the code is close to correct (except the
DCHECK(!client_state_->order_data()->IsProcessingOrderNumber())
inside SyncPointClient::WaitOutOfOrder is overly strict, and I think breaks
for OnSignalSyncToken), but the comments are misleading I think.

On Wed, Jan 6, 2016 at 5:16 PM, <piman@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1568563002/diff/40001/content/common/gpu/gpu_...
> File content/common/gpu/gpu_command_buffer_stub.cc (right):
>
>
>
https://codereview.chromium.org/1568563002/diff/40001/content/common/gpu/gpu_...
> content/common/gpu/gpu_command_buffer_stub.cc:1013: // within the global
> order.
> I don't think that's true, this is processed on the main thread, just
> like every other message (it would be a good idea to do on the IO
> thread, actually, but we don't do that now).
>
>
>
https://codereview.chromium.org/1568563002/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/in_process_command_buffer.cc (right):
>
>
>
https://codereview.chromium.org/1568563002/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/in_process_command_buffer.cc:952:
> sync_point_client_->WaitOutOfOrder(
> ditto, how is this different than WaitFenceSyncOnGpuThread in terms of
> ordering?
>
> https://codereview.chromium.org/1568563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Yen, 2016-01-07 18:40:58]

https://codereview.chromium.org/1568563002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1568563002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:1013: // within the global order.
On 2016/01/07 01:16:22, piman (Slow to review) wrote:
> I don't think that's true, this is processed on the main thread, just like
every
> other message (it would be a good idea to do on the IO thread, actually, but
we
> don't do that now).

You are right, this should be Wait instead of WaitOutOfOrder. I forgot that the
command buffer stub runs on the main thread and has an order number attached to
every command. That is not true for the InProcess case though.

Alternatively we can do this on the IO thread using WaitOutOfOrder if we want
to. I will change it back for now though.

https://codereview.chromium.org/1568563002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1568563002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:952:
sync_point_client_->WaitOutOfOrder(
On 2016/01/07 01:16:22, piman (Slow to review) wrote:
> ditto, how is this different than WaitFenceSyncOnGpuThread in terms of
ordering?

Here because order numbers are only generated in Flush(), even though
SignalSyncToken is run "in order" it does not have a proper order number
associated with the wait. Since WaitFenceSyncOnGpuThread is called by the
decoder it is properly managed.

Another way to solve this (since we do not currently support descheduling which
means they currently happen to run in order) is to also generate an order number
here around the Wait(), but WaitOutOfOrder() is simpler I think. This also opens
up the possibility of being able to deschedule commands.

[David Yen, 2016-01-07 18:47:58]

On 2016/01/07 01:25:59, piman (Slow to review) wrote:
> Actually, thinking a bit more, the difference between SignalSyncToken and
> WaitSyncToken is that the former doesn't block execution, and so can't
> cause deadlocks on the GPU process. This has a few consequences:
> - it is somewhat safe to wait for an unverified/future sync point (the
> callback may never fire, the client needs to be able to handle that)
> - it *can* be done outside the message processing
> (e.g. GpuChannelManager::OnDestroyGpuMemoryBuffer), but doesn't have to
> (e.g. GpuCommandBufferStub::OnSignalSyncToken)
> - it *could* be done from other threads (though SyncPointClient isn't
> thread-safe).
> 
> So, I think the code is close to correct (except the
> DCHECK(!client_state_->order_data()->IsProcessingOrderNumber())
> inside SyncPointClient::WaitOutOfOrder is overly strict, and I think breaks
> for OnSignalSyncToken), but the comments are misleading I think.
> 

You are right that the rules I have set are overly strict, but I think that is
okay because it makes it easier to reason about. Anywhere where Wait() and
WaitOutOfOrder() can technically both be used should just use Wait(). It
wouldn't be a bug in that case to use WaitOutOfOrder() but the fact that it
catches other invalid use cases is worth it. So in this way, any
Wait()/WaitOutOfOrder() call there is only 1 correct one to use and it is hard
to mess up.

Which to use properly depends entirely on how order numbers are generated. The
GpuCommandBufferStub implementation is the only one where we number every single
message, that is not true for the InProcessComandBuffer or any of the mojo
command buffers which all generate order numbers for each command buffer flush.

[piman, 2016-01-07 22:39:07]

On 2016/01/07 18:47:58, David Yen wrote:
> On 2016/01/07 01:25:59, piman (Slow to review) wrote:
> > Actually, thinking a bit more, the difference between SignalSyncToken and
> > WaitSyncToken is that the former doesn't block execution, and so can't
> > cause deadlocks on the GPU process. This has a few consequences:
> > - it is somewhat safe to wait for an unverified/future sync point (the
> > callback may never fire, the client needs to be able to handle that)
> > - it *can* be done outside the message processing
> > (e.g. GpuChannelManager::OnDestroyGpuMemoryBuffer), but doesn't have to
> > (e.g. GpuCommandBufferStub::OnSignalSyncToken)
> > - it *could* be done from other threads (though SyncPointClient isn't
> > thread-safe).
> > 
> > So, I think the code is close to correct (except the
> > DCHECK(!client_state_->order_data()->IsProcessingOrderNumber())
> > inside SyncPointClient::WaitOutOfOrder is overly strict, and I think breaks
> > for OnSignalSyncToken), but the comments are misleading I think.
> > 
> 
> You are right that the rules I have set are overly strict, but I think that is
> okay because it makes it easier to reason about. Anywhere where Wait() and
> WaitOutOfOrder() can technically both be used should just use Wait(). It
> wouldn't be a bug in that case to use WaitOutOfOrder() but the fact that it
> catches other invalid use cases is worth it. So in this way, any
> Wait()/WaitOutOfOrder() call there is only 1 correct one to use and it is hard
> to mess up.
> 
> Which to use properly depends entirely on how order numbers are generated. The
> GpuCommandBufferStub implementation is the only one where we number every
single
> message, that is not true for the InProcessComandBuffer or any of the mojo
> command buffers which all generate order numbers for each command buffer
flush.


There is one extra difference between Wait() and WaitOutOfOrder(), which is that
WaitOutOfOrder will wait on a future/not-validated SyncToken (which is both a
feature and a deadlock risk). No client of SignalSyncPoint/SignalSyncToken
relies on this currently, but I think it would be desirable to have all
implementations to be consistent there.

[David Yen, 2016-01-07 22:48:45]

On 2016/01/07 22:39:07, piman (Slow to review) wrote:
> On 2016/01/07 18:47:58, David Yen wrote:
> > On 2016/01/07 01:25:59, piman (Slow to review) wrote:
> > > Actually, thinking a bit more, the difference between SignalSyncToken and
> > > WaitSyncToken is that the former doesn't block execution, and so can't
> > > cause deadlocks on the GPU process. This has a few consequences:
> > > - it is somewhat safe to wait for an unverified/future sync point (the
> > > callback may never fire, the client needs to be able to handle that)
> > > - it *can* be done outside the message processing
> > > (e.g. GpuChannelManager::OnDestroyGpuMemoryBuffer), but doesn't have to
> > > (e.g. GpuCommandBufferStub::OnSignalSyncToken)
> > > - it *could* be done from other threads (though SyncPointClient isn't
> > > thread-safe).
> > > 
> > > So, I think the code is close to correct (except the
> > > DCHECK(!client_state_->order_data()->IsProcessingOrderNumber())
> > > inside SyncPointClient::WaitOutOfOrder is overly strict, and I think
breaks
> > > for OnSignalSyncToken), but the comments are misleading I think.
> > > 
> > 
> > You are right that the rules I have set are overly strict, but I think that
is
> > okay because it makes it easier to reason about. Anywhere where Wait() and
> > WaitOutOfOrder() can technically both be used should just use Wait(). It
> > wouldn't be a bug in that case to use WaitOutOfOrder() but the fact that it
> > catches other invalid use cases is worth it. So in this way, any
> > Wait()/WaitOutOfOrder() call there is only 1 correct one to use and it is
hard
> > to mess up.
> > 
> > Which to use properly depends entirely on how order numbers are generated.
The
> > GpuCommandBufferStub implementation is the only one where we number every
> single
> > message, that is not true for the InProcessComandBuffer or any of the mojo
> > command buffers which all generate order numbers for each command buffer
> flush.
> 
> 
> There is one extra difference between Wait() and WaitOutOfOrder(), which is
that
> WaitOutOfOrder will wait on a future/not-validated SyncToken (which is both a
> feature and a deadlock risk). No client of SignalSyncPoint/SignalSyncToken
> relies on this currently, but I think it would be desirable to have all
> implementations to be consistent there.

Hmm, it was not my intention to support future sync tokens which are not
validated, that would require some extra thinking to be sure no deadlocks could
occur. Looking at the function, although I am passing UINT32_MAX as the "wait
order number", it should be caught by this logic within
"ValidateReleaseOrderNumber()" (line 164):

  // So far it could be valid, but add an order fence guard to be sure it
  // gets released eventually.
  const uint32_t expected_order_num =
      std::min(unprocessed_order_num_, wait_order_num);
  order_fence_queue_.push(
      OrderFence(expected_order_num, fence_release, client_state));

So if a WaitOutOfOrder() is called, it is guaranteed to be released by the time
the current unprocessed order number is reached. So it can't wait on a future
sync tokens still and must be validated to ensure the sync token has been in
fact queued.

[piman, 2016-01-07 22:58:12]

On 2016/01/07 22:48:45, David Yen wrote:
> On 2016/01/07 22:39:07, piman (Slow to review) wrote:
> > On 2016/01/07 18:47:58, David Yen wrote:
> > > On 2016/01/07 01:25:59, piman (Slow to review) wrote:
> > > > Actually, thinking a bit more, the difference between SignalSyncToken
and
> > > > WaitSyncToken is that the former doesn't block execution, and so can't
> > > > cause deadlocks on the GPU process. This has a few consequences:
> > > > - it is somewhat safe to wait for an unverified/future sync point (the
> > > > callback may never fire, the client needs to be able to handle that)
> > > > - it *can* be done outside the message processing
> > > > (e.g. GpuChannelManager::OnDestroyGpuMemoryBuffer), but doesn't have to
> > > > (e.g. GpuCommandBufferStub::OnSignalSyncToken)
> > > > - it *could* be done from other threads (though SyncPointClient isn't
> > > > thread-safe).
> > > > 
> > > > So, I think the code is close to correct (except the
> > > > DCHECK(!client_state_->order_data()->IsProcessingOrderNumber())
> > > > inside SyncPointClient::WaitOutOfOrder is overly strict, and I think
> breaks
> > > > for OnSignalSyncToken), but the comments are misleading I think.
> > > > 
> > > 
> > > You are right that the rules I have set are overly strict, but I think
that
> is
> > > okay because it makes it easier to reason about. Anywhere where Wait() and
> > > WaitOutOfOrder() can technically both be used should just use Wait(). It
> > > wouldn't be a bug in that case to use WaitOutOfOrder() but the fact that
it
> > > catches other invalid use cases is worth it. So in this way, any
> > > Wait()/WaitOutOfOrder() call there is only 1 correct one to use and it is
> hard
> > > to mess up.
> > > 
> > > Which to use properly depends entirely on how order numbers are generated.
> The
> > > GpuCommandBufferStub implementation is the only one where we number every
> > single
> > > message, that is not true for the InProcessComandBuffer or any of the mojo
> > > command buffers which all generate order numbers for each command buffer
> > flush.
> > 
> > 
> > There is one extra difference between Wait() and WaitOutOfOrder(), which is
> that
> > WaitOutOfOrder will wait on a future/not-validated SyncToken (which is both
a
> > feature and a deadlock risk). No client of SignalSyncPoint/SignalSyncToken
> > relies on this currently, but I think it would be desirable to have all
> > implementations to be consistent there.
> 
> Hmm, it was not my intention to support future sync tokens which are not
> validated, that would require some extra thinking to be sure no deadlocks
could
> occur. Looking at the function, although I am passing UINT32_MAX as the "wait
> order number", it should be caught by this logic within
> "ValidateReleaseOrderNumber()" (line 164):
> 
>   // So far it could be valid, but add an order fence guard to be sure it
>   // gets released eventually.
>   const uint32_t expected_order_num =
>       std::min(unprocessed_order_num_, wait_order_num);
>   order_fence_queue_.push(
>       OrderFence(expected_order_num, fence_release, client_state));
> 
> So if a WaitOutOfOrder() is called, it is guaranteed to be released by the
time
> the current unprocessed order number is reached. So it can't wait on a future
> sync tokens still and must be validated to ensure the sync token has been in
> fact queued.

Ok, LGTM. For some reason that logic always confuses me.

[sky, 2016-01-08 16:21:51]

LGTM

[David Yen, 2016-01-08 21:31:03]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2016-01-08 21:31:04]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1568563002/#ps80001
(title: "Allow WaitOutOfOrder if no client order data")

[commit-bot: I haz the power, 2016-01-08 21:33:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568563002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568563002/80001

[commit-bot: I haz the power, 2016-01-08 21:58:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-08 21:59:22]

Description was changed from

==========
Added a way for sync point clients to issue out of order waits.

This patch fixes a subtle bug that could occur if a sync point client
is waiting without a corresponding order number. This can occur for any
wait that is not done within the usual command buffer processing loop.
An example is the command buffer stub OnSignalSyncToken() function
which issues a wait directly on the IO thread.

Previously, a similar concept was introduced in SyncPointClientWaiter
which was used to wait out of order when a Gpu Memory Buffer was
destroyed. Because of the need for this functionality in the regular
SyncPointClient, I have folded the SyncPointClientWaiter functions into
SyncPointClient under WaitOutOfOrder() & WaitOutOfOrderNonThreadSafe().

Because of how subtle this bug is, further state tracking is now done
so we can test whether or not an order number is processing or not.
Using that test, DCHECKs have been added so Wait()/WaitNonThreadSafe
can only be called when processing an order number, and the the out of
order variants can only be called when not processing an order number.

BUG=514815
TEST=Added unit tests to test SignalSyncToken().
==========

to

==========
Added a way for sync point clients to issue out of order waits.

This patch fixes a subtle bug that could occur if a sync point client
is waiting without a corresponding order number. This can occur for any
wait that is not done within the usual command buffer processing loop.
An example is the command buffer stub OnSignalSyncToken() function
which issues a wait directly on the IO thread.

Previously, a similar concept was introduced in SyncPointClientWaiter
which was used to wait out of order when a Gpu Memory Buffer was
destroyed. Because of the need for this functionality in the regular
SyncPointClient, I have folded the SyncPointClientWaiter functions into
SyncPointClient under WaitOutOfOrder() & WaitOutOfOrderNonThreadSafe().

Because of how subtle this bug is, further state tracking is now done
so we can test whether or not an order number is processing or not.
Using that test, DCHECKs have been added so Wait()/WaitNonThreadSafe
can only be called when processing an order number, and the the out of
order variants can only be called when not processing an order number.

BUG=514815
TEST=Added unit tests to test SignalSyncToken().

Committed: https://crrev.com/563fb212fda4ad5ea477b754d628527e3f20010f
Cr-Commit-Position: refs/heads/master@{#368441}
==========

[commit-bot: I haz the power, 2016-01-08 21:59:23]

Patchset 5 (id:??) landed as
https://crrev.com/563fb212fda4ad5ea477b754d628527e3f20010f
Cr-Commit-Position: refs/heads/master@{#368441}