improve performance of ipmi kcs interface.

[lprylli_netflix.com]

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.

[lprylli_netflix.com]

Remove spurious comment.

[jhb]

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)

[lprylli_netflix.com]

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.

[jhb]

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.

[lprylli_netflix.com]

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.