Differential D20527

improve performance of ipmi kcs interface.
ClosedPublic
Actions

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

Details

Reviewers

jtl
imp
jhb

Commits

rS348996: The current IPMI KCS code is waiting 100us for all transitions (roughly

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

lprylli_netflix.com created this revision.Jun 5 2019, 6:33 PM

Harbormaster completed remote builds in B24704: Diff 58273.Jun 5 2019, 6:33 PM

jtl accepted this revision.Jun 5 2019, 7:56 PM

jtl added reviewers: imp, jhb.

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

Harbormaster completed remote builds in B24711: Diff 58289.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 ↗	(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

Harbormaster completed remote builds in B24727: Diff 58320.Jun 6 2019, 5:58 PM

lprylli_netflix.com marked an inline comment as done.Jun 6 2019, 6:04 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.

Closed by commit rS348996: The current IPMI KCS code is waiting 100us for all transitions (roughly (authored by jtl). · Explain WhyJun 12 2019, 4:06 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

head/

sys/

dev/

ipmi/

ipmi_kcs.c

59 lines

Diff 58565

View Options

head/sys/dev/ipmi/ipmi_kcs.c

improve performance of ipmi kcs interface.ClosedPublicActions