[ovs-dev] [PATCH 2/3] datapath: Fix flow time used computation.

[Jesse Gross]

On Mon, Jan 31, 2011 at 1:42 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Sat, Jan 29, 2011 at 03:01:24PM -0800, Jesse Gross wrote:
>> The current reporting of flow last used time has two issues that
>> cause it to incorrectly report the system monotonic time when the
>> flow was last used.
>>
>> The first is that it simply converts the stored jiffies value to
>> milliseconds by scaling with a constant.  This does not work because
>> jiffies is not zero based and can wrap around on 32-bit platforms.
>>
>> The second is there is no guarantee that jiffies advances at the
>> same rate as the RTC based monotonic time that userspace uses.
>> A variety of factors can cause differences, including system suspend
>> and clock drift.  These are not too important for relatively short
>> time periods such as the duration of the flow (nor is the flow timing
>> precision of extreme importance).  However, when the time being
>> measured is the duration since system boot (assuming that the above
>> issues had been addressed) the difference can become significant.
>>
>> This addresses both issues by restoring behavior similar to the
>> previous method of computing the flow used time, though in a
>> slightly different form to reflect the needs of the Netlink code.
>>
>> Signed-off-by: Jesse Gross <jesse at nicira.com>
>
> This looks OK to me.
>
> GCC is pretty good at optimizing division by a constant.  Does using
> reciprocal_divide() really improve code generation?  If it does, then
> I'd appreciate at least a comment saying what's the divisor--I had to
> figure it out by looking up reciprocal_divide().

Hmm, it used to help a lot more: originally I had a 64-bit divide,
which required a library function on 32-bit processors and that didn't
pay attention to the constness.  Not surprisingly, the generated code
for that was complete garbage.  I switched to 32-bit operations and
reciprocal divide at the same time and I guess I never looked at the
code generation for the intermediate version.

The generated code actually is slightly better for the version here
but not enough to make it worthwhile: GCC at least figured out that it
can transform this into a multiplication.  I'm going to drop the first
patch in this series and send it out again.