Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple builds/versions of the same provider with different features enabled #6708

Closed
acgoldma opened this issue Apr 23, 2021 · 6 comments

Comments

@acgoldma
Copy link
Contributor

Open this for comment:

In the event you build a custom external provider with an example version of v1.12.0, but the admin that controls the fabric updates libfabric to v1.12.1, which has an internal copy of the same provider, but with different configure options, there is no way to force OFI to use the external version you are pointing to with FI_PROVIDER_PATH.

In the kernel world the compatibility (ABI/API) is the only requirement, then it will use an external module if it exists, and fallback to internal if not found/valid.

As I understand from emails with @shefty, there was an explicit reason why libfabric chooses the newest version regardless of location.

We each came up with a few possible solutions and reasons for them, but wanted community feedback before proposing a patch.

Possible Solution 1: New Provider Selection Algorithm Environment variable.

FI_PROV_SEL_ALGO=[default|version] = Current behavior (highest version, first come)
FI_PROV_SEL_ALGO=[first|path] = first parsed [path order] (external, internal)
Cons:

  • Limited in scope with little room for expansion
  • New ENV customers will have to know

Pros:

  • Minimal code change
  • Simple understanding

Possible Solution 2: Expand FI_PROVIDER to URI derived model "%" or "?" etc. to separate key value pairs

Example: FI_PROVIDER=tcp?version=1.12.1,udp?dl=disable,psm3?[dl=only][version=1.11.2]
OR FI_PROVIDER=tcp%version=1.12.1,udp%dl=disable,psm3%dl=only%version=1.11.2

TCP must use version 1.12.1
UDP uses internal version only
PSM3 must use DL version 1.11.2 only

Cons:

  • Possible more complex code changes required
  • More complex parsing
  • more complex validation
  • how do we handle repeated params, or complex situations like:
    FI_PROVIDER=tcp%version=1.12.1,tcp%version=1.12.0%dl=only
  • Would restrict names to not use special symbols that we define (add more validation)

Pros:

  • Possible expansion with new params and maybe new operators = -> > < ~ !=
    • First pass only really needs dl=only to minimally satisfy main issue.
  • No new ENV to maintain.

Possible Solution 3: Remove version checking from provider and require user to properly set FI_PROVIDER_PATH

Cons:

  • Changes default behavior
  • An older OFI provider in the path may be picked up when a newer one exist internal or later in the path

Pros:

  • To select a specific provider, a user just has to set FI_PROVIDER_PATH to the custom location or unset to use internal copy.

Possible Solution 4: Replace provider version checking with ABI version checking.

Use the highest FABIRC_ABI, otherwise first found in PATH. Middle ground between current behavior and Solution 3

Cons:

  • Changes default behavior
  • An older OFI provider in the path may be picked up when a newer one exist internal or later in the path

Pros:

  • To select a PROVIDER, a user just has to set FI_PROVIDER_PATH to the custom location or unset to use internal copy.
@shefty
Copy link
Member

shefty commented Apr 23, 2021

Changing current behavior should be avoided unless there is no reasonable alternative.

