Skip to content

Topic/osc controls #2626

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

Closed
wants to merge 2 commits into from
Closed

Topic/osc controls #2626

wants to merge 2 commits into from

Conversation

gpaulsen
Copy link
Member

@gpaulsen gpaulsen commented Dec 21, 2016

Adding mca parameter "enable" to enable / disable osc_rdma and osc_pt2pt.
Adding mca parameter "allow_thread_multiple" to enable (default) / disable osc_rdma and osc_pt2pt when user passed in MPI_THREAD_MULTIPLE to MPI_Init_threads().

Due to the Issue #2614, we recommend that v2.0.2 gets released with the following additions to the etc/openmpi-mca-params.conf file:

osc_rdma_enable = 1
osc_rdma_allow_thread_multiple = 0
osc_pt2pt_enable = 1
osc_pt2pt_allow_thread_multiple = 0

This PR does NOT make any changes to the mca params.conf file, the release engineer should make that decision / change.

Signed-off-by: Geoffrey Paulsen <gpaulsen@us.ibm.com>
   Adding enable, priority, verbose, and allow_thread_multiple.

Signed-off-by: Geoffrey Paulsen <gpaulsen@us.ibm.com>
Copy link
Member

@jjhursey jjhursey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bosilca
Copy link
Member

bosilca commented Dec 21, 2016

If we add these 4 MCA parameters we are bound to provide them in future releases (at least as synonyms), even after we fix the threading issues in the OSC components. Instead, why not simply asking for the osc MCA parameter to be explicitly defined in he multi-threaded case ?

I can't test right now to see when the OSC module component_init function called ? If it is called directly in MPI_Init, then we will have a lot of spurious failures when threading is enabled, even when one-sided is not used.

@gpaulsen
Copy link
Member Author

Sorry, I can't quite parse: "why not simply asking for the osc MCA parameter to be explicitly defined in he multi-threaded case ?" Can you elaborate?

Yes, component_init is called directly or indirectly at MPI_Init and MPI_Init_thread time. That's when this PR checks to see if the user requested MPI_Init_thread, but also has osc_pt2pt_allow_thread_multiple = 0, and then disallows that component. What sort of errors are you envisioning in this model?

@hppritcha
Copy link
Member

We will only retain these MCA params for 2x series.
👍

@gpaulsen
Copy link
Member Author

So should I not push to master? Only PR them against 2.0.x and 2.x ?

@bosilca
Copy link
Member

bosilca commented Dec 22, 2016

@gpaulsen in the same way as we manually specify the BTL or PML we want to use (--mca btl ...), we can specify the OSC components to be used. Thus, if the user does not manually add "--mca osc rdma" to his MCA params, we can disqualify the RDMA module in the multi-threaded case.

If component_init is called during the initialization phase, what will happen if all OSC components disqualify themselves when threading support is requested ? Will the initialization succeed or it will fail because no OSC components are available ? If the latter, then we have a problem.

@hppritcha this is not how we did things in the past. We kept the MCA parameters, either as synonyms or as deprecated parameters for a while.

@hppritcha
Copy link
Member

hppritcha commented Dec 22, 2016

@gpaulsen I'm now thinking it would be better to abandon this approach and simply patch 2.0.x to exclude P2P OSC in the select operation until we have a fix. I don't see a need for any MCA parameter or even environment variable as developers would fix the problem on master then push the fix back to 2.0.x branch. I'll open an issue and link it with 2614 to remind the developer(s) to revert the commit that will be associated with this patch.

@gpaulsen
Copy link
Member Author

gpaulsen commented Dec 22, 2016

So, the 'enable' parameter is similar to some other components that have an enable parameter. Obveously there are a few ways to disable a component:

  1. specify --mca btl ^component
  2. remove the component shared library from the install.
  3. some components have an 'enable' MCA parameter.

The reason that we prefer #3 is that the distro or the system admin can disable a component via the OPAL_PREFIX/etc/openmpi-mca-params.conf file easily.

As far as the 'enable_thread_multiple' MCA parameter, that is a bit weirder as it's conditional upon the value of MPI_THREAD_INIT, and we hope that it gets fixed in a future release.

I tested with no osc components, and it passes through MPI_Init (or MPI_Init_thread), but then fails at MPI_Win_create with the following message:
[host:pid] *** An error occurred in MPI_Win_create
[host:pid] *** reported by process [pid,3]
[host:pid] *** on communicator MPI_COMM_WORLD
[host:pid] *** MPI_ERR_WIN: invalid window
[host:pid] *** MPI_ERRORS_ARE_FATAL (processes in this communicator will now abort,
[host:pid] *** and potentially your MPI job)

