coll/han: fix coll preference selection in mca_coll_han_comm_create_new

[devreal]

Exclude HAN, don't include it.

Signed-off-by: Joseph Schuchart [email protected]

Refs #8248

[gkatev]

I'm not sure that this was actually a typo. It kind of looks to me like the dynamic path is supposed to choose HAN for the subcomms, and then manually call other components' modules on each level. It is for this reason that mca_coll_han_comm_create_new sets the INTRA_NODE and INTER_NODE info keys, and many dynamic-related functions take action depending on the topology level (global, intra, inter).

Of course, there are other bugs with the dynamic path (#10438 #9883). It looks like this commit disables some buggy path, that triggers the segfault seen in #8248. Honestly the dynamic path itself and the way it combines with the non-dynamic one is all a bit confusing.

[bosilca]

Bot of you are correct. In an ideal situation, we would like to have a multi-level architecture awareness, and then at each level HAN will make a choice on how to break the collective. However, we never make a choice of module based on the topologic_level, because we limit the scope of HAN to only 2-levels hierarchies. Basically, the user visible communicator need to make a decision on what module to call on the underlying communicators, but as we don't support multi-level architecture-awareness we need to prevent the underlying communicators from using HAN.

[gkatev]

So if I'm understanding correctly, the dynamic path is designed to support multiple levels, but because it is not yet fully implemented, it is manually bypassed in favour of the traditional up/low path?

From reading the code it's not fully apparent to me why the dynamic path is not used even for just 2 levels of hierarchy, it looks like it would work(with small fixes/adjustments). It generally feels like there is duplication between the two paths.

[bosilca]

Yes, the original design called for multiple levels, but then it turned out we were lacking architectural information (basically netloc) to take advantage of this, so we went ahead and worked under a 2-levels assumption. If I remember correctly the only way to build a multi-level is using the decision files, and there you explicitly mark the modules you want to use, so we don't need to exclude/include anything.

[gkatev]

Would it make sense to merge the dynamic and non-dynamic paths? So, for the most part, join mca_coll_han_comm_create and mca_coll_han_comm_create_new, and the "plain" and "dynamic" MCA parameters, and perform all the adjustments that these changes entail. All this still under the 2-level assumption, but with some possibility for future extension in mind. The reason to do so would be to make the component simpler to understand/use/modify, and to concentrate future development effort.

[bosilca]

That particular code path was added by the folks at Bull (@EmmanuelBRELLE, @bsergentm). I'm all for simplification as long as we retain similar capabilities, but maybe they want to chime in.

[FlorentGermain-Bull]

Hello
I confirm that han must be activated on sub_communicator when using mca_coll_han_comm_create_new.
On our side, we extended han to support an arbitrary number of topological levels (based on MPI_Comm_split_type for now). In few words, we just tell han which topological level han should be aware of and sub_communicator are created acording to it.

We consider pushing it to the github repo but we do not know when yet.

I think making han simpler is a good idea too. Merging mca_coll_han_comm_create and mca_coll_han_comm_create_new would need some work but it should now be achievable