Page MenuHomeFreeBSD

Prevent overflow issues in timeout processing
ClosedPublic

Authored by smh on Nov 13 2014, 10:36 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 18, 2:47 PM
Unknown Object (File)
Mon, Mar 18, 2:47 PM
Unknown Object (File)
Mon, Mar 18, 2:47 PM
Unknown Object (File)
Mon, Mar 18, 2:47 PM
Unknown Object (File)
Mon, Mar 18, 2:47 PM
Unknown Object (File)
Feb 16 2024, 8:11 AM
Unknown Object (File)
Feb 16 2024, 8:11 AM
Unknown Object (File)
Feb 16 2024, 8:11 AM
Subscribers
None

Details

Summary

Previously, any timeout value for which (timeout * hz) will overflow the signed integer, will give weird results, since callout(9) routines will convert negative values of ticks to '1'. For unsigned integer overflow we will get sufficiently smaller timeout values than expected.

Switch from callout_reset, which requires conversion to int based ticks to callout_reset_sbt to avoid this.

This was based on the original work done by Eygene Ryabinkin <rea@freebsd.org> back in 5 Aug 2011 which used a macro to help avoid the overlow.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

smh retitled this revision from to Properly convert msec timeouts to ticks.
smh updated this object.
smh edited the test plan for this revision. (Show Details)
smh added reviewers: mav, davide.
smh updated this object.
mav requested changes to this revision.Nov 13 2014, 10:57 PM
mav edited edge metadata.

I don't like this change.

  1. I think you are going wrong way converting milliseconds to ticks. callout_reset() you are using is indeed a wrapper around callout_reset_sbt(), so you are adding one more conversion on top, that consumes time and increases chances of overflow or precision loss. You should use callout_reset_sbt() directly and avoid any dependency from HZ.
  1. I am not sure we need one more function in callout API. It is already overbloated, and I would not continue that. I don't seem big problem in using callout_reset_sbt(c, SBT_1MS * timeout, 0, ...) directly.
This revision now requires changes to proceed.Nov 13 2014, 10:57 PM
smh edited edge metadata.

Switch from callout_reset to callout_reset_sbt as suggested by mav.

This required the asr and iir driver to be updated to callouts from timeouts and the mpt driver needed mpt_wait_req reworking so I'd appreciate feedback on those changes specifically.

In D1157#5, @mav wrote:

I don't like this change.

  1. I think you are going wrong way converting milliseconds to ticks. callout_reset() you are using is indeed a wrapper around callout_reset_sbt(), so you are adding one more conversion on top, that consumes time and increases chances of overflow or precision loss. You should use callout_reset_sbt() directly and avoid any dependency from HZ.
  1. I am not sure we need one more function in callout API. It is already overbloated, and I would not continue that. I don't seem big problem in using callout_reset_sbt(c, SBT_1MS * timeout, 0, ...) directly.

Thanks mav, seems callout_reset_sbt isn't documented in the man page so not a surprise this option was missed when Eygene originally created this patchset. Updated version based on your changes now uploaded.

That looks better, but:

  • You don't need C_HARDCLOCK do be passed everywhere. It was made to mimic old HZ-based behavior, but these cases have nothing to do with HZ at all.
  • jhb@ yesterday updated iir driver to use callout(9), and I guess he may already have a patch for asr.
smh edited edge metadata.

Remove C_HARDCLOCK from reset calls as suggested by mav.

smh retitled this revision from Properly convert msec timeouts to ticks to Prevent overflow issues in timeout processing.Nov 14 2014, 11:25 PM
smh updated this object.

One more bug: asr_ccb is a union, not a struct. That is why I think adding callout structure to it won't work.

The rest of the patch (except not applicable now iir) looks fine to me.

This looks good to me modulo mav's last round of comments.

davide edited edge metadata.
In D1157#16, @mav wrote:

One more bug: asr_ccb is a union, not a struct. That is why I think adding callout structure to it won't work.

The rest of the patch (except not applicable now iir) looks fine to me.

Yer I kept that in for now but I've pinged jhb to see if he has anything for asr before I look at that.

Thanks for the reviews guys.

smh edited edge metadata.

Revert previous changes to asr in favour of safer timeout to tick conversion as jhb is planing to axe asr anyway.

You OK with this latest revision mav?

mav edited edge metadata.
This revision is now accepted and ready to land.Nov 19 2014, 3:29 AM

Hmm, I have a few more pending timeout -> callout conversions I'd like to commit that you might then want to fix. Can you give me another day or so to commit those?

Sure no problem, give me a prod when your done :)

I'm done with merging anything that affects you. My only remaining patch is to kern_fail.c which you don't touch now.

smh updated this revision to Diff 2503.

Closed by commit rS274819 (authored by @smh).