It would be nice to have a better error message in this case, but we should be able to detect this and produce a nicer error message (I just don't know how yet).

@rhc54
Copy link
Contributor

rhc54 commented Dec 22, 2016

I'm a little confused here - can someone please clarify something for me? We have never included values in the template default MCA param file before, and we shouldn't be doing so. It is impossible to predict what different installations need.

If something needs to be disabled, let the system admin do so. I would urge that we not set a new precedent in this regard.

@hppritcha
Copy link
Member

I opened a PR #2630 which ought to address @bosilca 's concerns about mca parameters for workarounds like this one.

@hppritcha
Copy link
Member

If we don't reach a consensus by COB today on how to do a hack workaround for #2614, I'm going to declare 2.0.2rc2 dead wrt to a release in 2016. We can do an rc2 in first week of January, and hopefully get a release out either end of first or second week of January.

@rhc54
Copy link
Contributor

rhc54 commented Dec 22, 2016

FWIW: I think #2630 looks fine to me.

@gpaulsen
Copy link
Member Author

I think #2630 would be sufficient for osc_pt2pt for v2.0.2, but we've also seen hangs in osc_rdma component as well. We haven't tested osc_rdma enough to know that there aren't wrong answers, but we're concerned enough that we recommend also disabling osc_rdma for the MPI_THREAD_MULTIPLE case.

We should discuss this situation at the face to face and see if we can come up with some general procedures (disable things via conf files, or hard coded disables, or documentation, etc) that works for everyone. MCA parameter seems very useful for this situation, so I'd like to hear folk's opinions.

@rhc54
Copy link
Contributor

rhc54 commented Dec 22, 2016

It depends on the type of problem. If something truly doesn't work for a particular case (e.g., MPI_THREAD_MULTIPLE), then we disable it internally by checking for that case. We wouldn't require someone to set an MCA parameter for that purpose as we know it doesn't work in all environments.

If we have a scenario where something does work, but not in a particular environment, then we try to detect that environment and again disable it internally.

If something works, but there is an application-dependent tradeoff (e.g., a performance hit) associated with using it, then we use MCA params to help dictate selection. This is what @bosilca was alluding to.

This seems to be to be a situation where we honestly aren't sure these components work in threaded situations. This is detectable, and we shouldn't be using MCA params to deal with it - we should simply detect that this is a multi-threaded scenario and disable the component.

We should also continue our policy of never putting a param in the default MCA param file we distribute. It can take a lot of effort for someone to track down that unexpected behavior was caused by some unknown param being set.

@hppritcha
Copy link
Member

I'm reluctant to disable OSC rdma, but if the consensus is that we should do that, I'll modify #2630 to handle disable rdma as well. @hjelmn

@hjelmn
Copy link
Member

hjelmn commented Dec 22, 2016

osc/rdma is thread safe. do you have a reproducer that shows otherwise?

@hjelmn
Copy link
Member

hjelmn commented Dec 22, 2016

btw, we already have a way to enable/disable a component. --mca osc.

@gpaulsen
Copy link
Member Author

That all sounds reasonable to me. I'm still learning, so I appreciate everyone's patience.

I only question the "never put a param in the default MCA param file". If the value is NOT set in that file, there is STILL a (more hidden in my way of thinking) default that's passed in via the mca_base_component_var_register() function. In other projects I've seen a policy of always putting the same default that's in the registration call in the conf file also, so that all options are more visible to customers. Of course those values can be sometimes be queried using ompi_info and mpirun if run in the same environment as the job (not always easy to do).

Regarding the hard coding the disabling of components, we have seen cases in the past where customers are upset because we've had to disable a component for a particular release due to wrong answers, but they are certain that their app is not tickling the wrong answers and want to compare apples to apples (sometimes even if they DO get a wrong answer for performance comparisons). This is why, in the past, we've provided some runtime tunable to enable even the wrong-answers case, if possible. I agree, generally not as useful, but this is our rational.

@gpaulsen
Copy link
Member Author

@hjelmn - osc_rdma hang with MPI_THREAD_MULTIPLE can be seen with 1sided_mt.c here:
https://github.com/open-mpi/ompi-tests/pull/25

@rhc54
Copy link
Contributor

rhc54 commented Dec 22, 2016

What happens when you remove the parameter from the default param file? If the default value is still applied, then doesn't that confuse people? After all, they saw the param in the file, and then removed it - so why isn't it "gone"?

I hear what you are saying about the hard coding issue. Alternative is to shut it off unless someone specifically requests it via MCA param. We've used that before too.

@hjelmn
Copy link
Member

hjelmn commented Dec 22, 2016

Ok, will take a look later today.

@gpaulsen
Copy link
Member Author

Yeah. users... Can't make em all happy.

I put a topic in the face to face agenda to discuss... I'll try to frame the conversation. It's just an extension of the general task of making the UI better, which of course is subjective and never ending, but good to talk about, and make progress on.

@gpaulsen
Copy link
Member Author

I'm closing this PR, as the community chose to go a different direction in:
PR #2630.

@gpaulsen gpaulsen closed this Dec 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants