[PATCH] profiling: fix streaming of TOPN counters

[Richard Biener]

On Thu, Feb 18, 2021 at 10:46 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 2/18/21 10:31 AM, Richard Biener wrote:
> > On Wed, Feb 17, 2021 at 2:16 PM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> On 2/17/21 9:36 AM, Martin Liška wrote:
> >>> I'll write it even more robust...
> >>
> >> This is more elegant approach I've just tested on the instrumented clang.
> >>
> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>
> >> Ready to be installed?
> >
> > Isn't it still too complicated?  We're asked to write N counters so why
> > don't we just write N counters?
>
> Well, we are asked to stream N counters. But each has a variable length,
> which makes it funny :)
>
>    Thus, you already do
> >
> >         for (struct gcov_kvp *node = (struct gcov_kvp *)(intptr_t)start;
> > -          node != NULL; node = node->next)
> > +          node != NULL; node = node->next, j++)
> >          {
> >            gcov_write_counter (node->value);
> >            gcov_write_counter (node->count);
> > +
> > +         /* Stop when we reach expected number of items.  */
> > +         if (j + 1 == list_sizes[i])
> > +           break;
> >          }
> >
> > why isn't this then only thing you need (just using pair_count as gathered
> > previously)?  And just count pair_count to zero as IV?  No need to
> > do the node != NULL check?
>
> We can't do that, because that will lead in a corrupted profile.
> We can have 2 problematic situations:
>
> 1) linked list is shorter, e.g.
>
> 10 2 10000 10
>
> Here 2 represents 2 tuples, but we stream only one {10000: 10}. That happens in the instrumented clang.
>
> 2) linked list is longer, e.g.
> 10 1 10000 5 222222 5
>
> Here 1 represents 1 tuple, be we stream 2 tuples {10000: 10} and {222222: 5}.

I guess the problem is that for whatever reason the stream includes

   unsigned disk_size = GCOV_TOPN_DISK_COUNTERS * counters + 2 * pair_total;
   gcov_write_tag_length (GCOV_TAG_FOR_COUNTER (t_ix),
                         GCOV_TAG_COUNTER_LENGTH (disk_size));

which has the sum of the list lengths, but ...

-      gcov_type pair_count = ci_ptr->values[GCOV_TOPN_MEM_COUNTERS * i + 1];
       gcov_write_counter (ci_ptr->values[GCOV_TOPN_MEM_COUNTERS * i]);
-      gcov_write_counter (pair_count);

we stream something like that again.  What are the two different counters?
You save one, but the other you take async again?

The disk format here is sub-optimal at least and optimizing the read side
which has locking in place for making the write-side racy doesn't look good.

> >
> > Note architectures with less nice memory ordering guarantees might
> > eventually see partially updated pointers and counters so I think
> > we at least want atomic_read ()s of the values with the weakest
> > consistency possible.  (but that can be done as followup if we agree on that)
>
> Can we really see a partially updated pointer?

Not on x86 for aligned data.  Note the same applies to the counter values.

We've seen allocating memory from libgcov is problematic, so this is why I'm
asking.

> Thanks,
> Martin
>
> >
> > Richard.
> >
> >> Thanks,
> >> Martin
>