[ovs-dev] [PATCH] netflow: Send multiple records for byte counts > UINT32_MAX

Justin Pettit jpettit at nicira.com
Wed Sep 1 07:19:48 UTC 2010


On Aug 31, 2010, at 11:24 PM, Jesse Gross wrote:

> On Tue, Aug 31, 2010 at 10:00 PM, Justin Pettit <jpettit at nicira.com> wrote:
>> +    n_recs = ((expired->byte_count - nf_flow->byte_count_off)
>> +               + UINT32_MAX + 1) / UINT32_MAX;
> 
> I would probably just do the division and then add 1 at the end.  It
> seems clearer to not have the extra UINT32_MAX in there.  Do you even
> need the + 1 here as currently written?

Whoops, that was supposed to be "- 1" instead of "+ 1".  I'm trying to achieve a "round-up" division.  In particular, I'm trying to handle the problem of the number of entries being exactly on the threshold.  For example:

	(5 + 5 - 1) / 5 = 3

With "-" does it look reasonable?

>> +    avg_pkt_count = (expired->packet_count - nf_flow->packet_count_off)
>> +                    / n_recs;
>> +    avg_byte_count = (expired->byte_count - nf_flow->byte_count_off)
>> +                     / n_recs;
> 
> Since this is integer math, avg_XXX_count * n_recs will likely be less
> than (expired->XXX_count - nf_flow->XXX_count_off).  The sum of all of
> these rounding errors will be made up in the last packet.  However,
> what happens if that last packet is already close to the 32 bit limit?
> 
> Imagine this case (all integer math):
> expired->byte_count - nf_flow->packet_count_off = 21474836472
> n_recs = 5
> avg_pkt_count = 4294967294
> result of subtraction for last packet: 4294967296 = 0 in 32 bit

Good point.  I've reworked the patch so that it maintains a running average, so there's no accumulated cruft right at the end.

>> +    while (n_recs--) {
>> +        uint32_t pkt_count = n_recs ? avg_pkt_count :
>> +                    expired->packet_count - nf_flow->packet_count_off;
>> +        uint32_t byte_count = n_recs ? avg_byte_count :
>> +                    expired->byte_count - nf_flow->byte_count_off;
>> +
>> +        gen_netflow_rec(nf, nf_flow, expired, pkt_count, byte_count);
>> +
>> +        nf_flow->packet_count_off += pkt_count;
>> +        nf_flow->byte_count_off += byte_count;
>> +    }
> 
> This loop is trying to be a little too clever for me.  I would just
> write it as a for loop, drop the ternary ifs, use a couple extra
> temporary variables, etc  You also compute the difference between the
> expiration record and the offset 3 different times in this function, I
> would put it in a temporary variable to start with and subtract off of
> that here.


The revised patch should be a bit cleaner.  I'll send it out in a bit.

Thanks for the review!

--Justin






More information about the dev mailing list