Page MenuHomeFreeBSD

improve performance of ipmi kcs interface.
ClosedPublic

Authored by lprylli_netflix.com on Jun 5 2019, 6:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 20 2024, 7:18 AM
Unknown Object (File)
Feb 12 2024, 4:12 PM
Unknown Object (File)
Feb 12 2024, 9:57 AM
Unknown Object (File)
Feb 12 2024, 9:57 AM
Unknown Object (File)
Feb 1 2024, 10:43 AM
Unknown Object (File)
Jan 27 2024, 11:13 PM
Unknown Object (File)
Jan 2 2024, 8:27 PM
Unknown Object (File)
Dec 25 2023, 6:07 AM
Subscribers
None

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

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 24727
Build 23488: arc lint + arc unit

Event Timeline

jtl added reviewers: imp, jhb.
This revision is now accepted and ready to land.Jun 5 2019, 7:57 PM
This revision now requires review to proceed.Jun 5 2019, 11:31 PM

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–74

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)

This revision is now accepted and ready to land.Jun 6 2019, 12:03 AM

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

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.