Today, FI_PROVIDER can either be used to indicate which providers to enable or which providers to disable. A URI model would allow for a mix. It would also allow specifying multiple options for a single provider. URI format is defined for FI_ADDR_STR, so URI parsing is already needed by the library and is consistent with the rest of the API. (See https://ofiwg.github.io/libfabric/v1.12.1/man/fi_getinfo.3.html - Addressing Formats). In the context of the problem to solve, we essentially need to look for "?dl=only" to be attached to the end of a provider name. When that occurs, the internal provider is skipped. Anything else is a future enhancement/problem. But if a provider were listed twice, I would just throw an error.

A URI would usable directly by an application through the API, which avoids having an environment variable as the only way to specify provider selection details. We have problems with apps accessing libfabric through different middleware, with one setting FI_PROVIDER, which disables the desired provider from the other.

I'm not a fan of trying to expose some internal algorithm and expecting that to mean something to a user. There can be multiple versioned DLs present for a single provider in the same installed directory. That's why a search path isn't sufficient.

@shefty shefty changed the title Supplying FI_PROVIDER_PATH does not guarantee the external provider is selected Support multiple builds/versions of the same provider with different features enabled Apr 24, 2021
@shefty
Copy link
Member

shefty commented Apr 24, 2021

After more thought, I believe the URI approach is the correct one. A strong reason is that it's usable directly with the existing API. Environment variables should not be used to work-around gaps in the API, such that they become an API extension for practical purposes. For example, FI_PROVIDER is widely used, but technically not needed. It's used to work around limitations in the applications, not the libfabric API.

The larger problem this issue is exposing is that libfabric only supports one provider for any given name. This is an implementation limitation, which we can relax. Libfabric could support multiple builds of the same provider, and by default order them by version. Applications that select the first fi_info that fi_getinfo returns would get the same behavior. I think this is the first change that we need.

To be clear, I'm not only suggesting that libfabric support different versions (1.11.0, 1.12.0) of a provider, but different builds of the same version. The builds would have been configured differently. Ideally, different builds would have some sort of distinguishing id, but that probably goes beyond what a numeric value can capture.

Once we allow multiple copies of the same provider, the second issue is to define a way for an application to prefer one over the other. The URI provides this. The discussion so far has focused on whether to use the internal or DL provider, or a specific version. But I'll argue that's really not what the application cares about. The application is more concerned about whether the build it's using supports some desired feature. The DL/version selection is acting as a proxy for that. It would be better if the request were more explicit, for example, prov=psm3?rndv=enabled.

There are a couple of options to have that level of filtering. One, the provider handles it. The can be achieved if we require all providers above a certain API version (say >= 1.13.0) to do this. For forward compatibility, if a provider sees an unrecognized key-value pair, it must fail the getinfo request. The core would automatically skip older providers if any key-value pair is present.

The second option is for the core to handle this. This requires the provider to include it's supported key-value pairs as part of the provider name. As before, if a provider does not indicate that a given key is supported, the core would skip the provider. My initial guess is that provider filtering may produce much simpler code than trying to construct some generic filtering mechanism.

In either case, the core can still easily perform filtering based on DL/version. So, that's the only solution that needs to be implemented immediately. However, I think we design and begin implementing for the case where the key-value option is more explicit. This may be as simple as updating providers above an API version to be aware that key-value pairs may be used.

Setting FI_PROVIDER would need to act the same as if the application set the key-value pair with getinfo.

@acgoldma
Copy link
Contributor Author

One thing I thought of is, that we currently load all the providers and limit to one of each name before we even ask what the user wants (fi_info hints).
While this is a more complex change, maybe we need to load all providers and store the fi_info structs, then filter by hints.
OR Even better: Delay loading providers until they are asked for. We would need to add things that were in the internal prov_info struct into the fi_info structs, and maybe then we can add new per provider capabilities. This would also work with the multiple version support you talked about in a previous comment.

An Example: So if I want provider psm3 that is equal to or newer than v1.12.2, which also has both cuda support and rv support:

flags |= FI_PROV_INFO;  // New flag to show the new struct below is populated
hints.prov_info.{ 
  .capmask = PSM3_CUDA | PSM3_RV,       // Custom Provider defined caps (configured at build time)
  .version = FI_VERSION(112,20),        // v1.12.2 
  .version_opt = FI_PROV_VER_OPT_GE,    // >=,  other enums:  _GT, LT, LE, NE, EQ  
}

Though this would be a massive API change, I believe it would be interesting.

If we delayed the loading of providers, we could even optimize only loading requested providers based upon input to the hints in fi_getinfo(). Like if you request tcp, it will only call register on providers that have tcp in the name. This would optimize by only loading/unloading tcp providers instead of all providers (which is what is currently is done).

We might have to maybe preload with some minor details like name, version(s), capmask, but would simplify what we store during fi_init(), and not have to have each provider call it's own init() functions.

@shefty
Copy link
Member

shefty commented Apr 28, 2021

Yes, I was suggesting that we store all copies of a provider. I believe this is a required, fundamental change.

Until a provider has been 'loaded', libfabric has no idea of the provider's name, version, etc. Utility and hooking providers are enabled without the user explicitly selecting them. I don't see that trying to defer loading is feasible. libfabric has to create the database of fi_provider structures.

For flags, the API reserves the upper 4 bits for provider use. Technically one of those bits could be used to indicate that extended attributes are present, so there's more flexibility than just 4 bits available. But capability bits should really be used for application visible features, not selecting provider implementation details. So, while HMEM support is a defined cap already, something like 'rendezvous' is really a provider implementation detail. Still, the provider flag bits could be an option in some cases.

Following that path, at one level, we're wanting an admin to modify an application's fi_getinfo call by specifying additional attributes. The top-level hook that we have for this is the FI_PROVIDER environment variable, but that only impacts the provider name. There are some provider specific environment variables that can override other attributes (tx size, use shared receives, ...). But there's no top level, generic fi_getinfo 'hook'. If we had this, some of the provider specific overrides could go away. IMO, this is worth investigating, possibly separately.

For immediate needs, it may work to define FI_PROVIDER_VERSION as an additional fi_getinfo filter. A string gives us more flexibility than the API. E.g. "=v1.12.2" vs ">=1.12.0". That gives us a starting point for selecting between two versions of the same provider. Other options require some level of API extension.

But the extensions aren't something an application is likely to set. So the main mechanism for setting those will be via key-value pairs through environment variables, which needs to be user friendly. libfabric won't know how to translate something like "PSM3_RV=yes" into an equivalent bit value, and specifying "PROV_CAPS=5" to set bits 1 & 2 isn't user friendly. I think the best option here is for the provider to give a list of key-value pairs that it supports, in string format, and for libfabric to use that as part of provider selection.

@acgoldma
Copy link
Contributor Author

I think the best option here is for the provider to give a list of key-value pairs that it supports, in string format, and for libfabric to use that as part of provider selection.

We do somewhat have the 'api' for this through the fi_param_define() functions. In the fi_prov_ini() a provider could define its own custom options to check and we could add a list to the fi_provider struct for possible expansion. For now though we can have a general param list that applies to all providers like "dl" "version"

Something like:

int fi_prov_param_define(
    const struct fi_provider *provider, // NULL means store in general list?
    const char *param_name,
    enum fi_param_type type,
    const void *possible_values,
    const char help_string_fmt, ...)  <-- not sure if we need this, but could be useful for debugging.

all but possible_values are same as fi_param_define.

  • For types bool: fi_parse_bool() is used. possible_values is ignored as only 2 possible inputs (0/1).

  • For int type: strtol() is used. possible_values is range string (e.g. "0-3,5"). Not sure how negative values would be handled here? Maybe an array of 2 char strings. { "Negative range", "Positive Range"}
    Example: {"22-5", "0,2-5"} which translates to (-22) to (-5) or 0 or (+2) to (+5)

  • For string type: possible_values would be an array of char strings with last index into array being NULL pointer.

@shefty
Copy link
Member

shefty commented Apr 28, 2021

Interesting -- my half baked thought on this:

If we define a new fi_param_type (FI_PARAM_KEYVALUE), a provider could call fi_param_define() to inform libfabric of its kv-pairs. In this case, the param_name is formatted as "name=value".

struct fi_param_entry is expanded to capture the desired value.

Somewhere in this, the libfabric core checks the environment variables to see if any key-value parameters have been set. If any have been set, then fi_getinfo() only calls core providers that have a matching key-value parameter defined.

From the provider's perspective, it would do:

fi_param_define(&my_prov, "rendezvous=yes", FI_PARAM_KEYVALUE, "require rendezvous support");

and the user would do:

export FI_PSM3_RENDEZVOUS=yes

If we expand the fi_param_type's, it could add a little more flexibility. For example, FI_PARAM_KV_STRING would perform string matching (what was shown above). We could also define FI_PARAM_KV_BOOL, which would allow the user to set:

export FI_PSM3_RENDEZVOUS=yes
or
export FI_PSM3_RENDEZVOUS=true
or
export FI_PSM3_RENDEZVOUS=1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants