mctpd: Make timeout and bus owner configs configurable

khangng-ampere commented 1 year ago

Currently, the timeout and bus owner configs is statically defined inside mctpd.c. This PR makes those configs configurable:

The timeout can be configured via a program argument, so that people can tweak it at runtime if necessary.
~~The bus owner config can be configured via meson_options.txt, because this seems to be a build time / one time decision~~. The bus owner config is changed to be configured via a program argument for easier use.

jk-ozlabs commented 1 year ago

I would prefer we do this as a command-line argument (or through run-time configuration, but that can come later). A compile-time flag makes testing more complex...

just to clarify - this was on the bus-owner configuration commit!

khangng-ampere commented 1 year ago

I updated the PR. Not sure what to call the flag, -t is already taken by the timeout. I picked -r because the word "role" did appear in the DSP0236... but only once.

jk-ozlabs commented 8 months ago

I've been thinking about a configuration file for a while, which would allow adjustment to the timeout and bus-owner states, and allow for future flexibility (eg, for the EID pools) too.

I have a proposed change at https://github.com/CodeConstruct/mctp/commit/4b40af155a4edc10ca6c540ac94888992cdb0eda

Would this work for you?

Note that this uses the tomlc99 parser as a git subtree, so the git history might be a bit convoluted! I'm open to other approaches if necessary.

khangng-ampere commented 8 months ago

I've been thinking about a configuration file for a while, which would allow adjustment to the timeout and bus-owner states, and allow for future flexibility (eg, for the EID pools) too.

I have a proposed change at 4b40af1

Would this work for you?

Note that this uses the tomlc99 parser as a git subtree, so the git history might be a bit convoluted! I'm open to other approaches if necessary.

I just want to be able to tweak the configuration at runtime, so anything works for me :D Putting them all in a file looks nicer IMO.

My only tiny concern is why specifically TOML? Since my main interest is the OpenBMC project, there doesn't seem to be any TOML parsing C libs recipe yet AFAIK. I am more in favor of INI, despite the criticisms, because it is used by systemd-network configuration files and libinih is already a recipe at https://github.com/openbmc/openbmc/blob/master/meta-openembedded/meta-oe/recipes-support/inih/libinih_57.bb.

I have seen meson subprojects be used to deal with dependencies. For example, https://github.com/openbmc/pldm/tree/master/subprojects.

Since the config files support all the configs in this MR, I'm going to close this.

khangng-ampere commented 8 months ago

Ah oops, the config file branch isn't merged yet so I will reopen this for discussion.

jk-ozlabs commented 8 months ago

I just want to be able to tweak the configuration at runtime, so anything works for me :D Putting them all in a file looks nicer IMO.

OK, great!

My only tiny concern is why specifically TOML?

My reasoning behind the toml approach is that we'll likely need something more structured, in able to describe multiple networks and/or EID pools. We could still do that with ini, but requires namespacing the keys/sections, which may get messy.

Since my main interest is the OpenBMC project, there doesn't seem to be any TOML parsing C libs recipe yet AFAIK.

