[RFC,01/14] anv/tests: Fix block_pool_no_free test.

Submitted by Rafael Antognolli on Dec. 8, 2018, 12:05 a.m.

Details

Message ID 20181208000553.29501-2-rafael.antognolli@intel.com
State New
Headers show
Series "Do not use userptr in anv if softpin is available." ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Rafael Antognolli Dec. 8, 2018, 12:05 a.m.
The test was checking whether -1 was smaller than an unsigned int, which
is always false. So it was exiting early and never running until the
end, since it would reach the condition (thread_max == -1).

However, just fixing that is not enough. The test is currently getting
the highest block on each iteration, and then on the next one, until it
reaches the end. But by that point, we wouldn't have looked at all
blocks of all threads. For instance, for 3 threads and 4 blocks per
thread, a situation like this (unlikely to happen):

[Thread]: [Blocks]
   [0]: [0, 32, 64, 96]
   [1]: [128, 160, 192, 224]
   [2]: [256, 288, 320, 352]

Would cause the test to iterate only over the thread number 2.

The fix is to always grab the lowest block on each iteration, and assert
that it is higher than the one from the previous iteration.
---
 src/intel/vulkan/tests/block_pool_no_free.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/vulkan/tests/block_pool_no_free.c b/src/intel/vulkan/tests/block_pool_no_free.c
index 17006dd3bc7..730297d4e36 100644
--- a/src/intel/vulkan/tests/block_pool_no_free.c
+++ b/src/intel/vulkan/tests/block_pool_no_free.c
@@ -78,16 +78,16 @@  static void validate_monotonic(uint32_t **blocks)
    unsigned next[NUM_THREADS];
    memset(next, 0, sizeof(next));
 
-   int highest = -1;
+   uint32_t lowest = UINT32_MAX;
    while (true) {
-      /* First, we find which thread has the highest next element */
-      int thread_max = -1;
+      /* First, we find which thread has the lowest next element */
+      uint32_t thread_max = UINT32_MAX;
       int max_thread_idx = -1;
       for (unsigned i = 0; i < NUM_THREADS; i++) {
          if (next[i] >= BLOCKS_PER_THREAD)
             continue;
 
-         if (thread_max < blocks[i][next[i]]) {
+         if (thread_max > blocks[i][next[i]]) {
             thread_max = blocks[i][next[i]];
             max_thread_idx = i;
          }
@@ -96,13 +96,14 @@  static void validate_monotonic(uint32_t **blocks)
       /* The only way this can happen is if all of the next[] values are at
        * BLOCKS_PER_THREAD, in which case, we're done.
        */
-      if (thread_max == -1)
+      if (thread_max == UINT32_MAX)
          break;
 
-      /* That next element had better be higher than the previous highest */
-      assert(blocks[max_thread_idx][next[max_thread_idx]] > highest);
+      /* That next element had better be higher than the previous lowest */
+      assert(lowest == UINT32_MAX ||
+             blocks[max_thread_idx][next[max_thread_idx]] > lowest);
 
-      highest = blocks[max_thread_idx][next[max_thread_idx]];
+      lowest = blocks[max_thread_idx][next[max_thread_idx]];
       next[max_thread_idx]++;
    }
 }

Comments

On Fri, Dec 7, 2018 at 6:06 PM Rafael Antognolli <
rafael.antognolli@intel.com> wrote:

> The test was checking whether -1 was smaller than an unsigned int, which
> is always false. So it was exiting early and never running until the
> end, since it would reach the condition (thread_max == -1).
>
> However, just fixing that is not enough. The test is currently getting
> the highest block on each iteration, and then on the next one, until it
> reaches the end. But by that point, we wouldn't have looked at all
> blocks of all threads. For instance, for 3 threads and 4 blocks per
> thread, a situation like this (unlikely to happen):
>
> [Thread]: [Blocks]
>    [0]: [0, 32, 64, 96]
>    [1]: [128, 160, 192, 224]
>    [2]: [256, 288, 320, 352]
>
> Would cause the test to iterate only over the thread number 2.
>
> The fix is to always grab the lowest block on each iteration, and assert
> that it is higher than the one from the previous iteration.
> ---
>  src/intel/vulkan/tests/block_pool_no_free.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/src/intel/vulkan/tests/block_pool_no_free.c
> b/src/intel/vulkan/tests/block_pool_no_free.c
> index 17006dd3bc7..730297d4e36 100644
> --- a/src/intel/vulkan/tests/block_pool_no_free.c
> +++ b/src/intel/vulkan/tests/block_pool_no_free.c
> @@ -78,16 +78,16 @@ static void validate_monotonic(uint32_t **blocks)
>     unsigned next[NUM_THREADS];
>     memset(next, 0, sizeof(next));
>
> -   int highest = -1;
> +   uint32_t lowest = UINT32_MAX;
>

I think this should still be named "highest" since it is the highest thing
we've seen.  Also, I kind-of think -1 still makes sense as a starting
value.  Maybe we just need to make the comparison below explicitly signed
with a cast.


>     while (true) {
> -      /* First, we find which thread has the highest next element */
> -      int thread_max = -1;
> +      /* First, we find which thread has the lowest next element */
> +      uint32_t thread_max = UINT32_MAX;
>

Yes, this loop needs to change.  However, I think we should rename the
variables to thread_min and min_thread_idx.


>        int max_thread_idx = -1;
>        for (unsigned i = 0; i < NUM_THREADS; i++) {
>           if (next[i] >= BLOCKS_PER_THREAD)
>              continue;
>
> -         if (thread_max < blocks[i][next[i]]) {
> +         if (thread_max > blocks[i][next[i]]) {
>              thread_max = blocks[i][next[i]];
>              max_thread_idx = i;
>           }
> @@ -96,13 +96,14 @@ static void validate_monotonic(uint32_t **blocks)
>        /* The only way this can happen is if all of the next[] values are
> at
>         * BLOCKS_PER_THREAD, in which case, we're done.
>         */
> -      if (thread_max == -1)
> +      if (thread_max == UINT32_MAX)
>           break;
>
> -      /* That next element had better be higher than the previous highest
> */
> -      assert(blocks[max_thread_idx][next[max_thread_idx]] > highest);
> +      /* That next element had better be higher than the previous lowest
> */
> +      assert(lowest == UINT32_MAX ||
> +             blocks[max_thread_idx][next[max_thread_idx]] > lowest);
>
> -      highest = blocks[max_thread_idx][next[max_thread_idx]];
> +      lowest = blocks[max_thread_idx][next[max_thread_idx]];
>        next[max_thread_idx]++;
>     }
>  }
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>