Re: Possible loss of samples

[John L. <le...@mo...>]

On Thu, Jun 13, 2002 at 01:54:36PM -0400, William Cohen wrote:

> I was looking through the code for op_do_profile() in 
> oprofile/module/oprofile.c.  I noticed the following loop:
> 
> 	for (i=0; i < OP_NR_ENTRY; i++) {
> 		if (likely(!op_miss(samples[i]))) {
> 			samples[i].count++;
> 			return;
> 		}
> 
> The count field is used to hold both the register and the count 
> associated with that register.  The count field is 16 bits in size with 
> the upper 2 bits (OP_BITS) used to store which counter is being used and 
> the remaining low order bits used to store the number of samples. An 
> overflow will corrupt the counter information stored in this field. This 
> may not be much of a problem with things getting flushed to the buffer, 
> but the potential is there.  Thus, this could could loose (1U << 
> OP_BITS_COUNT) samples.  Shouldn't the code flush the data to the buffer 
> and start a new count, something like the attached patch?

This already happens :

#define op_miss(ops)  \
        ((ops).eip != regs->eip || \
        (ops).pid != current->pid || \
        op_full_count((ops).count))

However you've made it obvious that this feature of op_miss() is
somewhat obscure.

		if (!op_miss(samples[i]) && !op_full_count(samples[i].count))

makes it more obvious.

> When processors with more counter registers such as the Pentium 4 get 
> added (~16 register, 4 bits), the chance of this corrupting the 
> measurements increases.

I wonder what difference this extra check makes. it's probably neglible,
but the chances of this case happening are minimal, even with the new
hashing, and the pentium IV. And we probably want to support all 18
(i.e. 5 bits) for multiplexing purposes anyway, depending on what
happens with PEBS support.

At the same time, if this case /does/ happen, then the data loss would
be significant if we removed op_full_count().

regards
john

-- 
"All is change; all yields its place and goes"
	- Euripides