)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"97a3d1b0fbc6b926cbe42751e38ddd85c84ec1a5","unresolved":false,"context_lines":[{"line_number":10,"context_line":"initialized. Normally we should never be in such situation but a race"},{"line_number":11,"context_line":"condition highlighted that case."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"The second one is that, currently the code is doing the right pinning"},{"line_number":14,"context_line":"because we go through \u0027sibling_sets\u0027 (plural) in a reverse order. So"},{"line_number":15,"context_line":"\u0027sibling_set\u0027 (singular) when initialized is using the group of cores"},{"line_number":16,"context_line":"with the smallest set of siblings: 1 to N."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"This patch is fixing the issue by ensuring that singling_sets is not"},{"line_number":19,"context_line":"empty and passing the group 1 to get the pinning."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"5ff73747_aa8d6b32","line":16,"range":{"start_line":13,"start_character":0,"end_line":16,"end_character":42},"updated":"2017-04-25 14:18:40.000000000","message":"I don\u0027t think so. See comment on the next page for more info.","commit_id":"494c49aa834b15f09c982ed750c108b03c5cec71"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"97a3d1b0fbc6b926cbe42751e38ddd85c84ec1a5","unresolved":false,"context_lines":[{"line_number":15,"context_line":"\u0027sibling_set\u0027 (singular) when initialized is using the group of cores"},{"line_number":16,"context_line":"with the smallest set of siblings: 1 to N."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"This patch is fixing the issue by ensuring that singling_sets is not"},{"line_number":19,"context_line":"empty and passing the group 1 to get the pinning."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: I7ea0ecd4ef1436098863e0b3bf6f776fc495d4e3"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"5ff73747_af09fd95","line":18,"range":{"start_line":18,"start_character":48,"end_line":18,"end_character":61},"updated":"2017-04-25 14:18:40.000000000","message":"sibling_sets","commit_id":"494c49aa834b15f09c982ed750c108b03c5cec71"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"97a3d1b0fbc6b926cbe42751e38ddd85c84ec1a5","unresolved":false,"context_lines":[{"line_number":19,"context_line":"empty and passing the group 1 to get the pinning."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: I7ea0ecd4ef1436098863e0b3bf6f776fc495d4e3"},{"line_number":22,"context_line":"Closes-bug: bug/1685226"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"5ff73747_6fead5ac","line":22,"range":{"start_line":22,"start_character":12,"end_line":22,"end_character":23},"updated":"2017-04-25 14:18:40.000000000","message":"#1685226","commit_id":"494c49aa834b15f09c982ed750c108b03c5cec71"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"1a19c89632dee985910cff79179af0ebd18967fd","unresolved":false,"context_lines":[{"line_number":10,"context_line":"initialized. Normally we should never be in such situation but a race"},{"line_number":11,"context_line":"condition highlighted that case."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"The second one is that, currently the code is doing the right pinning"},{"line_number":14,"context_line":"because we go through \u0027sibling_sets\u0027 (plural) in a reverse order. So"},{"line_number":15,"context_line":"\u0027sibling_set\u0027 (singular) when initialized is using the group of cores"},{"line_number":16,"context_line":"with the smallest set of siblings: 1 to N."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"This patch is fixing the issue by ensuring that sibling_sets is not"},{"line_number":19,"context_line":"empty and passing the group 1 to get the pinning."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"5ff73747_98aae8cf","line":16,"range":{"start_line":13,"start_character":0,"end_line":16,"end_character":42},"updated":"2017-04-26 11:00:28.000000000","message":"This is slightly confusingly worded and that\u0027s what threw me off. I think I understand you now though. What you\u0027re saying here is that we\u0027re relying on Python\u0027s leaking of index variables to the scope of the enclosing function [1]. Under normal circumstances, this shouldn\u0027t cause any issues because the last loop would have checked for sibling size of 1. However, this race is causing that to not be the case and therefore sibling_set to be unset, as you say.\n\n[1] http://eli.thegreenplace.net/2015/the-scope-of-index-variables-in-pythons-for-loops/","commit_id":"6be38fbc253ba9924325882120c50af02128f573"}],"nova/virt/hardware.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"97a3d1b0fbc6b926cbe42751e38ddd85c84ec1a5","unresolved":false,"context_lines":[{"line_number":880,"context_line":"                fields.CPUThreadAllocationPolicy.REQUIRE and"},{"line_number":881,"context_line":"                not pinning):"},{"line_number":882,"context_line":"            pinning \u003d list(zip(sorted(instance_cell.cpuset),"},{"line_number":883,"context_line":"                               itertools.chain(*sibling_sets[1])))"},{"line_number":884,"context_line":""},{"line_number":885,"context_line":"        threads_no \u003d _threads(instance_cell, threads_no)"},{"line_number":886,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5ff73747_ea681388","line":883,"range":{"start_line":883,"start_character":31,"end_line":883,"end_character":64},"updated":"2017-04-25 14:18:40.000000000","message":"I don\u0027t know if this is right. iirc, sibling_sets is a dict, with number of siblings as the keys and lists of tuples as the keys. Each tuple is a collection of sibling cores. We want to combine all of these cores into a flat list if we\u0027re using the PREFER policy because we don\u0027t necessarily care about core sibling relationships.\n\nI think what you\u0027re doing here is only taking cores that have no siblings. That\u0027s _not_ the behavior we want and we would have handled this already above (line 855 to 874)\n\nYou should look at the commit that actually added these lines to see if that makes sense","commit_id":"494c49aa834b15f09c982ed750c108b03c5cec71"},{"author":{"_account_id":8802,"name":"Vladik Romanovsky","email":"vromanso@redhat.com","username":"vladikr"},"change_message_id":"22f638d6291ddc9235aef0c2d7e72e9c76987854","unresolved":false,"context_lines":[{"line_number":880,"context_line":"                fields.CPUThreadAllocationPolicy.REQUIRE and"},{"line_number":881,"context_line":"                not pinning):"},{"line_number":882,"context_line":"            pinning \u003d list(zip(sorted(instance_cell.cpuset),"},{"line_number":883,"context_line":"                               itertools.chain(*sibling_sets[1])))"},{"line_number":884,"context_line":""},{"line_number":885,"context_line":"        threads_no \u003d _threads(instance_cell, threads_no)"},{"line_number":886,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5ff73747_12c1352f","line":883,"range":{"start_line":883,"start_character":31,"end_line":883,"end_character":64},"in_reply_to":"5ff73747_ea681388","updated":"2017-04-25 15:58:47.000000000","message":"I think the key in the siblings_set dict is something like \"what cpus can I get if my threads_per_core is X\"\n\nAccording to this example, it this code should work fine:\n sibling_sets: defaultdict(\u003ctype \u0027list\u0027\u003e, {1: [CoercedSet([0, 1]), CoercedSet([3])], 2: [CoercedSet([0, 1])]})","commit_id":"494c49aa834b15f09c982ed750c108b03c5cec71"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"0d50331919971b87eef68cf70a3f35d6f466e421","unresolved":false,"context_lines":[{"line_number":880,"context_line":"                fields.CPUThreadAllocationPolicy.REQUIRE and"},{"line_number":881,"context_line":"                not pinning):"},{"line_number":882,"context_line":"            pinning \u003d list(zip(sorted(instance_cell.cpuset),"},{"line_number":883,"context_line":"                               itertools.chain(*sibling_sets[1])))"},{"line_number":884,"context_line":""},{"line_number":885,"context_line":"        threads_no \u003d _threads(instance_cell, threads_no)"},{"line_number":886,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5ff73747_f44fd931","line":883,"in_reply_to":"5ff73747_ea681388","updated":"2017-04-25 15:46:02.000000000","message":"Not really if you look at the algorithm which defining that datastructure (~line700) you can see that: the key 1 will provide cores that have no siblings + cores which have siblings and basically even if all cores are providing siblings that key is going to be defined.\n\nsome examples:\nsinbling_sets \u003d {1: (0, 1), (2, 3), 2: (0, 1), (2, 3)]\nsinbling_sets \u003d {1: (0, 1), (3), 2: (0, 1)]\n\nThat is basically why we go thought that list in a reverse order and also why sibling_set used line 883 is equal to sibling_sets[1].","commit_id":"494c49aa834b15f09c982ed750c108b03c5cec71"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"97a3d1b0fbc6b926cbe42751e38ddd85c84ec1a5","unresolved":false,"context_lines":[{"line_number":883,"context_line":"                               itertools.chain(*sibling_sets[1])))"},{"line_number":884,"context_line":""},{"line_number":885,"context_line":"        threads_no \u003d _threads(instance_cell, threads_no)"},{"line_number":886,"context_line":""},{"line_number":887,"context_line":"    if not pinning:"},{"line_number":888,"context_line":"        return"},{"line_number":889,"context_line":"    LOG.debug(\u0027Selected cores for pinning: %s, in cell %s\u0027, pinning,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5ff73747_8a5cef7e","line":886,"updated":"2017-04-25 14:18:40.000000000","message":"I think we should log here to say we didn\u0027t get a sibling set. There\u0027s no way that this should be happening under normal circumstances and we should get some info about how often these race conditions happen","commit_id":"494c49aa834b15f09c982ed750c108b03c5cec71"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"1a19c89632dee985910cff79179af0ebd18967fd","unresolved":false,"context_lines":[{"line_number":702,"context_line":"        for threads_no in range(1, len(sib) + 1):"},{"line_number":703,"context_line":"            sibling_sets[threads_no].append(sib)"},{"line_number":704,"context_line":"    LOG.debug(\u0027Built sibling_sets: %(siblings)s\u0027, {\u0027siblings\u0027: sibling_sets})"},{"line_number":705,"context_line":""},{"line_number":706,"context_line":"    pinning \u003d None"},{"line_number":707,"context_line":"    threads_no \u003d 1"},{"line_number":708,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5ff73747_18d7f825","line":705,"updated":"2017-04-26 11:00:28.000000000","message":"Thinking on this more, there shouldn\u0027t be any use case where sibling_sets is uninitialized, right? However, due to races or whatnot it *is* happening so we need to handle that. I\u0027m thinking it we could fast exit here and move the logging from line 886 to this. That would totally mitigate any undefined variable issues and avoid us going through the rigmarole of logging what policy we\u0027re using etc.","commit_id":"6be38fbc253ba9924325882120c50af02128f573"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"dc75c3b18cce8781e0c80fb6f7d2319aaa044f6c","unresolved":false,"context_lines":[{"line_number":702,"context_line":"        for threads_no in range(1, len(sib) + 1):"},{"line_number":703,"context_line":"            sibling_sets[threads_no].append(sib)"},{"line_number":704,"context_line":"    LOG.debug(\u0027Built sibling_sets: %(siblings)s\u0027, {\u0027siblings\u0027: sibling_sets})"},{"line_number":705,"context_line":""},{"line_number":706,"context_line":"    pinning \u003d None"},{"line_number":707,"context_line":"    threads_no \u003d 1"},{"line_number":708,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5ff73747_cc3c2eca","line":705,"in_reply_to":"5ff73747_18d7f825","updated":"2017-04-26 13:00:17.000000000","message":"It\u0027s basically what\u0027s already done.\n\n if ..\n elif ..\n else\n   return","commit_id":"6be38fbc253ba9924325882120c50af02128f573"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"1a19c89632dee985910cff79179af0ebd18967fd","unresolved":false,"context_lines":[{"line_number":837,"context_line":"    if (instance_cell.cpu_thread_policy \u003d\u003d"},{"line_number":838,"context_line":"            fields.CPUThreadAllocationPolicy.ISOLATE):"},{"line_number":839,"context_line":"        # make sure we have at least one fully free core"},{"line_number":840,"context_line":"        if threads_per_core not in sibling_sets:"},{"line_number":841,"context_line":"            LOG.debug(\u0027Host does not have any fully free thread sibling sets.\u0027"},{"line_number":842,"context_line":"                      \u0027It is not possible to emulate a non-SMT behavior \u0027"},{"line_number":843,"context_line":"                      \u0027for the isolate policy without this.\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5ff73747_58ec60f2","line":840,"range":{"start_line":840,"start_character":35,"end_line":840,"end_character":47},"updated":"2017-04-26 11:00:28.000000000","message":"You also need to handle this, on the off chance that this race condition happens when using the ISOLATE policy. I think fast exiting above is a better solution though","commit_id":"6be38fbc253ba9924325882120c50af02128f573"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"dc75c3b18cce8781e0c80fb6f7d2319aaa044f6c","unresolved":false,"context_lines":[{"line_number":837,"context_line":"    if (instance_cell.cpu_thread_policy \u003d\u003d"},{"line_number":838,"context_line":"            fields.CPUThreadAllocationPolicy.ISOLATE):"},{"line_number":839,"context_line":"        # make sure we have at least one fully free core"},{"line_number":840,"context_line":"        if threads_per_core not in sibling_sets:"},{"line_number":841,"context_line":"            LOG.debug(\u0027Host does not have any fully free thread sibling sets.\u0027"},{"line_number":842,"context_line":"                      \u0027It is not possible to emulate a non-SMT behavior \u0027"},{"line_number":843,"context_line":"                      \u0027for the isolate policy without this.\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5ff73747_ec448a1d","line":840,"in_reply_to":"5ff73747_58ec60f2","updated":"2017-04-26 13:00:17.000000000","message":"Nothing to do here, if sibling_sets is empty the where are going to exist","commit_id":"6be38fbc253ba9924325882120c50af02128f573"},{"author":{"_account_id":8802,"name":"Vladik Romanovsky","email":"vromanso@redhat.com","username":"vladikr"},"change_message_id":"4354d62f3b4e96e362ca7a279da95792fa7aa097","unresolved":false,"context_lines":[{"line_number":837,"context_line":"    if (instance_cell.cpu_thread_policy \u003d\u003d"},{"line_number":838,"context_line":"            fields.CPUThreadAllocationPolicy.ISOLATE):"},{"line_number":839,"context_line":"        # make sure we have at least one fully free core"},{"line_number":840,"context_line":"        if threads_per_core not in sibling_sets:"},{"line_number":841,"context_line":"            LOG.debug(\u0027Host does not have any fully free thread sibling sets.\u0027"},{"line_number":842,"context_line":"                      \u0027It is not possible to emulate a non-SMT behavior \u0027"},{"line_number":843,"context_line":"                      \u0027for the isolate policy without this.\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5ff73747_51c8bb20","line":840,"range":{"start_line":840,"start_character":35,"end_line":840,"end_character":47},"in_reply_to":"5ff73747_58ec60f2","updated":"2017-04-26 12:46:04.000000000","message":"sibling_sets will always be initialized on line 700.","commit_id":"6be38fbc253ba9924325882120c50af02128f573"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"e497e3c80b0162028c094470dce0987a5a7c432b","unresolved":false,"context_lines":[{"line_number":837,"context_line":"    if (instance_cell.cpu_thread_policy \u003d\u003d"},{"line_number":838,"context_line":"            fields.CPUThreadAllocationPolicy.ISOLATE):"},{"line_number":839,"context_line":"        # make sure we have at least one fully free core"},{"line_number":840,"context_line":"        if threads_per_core not in sibling_sets:"},{"line_number":841,"context_line":"            LOG.debug(\u0027Host does not have any fully free thread sibling sets.\u0027"},{"line_number":842,"context_line":"                      \u0027It is not possible to emulate a non-SMT behavior \u0027"},{"line_number":843,"context_line":"                      \u0027for the isolate policy without this.\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5ff73747_87e72141","line":840,"in_reply_to":"5ff73747_ec448a1d","updated":"2017-04-26 13:17:42.000000000","message":"Aye, defaultdict to the rescue","commit_id":"6be38fbc253ba9924325882120c50af02128f573"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"1a19c89632dee985910cff79179af0ebd18967fd","unresolved":false,"context_lines":[{"line_number":880,"context_line":"                fields.CPUThreadAllocationPolicy.REQUIRE and"},{"line_number":881,"context_line":"                not pinning):"},{"line_number":882,"context_line":"            pinning \u003d list(zip(sorted(instance_cell.cpuset),"},{"line_number":883,"context_line":"                               itertools.chain(*sibling_sets[1])))"},{"line_number":884,"context_line":"        threads_no \u003d _threads(instance_cell, threads_no)"},{"line_number":885,"context_line":"    else:"},{"line_number":886,"context_line":"        LOG.warn(\"sibling_sets is empty, at this step that should never happen\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"5ff73747_45647922","line":883,"range":{"start_line":883,"start_character":59,"end_line":883,"end_character":60},"updated":"2017-04-26 11:00:28.000000000","message":"I only just spotted the extra \u0027s\u0027 here and had totally missed that in your commit message. Oops. Yeah, this makes perfect sense now.","commit_id":"6be38fbc253ba9924325882120c50af02128f573"}]}