prov/verbs: Reload the list of interfaces on each call to fi_getinfo() #10886

sydidelot · 2025-03-19T15:33:49Z

The verbs provider loads the list of verbs devices available on the node
only once, on the first call to fi_getinfo().
If a network device is added (hot-plugged) after this initial call to
fi_getinfo(), it won't be visible in libfabric.

A subsidiary problem is that fi_getinfo() only returns network
adapters with active links.
If the link is initially inactive and becomes active after the first call to
fi_getinfo(), this interface will not be visible in libfabric.

This is a particularly a problem for long-running services where restarting
a process to discover newly added network devices is not an option.

With this patch, the list of verbs interfaces is reloaded on each call
to fi_getinfo().

Fixes #10881

Signed-off-by: Sylvain Didelot [email protected]

shefty · 2025-03-20T00:22:02Z

Re-scanning the interfaces is expensive. Utility providers will force scanning multiple times.

sydidelot · 2025-03-20T08:06:39Z

@shefty would it be possible to give the application control over when the interfaces should be re-scanned?
For example using an environment variable or a flag to fi_getinfo()?

shefty · 2025-03-20T17:49:20Z

@sydidelot - Giving control to the app is the best idea I could come up with to handle this. Going that route, I'd define a flag to pass into fi_getinfo(). An alternative I could think of that wouldn't involve application changes is to make refresh timer based. I'm not thrilled with that option. An environment variable to set the fi_getinfo() flag seems better. I think that could allow the util providers to unset the flag after it first passed to the core provider.

sydidelot · 2025-03-20T18:10:12Z

@shefty

An environment variable to set the fi_getinfo() flag seems better. I think that could allow the util providers to unset the flag after it first passed to the core provider.

I don't follow this point. Can you please clarify it please?
I was thinking about scanning the interfaces on the first call to fi_getinfo(), then re-scan on subsequent calls to fi_getinfo() only if the flag is that. This approach has the advantage of keeping the current behavior of fi_getinfo() while allowing new applications to explicitly request a re-scan using this flag.

shefty · 2025-03-20T22:09:51Z

I was suggesting defining a flag that can be passed into fi_getinfo(). When set, a new scan is performed.

The utility providers can always clear that flag, assuming that the core provider is called directly first, before any utility providers process fi_getinfo. (This is the case).

In addition, an environment variable could be set which the libfabric core can check. If set, the libfabric core would add this flag when calling the core provider's getinfo handler.

sydidelot · 2025-03-20T22:35:21Z

Thanks for the clarification @shefty! I didn't know that the core provider was called first.
I will incorporate these changes to the PR.

sydidelot · 2025-03-26T14:40:06Z

@shefty May I ask you to have a look at the new code when you have time?
I added the new getinfo flag as you suggested.
Thank you!

shefty

I did not review the last patch. I'll need more time for that. See comments so far.

prov/verbs/src/verbs_domain.c

prov/verbs/src/verbs_info.c

prov/verbs/src/verbs_domain.c

shefty · 2025-03-26T18:37:58Z

prov/rxd/src/rxd_init.c

+	if (rxd_env.always_rescan)
+		flags |= FI_RESCAN;
+	else
+		flags &= ~FI_RESCAN;


If the user doesn't set any environment variable, this has the impact of removing the FI_RESCAN flag from their request. That's probably not what we want. I would leave the flags unchanged in this case.

I updated the code and always_rescan is now an integer that can have the following values:

-1: The user didn't set the environment variable

0: rescan explicitly disabled by the environment variable

1: rescan explicitly enabled by the environment variable

Note that the value is now retrieved with fi_param_get_bool() but the underlying value is effectively an integer (and not a boolean).

Maybe convert to an enum? It should be okay to use get_bool, as that will leave the value alone if the variable isn't defined. I'd rename to just 'rescan' if false means never.

It seems that this would be a common flow (unset, false, true), but I don't see anything similar in tree based on a quick search.