We can get around that - currently by including the toml library directly (see https://github.com/CodeConstruct/mctp/commit/0501df8b64e42529f98fe3225a50384e37898982) . No new deps are required for this build currently. This subtree approach is a bit heavy-handed though; we have a few other options:

including just the toml.c / toml.h files in the repo directly; that's all we need for the parsing
adding a meson infrastructure , and/or a meta-oe recipe

I'd suggest a semi staged approach:

include the toml.c / toml.h sources directly for now, which gives an immediate build
submitting an appropriate meson build to the tomlc99 project
submitting a meta-oe recipe for tomlc99
once (2) and (3) are done, switch from including toml.c / toml.h to a meson wrap

I have seen meson subprojects be used to deal with dependencies. For example, https://github.com/openbmc/pldm/tree/master/subprojects.

That won't help with the obmc build case though, the wraps are disabled there.

khangng-ampere commented 8 months ago

I just want to be able to tweak the configuration at runtime, so anything works for me :D Putting them all in a file looks nicer IMO.

OK, great!

My only tiny concern is why specifically TOML?

My reasoning behind the toml approach is that we'll likely need something more structured, in able to describe multiple networks and/or EID pools. We could still do that with ini, but requires namespacing the keys/sections, which may get messy.

Since my main interest is the OpenBMC project, there doesn't seem to be any TOML parsing C libs recipe yet AFAIK.

We can get around that - currently by including the toml library directly (see 0501df8) . No new deps are required for this build currently. This subtree approach is a bit heavy-handed though; we have a few other options:
* including just the toml.c / toml.h files in the repo directly; that's all we need for the parsing

* adding a meson infrastructure , and/or a meta-oe recipe
I'd suggest a semi staged approach:
1. include the toml.c / toml.h sources directly for now, which gives an immediate build

2. submitting an appropriate meson build to the tomlc99 project

3. submitting a meta-oe recipe for tomlc99

4. once (2) and (3) are done, switch from including toml.c / toml.h to a meson wrap
I have seen meson subprojects be used to deal with dependencies. For example, https://github.com/openbmc/pldm/tree/master/subprojects.

That won't help with the obmc build case though, the wraps are disabled there.

Sounds good to me. I did not think through the process to stabilize the dependency at all, now it makes sense.

I have one question regarding the config: should the config be specific for each MCTP links? Currently, we can support TMBOs and EPs, which have the same mode for all MCTP links, but bridges/normal BOs require different modes. Each links could have separate timing parameters, MTUs, networks, EID pools, ...

Granted that the configuration schema/syntax is open to bikeshedding and can contain some way to do default/override options, but after parsing, the options are now stored in mctp_nl.linkmap_entry, instead of global ctx. Is this reasonable?

jk-ozlabs commented 8 months ago

Hi Khang,

I have one question regarding the config: should the config be specific for each MCTP links?

Some of it could be - we'd still want global defaults for the simple cases, but we can later make this override-able at different levels. Links would be one of those levels, but aren't appropriate for everything:

Currently, we can support TMBOs and EPs, which have the same mode for all MCTP links, but bridges/normal BOs require different modes. Each links could have separate timing parameters, MTUs, networks, EID pools, ...

Currently, mctpd doesn't assume control of the local network state (the network assignments and link-level MTU). The EID pools could be either per-network or per-link; or per-network with static allocations that are link-specific.

So, there's a bunch of design decisions behind that. Might be good to get some use-case data before we start making those though.

(maybe we should arrange a chat about your endpoint usage some time?)

Also, I have updated the conf branch for the plan above:

https://github.com/CodeConstruct/mctp/commits/dev/conf

and submitted a PR for meson support in the toml parser, so we can later do a wrap:

https://github.com/cktan/tomlc99/pull/90

pointbazaar commented 4 months ago

i have a need for the configurable timeout in emulation use-case.

If there is interest to support it, refer to https://github.com/openbmc/openbmc/tree/master/meta-evb/meta-evb-arm/meta-evb-fvp-base

There is a need for a large timeout since both fvp may be running at different speeds and also one or both may be running faster/slower than realtime, depending on configuration options. In this case, i need to set a large timeout of multiple seconds.

I do not need the capability to set the timeout for different links, it can be the same for all links (i only have one link).

Is this PR still moving forward or should a new PR be created? Can we agree to support a global option for all links and then perhaps later create per-link options to override?

jk-ozlabs commented 4 months ago

Hi @pointbazaar

Thanks for the input - good to know about the use case you have!

Is this PR still moving forward or should a new PR be created? Can we agree to support a global option for all links and then perhaps later create per-link options to override?

Yep, we'll still go with this one, and extend to per-link config in future.

CodeConstruct / mctp

mctpd: Make timeout and bus owner configs configurable #14