{"action":"created","issue":{"url":"https://api.github.com/repos/open-mpi/ompi/issues/415","labels_url":"https://api.github.com/repos/open-mpi/ompi/issues/415/labels{/name}","comments_url":"https://api.github.com/repos/open-mpi/ompi/issues/415/comments","events_url":"https://api.github.com/repos/open-mpi/ompi/issues/415/events","html_url":"https://github.com/open-mpi/ompi/pull/415","id":58560345,"number":415,"title":"Fix the calls to ibv_fork_init and remove btl_openib_want_fork_support.","user":{"login":"alinask","id":2405183,"avatar_url":"https://avatars.githubusercontent.com/u/2405183?v=3","gravatar_id":"","url":"https://api.github.com/users/alinask","html_url":"https://github.com/alinask","followers_url":"https://api.github.com/users/alinask/followers","following_url":"https://api.github.com/users/alinask/following{/other_user}","gists_url":"https://api.github.com/users/alinask/gists{/gist_id}","starred_url":"https://api.github.com/users/alinask/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alinask/subscriptions","organizations_url":"https://api.github.com/users/alinask/orgs","repos_url":"https://api.github.com/users/alinask/repos","events_url":"https://api.github.com/users/alinask/events{/privacy}","received_events_url":"https://api.github.com/users/alinask/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":23,"created_at":"2015-02-23T09:12:20Z","updated_at":"2015-02-24T16:02:22Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/open-mpi/ompi/pulls/415","html_url":"https://github.com/open-mpi/ompi/pull/415","diff_url":"https://github.com/open-mpi/ompi/pull/415.diff","patch_url":"https://github.com/open-mpi/ompi/pull/415.patch"},"body":"In order to have an effect, ibv_fork_init should be called in the\r\nbeginning of the verbs initialization flow - before the calls to the\r\nibv_create_qp and ibv_create_cq verbs.\r\nThese functions are called from the oob/ud code and by the time the\r\nother verbs components (btl openib, pml yalla, ...) call ibv_fork_init,\r\nit's too late. This commit forces the call to ibv_fork_init (if it's\r\nrequested) right at the beginning of all the components that are using\r\nverbs.\r\n\r\nThis commit also removes the btl_openib_want_fork_support mca parameter\r\nand adds a new mca parameter instead - opal_verbs_want_fork_support.\r\nThrough this new parameter, fork support may be requested for ALL\r\ncomponents.\r\nThe default value for this parameter is set to 1.\r\n\r\nBefore this commit the btl_openib_want_fork_support parameter didn't\r\nprovide fork support for the openib btl if its value was set to 1.\r\n(because when openib called ibv_fork_init, it was already after the\r\ncalls to ibv_create_* in oob/ud and thereofre it failed).\r\n\r\nreviewed by @miked-mellanox "},"comment":{"url":"https://api.github.com/repos/open-mpi/ompi/issues/comments/75784332","html_url":"https://github.com/open-mpi/ompi/pull/415#issuecomment-75784332","issue_url":"https://api.github.com/repos/open-mpi/ompi/issues/415","id":75784332,"user":{"login":"jsquyres","id":617316,"avatar_url":"https://avatars.githubusercontent.com/u/617316?v=3","gravatar_id":"","url":"https://api.github.com/users/jsquyres","html_url":"https://github.com/jsquyres","followers_url":"https://api.github.com/users/jsquyres/followers","following_url":"https://api.github.com/users/jsquyres/following{/other_user}","gists_url":"https://api.github.com/users/jsquyres/gists{/gist_id}","starred_url":"https://api.github.com/users/jsquyres/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jsquyres/subscriptions","organizations_url":"https://api.github.com/users/jsquyres/orgs","repos_url":"https://api.github.com/users/jsquyres/repos","events_url":"https://api.github.com/users/jsquyres/events{/privacy}","received_events_url":"https://api.github.com/users/jsquyres/received_events","type":"User","site_admin":false},"created_at":"2015-02-24T16:02:22Z","updated_at":"2015-02-24T16:02:22Z","body":"Here's a disconnect in our conversation: I was assuming that opal_common_verbs_fork_test() was something like this:\r\n\r\n```c\r\nint opal_common_verbs_fork_test(void)\r\n{\r\n    static bool have_called_this_already = false;\r\n    if (have_called_this_already) {\r\n        return ...;\r\n    }\r\n    have_called_this_already = true;\r\n\r\n    if (mca_var_says_we_want_ibv_fork_support) {\r\n        rc = ibv_fork_init();\r\n        return ...\r\n    }\r\n    return ...\r\n}\r\n```\r\n\r\nHence, `ibv_fork_init()` would actually only called once, not multiple times.\r\n\r\nI see that this is not the case in this PR.  The code is more like this:\r\n\r\n```c\r\nint opal_common_verbs_fork_test(void)\r\n{\r\n    if (mca_var_says_we_want_ibv_fork_support) {\r\n        rc = ibv_fork_init();\r\n        return ...\r\n    }\r\n   return ...\r\n}\r\n```\r\n\r\nI.e., there's no protection against calling `ibv_fork_init()` multiple times.\r\n\r\nI was assuming that since `opal_comon_verbs_fork_test()` is meant to be called multiple times, it should ensure that `ibv_fork_init()` is only called *once* and it simply caches the result.  Hence my confusion about your vs. Mike's words about invoking `ibv_fork_init` multiple times.\r\n\r\nThat being said, I think Rolf still has a good point: if **every** verbs-based component calls `opal_common_verbs_fork_test()` before it invokes any other verbs API calls, then we're guaranteed to be correct: fork support will be enabled for everyone, or it will be disabled for everyone.\r\n\r\nMy point: the discussion about the default value (0 or 1, and whether -1 is still relevant) is a different -- but still relevant -- issue.\r\n\r\nMake sense?"}}