I renamed the parameter to just 'rescan' as you suggested but I kept the boolean. Converting it to an enum will require some extra work and I think it is beyond the scope of this PR.

prov/rxm/src/rxm_init.c

shefty · 2025-03-26T18:55:09Z

prov/verbs/src/verbs_info.c

-
-	initialized = true;
+	vrb_devs_free(verbs_devs);
+	fi_freeinfo((void *)*all_infos);


Cast shouldn't be needed. Please update the function signature to remove the const.

If we want to remove the "const" in this function, we will also have to change the definition of util_prov::info and remove the "const" from there. Here is how the structure is defined right now:

struct util_prov { const struct fi_provider *prov; const struct fi_info *info; <--- here. ofi_alter_info_t alter_defaults; const int flags; };

Is it what you want?

Hmm... when that was defined, I think the assumption was that the info list was a static list of structures. But verbs needs to set this dynamically, including freeing it. Maybe that const should be removed. Can you tell if removing it causes an issue in a bunch of other places?

Removing the "const" only causes an issue to the verbs provider. I added a commit to remove it as a consequence.

shefty · 2025-03-26T21:27:45Z

The reason to add the FI_RESCAN flag is to only rescan if that flag is set. This maintains the current behavior and performance. Setting the environment variable can then be used to force rescan on every call into the verbs provider.

sydidelot

@shefty Thanks for the review! I have addressed all your review comments.

prov/verbs/src/verbs_domain.c

prov/verbs/src/verbs_info.c

sydidelot · 2025-03-27T10:37:17Z

prov/rxd/src/rxd_init.c

+	if (rxd_env.always_rescan)
+		flags |= FI_RESCAN;
+	else
+		flags &= ~FI_RESCAN;


I updated the code and always_rescan is now an integer that can have the following values:

-1: The user didn't set the environment variable

0: rescan explicitly disabled by the environment variable

1: rescan explicitly enabled by the environment variable

Note that the value is now retrieved with fi_param_get_bool() but the underlying value is effectively an integer (and not a boolean).

prov/rxm/src/rxm_init.c

sydidelot · 2025-04-01T06:56:16Z

@j-xiong May I ask you to check why the Intel integration tests are failing? Are the failures related to the PR?
Thank you!

j-xiong · 2025-04-04T00:47:09Z

@sydidelot Please rebase to pick up github action changes.

sydidelot · 2025-04-04T08:58:24Z

@j-xiong Rebased but it's still failing in Intel CI pipeline. Can you please let me know what tests are failing? I assume they are related to my changes.

j-xiong · 2025-04-04T14:20:40Z

There were a lot of failures, probably issue with CI itself. Restarted the tests.

sydidelot · 2025-04-08T20:02:26Z

@j-xiong My PR is still failing and I have no visibility into the tests results.

j-xiong · 2025-04-08T20:12:43Z

@sydidelot We are having infrastructure issue with CI, still under investigation.

j-xiong · 2025-04-15T20:30:36Z

Fabtests over rxm/verbs failed with the following error:

./include/ofi_lock.h:257: ofi_mutex_unlock: Assertion `lock->in_use' failed.

prov/verbs/src/verbs_info.c

With the next changes, vrb_init_mutex will not only be used during initialization, but it will also be used for protecting updates to vrb_util_prov.info Rename it "vrb_info_mutex" so the new name better reflects the new role of this mutex. Signed-off-by: Sylvain Didelot <[email protected]>

With the next changes to allow dynamic reloads of the verbs interfaces, the fabric can no longer hold a reference to vrb_util_prov.info, otherwise it may cause a use-after-free if the interface is removed from that list. Signed-off-by: Sylvain Didelot <[email protected]>

Rework the provider to make vrb_get_verbs_info() static as the function is only called from verbs_domain.c Signed-off-by: Sylvain Didelot <[email protected]>

Because we will dynamically update the list vrb_util_prov.info at run time, we need to serialize all accesses to that list with vrb_info_mutex. Signed-off-by: Sylvain Didelot <[email protected]>

Careful, API change! This commit adds the new flag FI_RESCAN to indicate that the provider should rescan available network interfaces. This operation may be computationally expensive but required for paiplications that need to re-scan network interfaces at run time. Signed-off-by: Sylvain Didelot <[email protected]>

Signed-off-by: Sylvain Didelot <[email protected]>

Double pointer is not needed and can be removed to simplify the code. Signed-off-by: Sylvain Didelot <[email protected]>

The verbs provider requires to set this list dynamically, so make it non const. Signed-off-by: Sylvain Didelot <[email protected]>

The verbs provider loads the list of verbs devices available on the node only once, on the first call to fi_getinfo(). If a network device is added (hot-plugged) after this initial call to fi_getinfo(), it won't be visible in libfabric. A subsidiary problem is that fi_getinfo() only returns network adapters with active links. If the link is initially inactive and becomes active after the first call to fi_getinfo(), this interface will not be visible in libfabric. This is a particularly a problem for long-running services where restarting a process to discover newly added network devices is not an option. With this patch, the list of verbs interfaces is reloaded on each call to fi_getinfo(). Fixes ofiwg#10881 Signed-off-by: Sylvain Didelot <[email protected]>

sydidelot · 2025-04-24T10:08:45Z

@j-xiong Thanks for catching this stupid bug! I fixed the PR a few days ago and it now passes the CI pipeline.
@j-xiong @shefty Can I request a new review from you? Also, should we add a few tests in fabtests/unit/getinfo_test.c for the new FI_RESCAN flag to getinfo? Thank you!

j-xiong · 2025-04-24T15:36:13Z

@sydidelot Adding tests for the new flag is a good idea. The question though, is how to create a scenario that the flag can show a difference.

j-xiong · 2025-04-25T20:45:43Z

@shefty Do you have any more concerns with this PR?

shefty

Only change request is to clean up the text describing the env var. Thanks!

shefty · 2025-04-25T21:12:56Z

prov/rxd/src/rxd_init.c

+	fi_param_define(&rxd_prov, "rescan", FI_PARAM_BOOL,
+			"Enables dynamic network interface detection by rescanning "
+			"system interfaces on each fi_getinfo() invocation. "
+			"(default: -1)");


I'd modify the text to something like: "Force or disable rescanning for network interface changes... (default: unset)". Replace -1 with unset, since -1 doesn't make much sense to the user reading this as a boolean value. Or simply drop the default text completely. I would make the text clear that setting this to true will force rescanning, while setting it to false will disable rescanning.

sydidelot force-pushed the verbs_getinfo branch from 4a490f0 to 209a93e Compare March 20, 2025 08:07

sydidelot force-pushed the verbs_getinfo branch from 209a93e to b81f941 Compare March 26, 2025 13:08

shefty reviewed Mar 26, 2025

View reviewed changes

sydidelot force-pushed the verbs_getinfo branch from b81f941 to 83f2396 Compare March 27, 2025 10:45

sydidelot commented Mar 27, 2025

View reviewed changes

sydidelot requested a review from shefty April 1, 2025 06:51

sydidelot force-pushed the verbs_getinfo branch from 83f2396 to a2059eb Compare April 1, 2025 20:13

sydidelot force-pushed the verbs_getinfo branch from a2059eb to e4c54d1 Compare April 4, 2025 06:38

sydidelot force-pushed the verbs_getinfo branch from e4c54d1 to 84ca6b6 Compare April 5, 2025 20:56

j-xiong reviewed Apr 16, 2025

View reviewed changes

prov/verbs/src/verbs_info.c Show resolved Hide resolved

prov/verbs/src/verbs_info.c Show resolved Hide resolved

sydidelot added 5 commits April 16, 2025 19:39

prov/verbs: Make vrb_get_verbs_info() static

55c9109

Rework the provider to make vrb_get_verbs_info() static as the function is only called from verbs_domain.c Signed-off-by: Sylvain Didelot <[email protected]>

prov/verbs: Always protect vrb_util_prov.info accesses with the mutex

41fd0c9

Because we will dynamically update the list vrb_util_prov.info at run time, we need to serialize all accesses to that list with vrb_info_mutex. Signed-off-by: Sylvain Didelot <[email protected]>

sydidelot added 5 commits April 16, 2025 19:39

prov/rxd: Add an environment variable to control FI_RESCAN

3871ef8

Signed-off-by: Sylvain Didelot <[email protected]>

prov/rxm: Add an environment variable to control FI_RESCAN

b1cead7

Signed-off-by: Sylvain Didelot <[email protected]>

prov/verbs: Use single pointer instead of double pointer

39f5f2e

Double pointer is not needed and can be removed to simplify the code. Signed-off-by: Sylvain Didelot <[email protected]>

ofi_util.h: Make util_prov::info non const

b28d893

The verbs provider requires to set this list dynamically, so make it non const. Signed-off-by: Sylvain Didelot <[email protected]>

sydidelot force-pushed the verbs_getinfo branch from 84ca6b6 to 4fe0c97 Compare April 16, 2025 19:42

sydidelot requested a review from j-xiong April 24, 2025 10:08

j-xiong approved these changes Apr 24, 2025

View reviewed changes

shefty requested changes Apr 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prov/verbs: Reload the list of interfaces on each call to fi_getinfo() #10886

prov/verbs: Reload the list of interfaces on each call to fi_getinfo() #10886

sydidelot commented Mar 19, 2025

shefty commented Mar 20, 2025

sydidelot commented Mar 20, 2025

shefty commented Mar 20, 2025

sydidelot commented Mar 20, 2025

shefty commented Mar 20, 2025

sydidelot commented Mar 20, 2025

sydidelot commented Mar 26, 2025

shefty left a comment

shefty Mar 26, 2025

sydidelot Mar 27, 2025

shefty Apr 1, 2025

sydidelot Apr 1, 2025

shefty Mar 26, 2025

sydidelot Mar 27, 2025

shefty Apr 1, 2025

sydidelot Apr 1, 2025

shefty commented Mar 26, 2025

sydidelot left a comment

sydidelot Mar 27, 2025

sydidelot commented Apr 1, 2025

j-xiong commented Apr 4, 2025

sydidelot commented Apr 4, 2025

j-xiong commented Apr 4, 2025

sydidelot commented Apr 8, 2025

j-xiong commented Apr 8, 2025

j-xiong commented Apr 15, 2025

sydidelot commented Apr 24, 2025

j-xiong commented Apr 24, 2025

j-xiong commented Apr 25, 2025

shefty left a comment

shefty Apr 25, 2025

prov/verbs: Reload the list of interfaces on each call to fi_getinfo() #10886

Are you sure you want to change the base?

prov/verbs: Reload the list of interfaces on each call to fi_getinfo() #10886

Conversation

sydidelot commented Mar 19, 2025

shefty commented Mar 20, 2025

sydidelot commented Mar 20, 2025

shefty commented Mar 20, 2025

sydidelot commented Mar 20, 2025

shefty commented Mar 20, 2025

sydidelot commented Mar 20, 2025

sydidelot commented Mar 26, 2025

shefty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shefty commented Mar 26, 2025

sydidelot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sydidelot commented Apr 1, 2025

j-xiong commented Apr 4, 2025

sydidelot commented Apr 4, 2025

j-xiong commented Apr 4, 2025

sydidelot commented Apr 8, 2025

j-xiong commented Apr 8, 2025

j-xiong commented Apr 15, 2025

sydidelot commented Apr 24, 2025

j-xiong commented Apr 24, 2025

j-xiong commented Apr 25, 2025

shefty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment