Page MenuHomeFreeBSD

improve performance of ipmi kcs interface.
ClosedPublic

Authored by lprylli_netflix.com on Jun 5 2019, 6:33 PM.

Details

Summary

Current code is waiting 100us for all transitions (roughly between
each byte either sent or received).
Most transitions actually completes in 2-3 microseconds.

By starting polling the status register with a delay of 4 us with
exponential backoff, the performance of most ipmi operations is
significantly improved:

  • bmc update on supermicro x9 to x11 from ~1hour to ~6-8 minutes.
  • ipmitool sensor list improves by a factor of 4.

No significant improvements on modern server seen by using a
lower delay (any attempt at optimizing further might be
better done by implementing bt channel between host<->bmc)
rather than trying extra tuning of kcs interface.

The changes should also generally reduce the total amount of cpu
or io bandwith used for a given ipmi operations.

The fact that Linux used to use 5us delay and now used a busy loop
with yield() adds confidence that lowering delay should not trigger
any hw bug anywhere.

Test Plan

on X9 and X11 motherboards, with:

  • ipmitool sensor list
  • supermicro/sum -c UpdateBmc --overwrite_cfg --overwrite_sdr --file ...
  • also used an ad-hoc code to get histogram of timings on various servers.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jtl added reviewers: imp, jhb.Jun 5 2019, 7:56 PM
jtl accepted this revision.
This revision is now accepted and ready to land.Jun 5 2019, 7:57 PM

Remove spurious comment.

This revision now requires review to proceed.Jun 5 2019, 11:31 PM
jhb added a comment.Jun 6 2019, 12:02 AM

The change looks good to me. The original version was indeed not trying to be clever at all.

A few style nits might be to say "I/O bus used for KCS" though "used for KCS" is arguably redundant. Maybe make the first new comment be a full sentence as well:

"Waits are 2-3 usecs on typical systems."

sys/dev/ipmi/ipmi_kcs.c
66 ↗(On Diff #58289)

For the last expression, can we just use '(status & mask) != state'? Maybe if we alter the wrapper functions to convert the 'state' into the actual flag value. Something like:

static int
kcs_wait_for_ibf(struct ipmi_softc *sc, int state)
{

        return (kcs_wait(sc, state ? KCS_STATUS_IBF : 0, KCS_STATUS_IBF));
}

Also, this is amusing: kcs_wait_for_ibf() is only used when state is 0, and kcs_wait_for_obf() is only used when state is 1, so a fair bit of this code was dead before. (Maybe sad more than amusing)

jhb accepted this revision.Jun 6 2019, 12:03 AM
This revision is now accepted and ready to land.Jun 6 2019, 12:03 AM
imp accepted this revision.Jun 6 2019, 5:46 PM

Minor mostly cosmetic improvements.

  • comment rephrasing.
  • make kcs_wait take a value/mask
  • make kcs_wait_for_{ibf,obf} second arg explicitely a bool to remove any ambiguity about usage.
This revision now requires review to proceed.Jun 6 2019, 5:58 PM
lprylli_netflix.com marked an inline comment as done.Jun 6 2019, 6:04 PM
jhb accepted this revision.Jun 6 2019, 6:13 PM

Since the callers are still passing 0/1 for 'level', I would maybe still have it as an int to keep the diff small so you don't have to change any of the calls to kcs_wait_for_[io]bf(). That's really minor though.

This revision is now accepted and ready to land.Jun 6 2019, 6:13 PM

Thanks for review.

With kcs_wait taking a int "value" representing a bit-set and kcs_wait_for_[ibf] taking a one-bit arg, I thought using "bool" for the latter gave an extra visual aid documenting it as taking a one-bit value to differentiate it from the former (whereas in the context of referring to a register bit, using either true/false or 0/1 in the callers seemed equivalent enough).
Let me know if you have a recommendation: if bool with "0|1" is to be avoided, I am tempting to extend the diff by removing the second argument altogether in "kcs_wait_for_[io]bf" (since as you observed in the code, it would never makes sense to wait for IBF to be 1 or OBF to be 0 in kcs host driver) to make the code even more obvious.

This revision was automatically updated to reflect the committed changes.