{"thread":{"id":"46853","subject":"[Bug/Solution] Git hangs in compat/poll/poll.c on HPE NonStop","startedAt":"2017-09-28T21:21:05Z","lastAt":"2017-09-29T08:41:35Z","messageCount":3,"participants":["Randall S. Becker","Junio C Hamano","Paolo Bonzini"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"329124","messageId":"009c01d3389c$f6c0cb00$e4426100$@nexbridge.com","threadId":"46853","inReplyTo":null,"subject":"[Bug/Solution] Git hangs in compat/poll/poll.c on HPE NonStop","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2017-09-28T21:01:27Z","receivedAt":"2017-09-28T21:21:05Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"Hi Team,\n\nAfter a whole lot of investigating, we (it is a large \"we\") have discovered\nthe reason for the hang we occasionally get in git-upload-pack on HPE\nNonStop servers - reported here well over a year ago. This resulted from a\nsubtle check that the operating system does on file descriptors. When it\nsees random values in pfd[i].revents, it sometimes thinks its dealing with a\nTTY and well, things end badly after that. There is a non-destructive fix\nthat I would like to propose for this that I have already tested. Sadly, I\nhave no email mechanism where our repo resides for a real patch message. The\npatch is based on 2.3.7 (16018ae), but should be applicable forward. We have\nheld off moving to a more recent version until resolving this, so that's\nnext on our plan.\n\n--- a/compat/poll/poll.c\n+++ b/compat/poll/poll.c\n@@ -438,6 +438,10 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)\n pfd[i].revents = happened;\n rc++;\n }\n+ else\n+ {\n+ pfd[i].revents = 0;\n+ }\n }\n\n return rc;\n\nSincerely,\nRandall\n\n-- Brief whoami: NonStop&UNIX developer since approximately\nUNIX(421664400)/NonStop(211288444200000000) \n-- In my real life, I talk too much.\n\n"},{"id":"329134","messageId":"xmqqo9puzbii.fsf@gitster.mtv.corp.google.com","threadId":"46853","inReplyTo":"009c01d3389c$f6c0cb00$e4426100$@nexbridge.com","subject":"Re: [Bug/Solution] Git hangs in compat/poll/poll.c on HPE NonStop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-28T22:47:17Z","receivedAt":"2017-09-28T22:47:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"[jch: a patch that hopefully is applicable is attached at the end;\n I'd appreciate input from Paolo, the contributor of the original\n upstream]\n\n\"Randall S. Becker\" writes:\n\n> After a whole lot of investigating, we (it is a large \"we\") have discovered\n> the reason for the hang we occasionally get in git-upload-pack on HPE\n> NonStop servers - reported here well over a year ago. This resulted from a\n> subtle check that the operating system does on file descriptors. When it\n> sees random values in pfd[i].revents,...\n\nWhat do you mean by \"random values\"? Where do these random values\ncome from? The original code the patch touches is _not_ setting\nanything, so it is leaving it uninitialized and that is seen by the\nsystem? If that is the case perhaps should we clear it before we\ncall into this codepath?\n\n> .... There is a non-destructive fix\n> that I would like to propose for this that I have already tested.\n\nI am not sure in what sense this is \"non-destructive\"; we left the\nvalue as it was when we didn't notice anything happening in\ncompute_revents(). Now we explicitly destroy the value there was\n(just like we do in the case where the corresponding file descriptor\nwas negative). Maybe overwriting with 0 is the right thing, but I\nwouldn't call it \"non-destructive\", though. Puzzling...\n\n> --- a/compat/poll/poll.c\n> +++ b/compat/poll/poll.c\n> @@ -438,6 +438,10 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)\n> pfd[i].revents = happened;\n> rc++;\n> }\n> + else\n> + {\n> + pfd[i].revents = 0;\n> + }\n> }\n>\n> return rc;\n\nFYI, the upstream gnulib rewrites this part of the code with\nd42461c3 (\"poll: fixes for large fds\", 2015-02-20) quite a bit, and\nit reads like this:\n\n+ {\n+ pfd[i].revents = (pfd[i].fd < 0\n+ ? 0\n+ : compute_revents (pfd[i].fd, pfd[i].events,\n+ &rfds, &wfds, &efds));\n+ rc += pfd[i].revents != 0;\n+ }\n\nThe code before your fix (and before d42461c3) used to assign to\npfd[i].revents only when compute_revents() gave us non-zero value,\nbut with d42461c3 in the upstream, it pfd[i].revents is assigned\nthe return value from compute_revents() even if the value returned\nis 0. And rc is incremented only when that value is non-zero.\n\nThe result of applying your patch is equivalent, so after all, I\ntend to think that your patch is the right fix in the context of the\ncode we have (i.e. the older code we borrowed from them).\n\nI wonder if we want to refresh the borrowed copy of poll.c to a\nnewer snapshot someday, but that is totally a separate topic. At\nleast, I now know that your fix in this patch will not be broken due\nto d42461c3 updating the code in a slightly different way ;-)\n\nRandall, I forged your Sign-off in the patch below, but please say\nit is OK, after reading Documentation/SubmittingPatches.\n\nThanks.\n\n-- >8 --\nFrom: Randall S. Becker \nSubject: poll.c: always set revents, even if to zero.\n\nMatch what happens to pfd[i].revents when compute_revents() returns\n0 to the upstream gnulib's commit d42461c3 (\"poll: fixes for large\nfds\", 2015-02-20). The revents field is set to 0, without\nincrementing the value rc to be returned from the function.\n\nThis fixes occasional hangs in git-upload-pack on NPE NonStop.\n\nSigned-off-by: Randall S. Becker \nSigned-off-by: Junio C Hamano \n---\n compat/poll/poll.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/compat/poll/poll.c b/compat/poll/poll.c\nindex b10adc780f..ae03b74a6f 100644\n--- a/compat/poll/poll.c\n+++ b/compat/poll/poll.c\n@@ -438,6 +438,10 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)\n \t pfd[i].revents = happened;\n \t rc++;\n \t }\n+\telse\n+\t {\n+\t pfd[i].revents = 0;\n+\t }\n }\n \n return rc;\n"},{"id":"329164","messageId":"d934af29-9cee-ea74-40c8-e63f7592f8aa@redhat.com","threadId":"46853","inReplyTo":"xmqqo9puzbii.fsf@gitster.mtv.corp.google.com","subject":"Re: [Bug/Solution] Git hangs in compat/poll/poll.c on HPE NonStop","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2017-09-29T08:41:23Z","receivedAt":"2017-09-29T08:41:35Z","isPatch":false,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 29/09/2017 00:47, Junio C Hamano wrote:\n> [jch: a patch that hopefully is applicable is attached at the end;\n> I'd appreciate input from Paolo, the contributor of the original\n> upstream]\n\nI wasn't involved in upstream commit d42461c3, but Linux is also always\noverwriting the revents output with zero.\n\nReviewed-by: Paolo Bonzini \n\nPaolo\n\n> \"Randall S. Becker\" writes:\n> \n>> After a whole lot of investigating, we (it is a large \"we\") have discovered\n>> the reason for the hang we occasionally get in git-upload-pack on HPE\n>> NonStop servers - reported here well over a year ago. This resulted from a\n>> subtle check that the operating system does on file descriptors. When it\n>> sees random values in pfd[i].revents,...\n> \n> What do you mean by \"random values\"? Where do these random values\n> come from? The original code the patch touches is _not_ setting\n> anything, so it is leaving it uninitialized and that is seen by the\n> system? If that is the case perhaps should we clear it before we\n> call into this codepath?\n> \n>> .... There is a non-destructive fix\n>> that I would like to propose for this that I have already tested.\n> \n> I am not sure in what sense this is \"non-destructive\"; we left the\n> value as it was when we didn't notice anything happening in\n> compute_revents(). Now we explicitly destroy the value there was\n> (just like we do in the case where the corresponding file descriptor\n> was negative). Maybe overwriting with 0 is the right thing, but I\n> wouldn't call it \"non-destructive\", though. Puzzling...\n> \n>> --- a/compat/poll/poll.c\n>> +++ b/compat/poll/poll.c\n>> @@ -438,6 +438,10 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)\n>> pfd[i].revents = happened;\n>> rc++;\n>> }\n>> + else\n>> + {\n>> + pfd[i].revents = 0;\n>> + }\n>> }\n>>\n>> return rc;\n> \n> FYI, the upstream gnulib rewrites this part of the code with\n> d42461c3 (\"poll: fixes for large fds\", 2015-02-20) quite a bit, and\n> it reads like this:\n> \n> + {\n> + pfd[i].revents = (pfd[i].fd < 0\n> + ? 0\n> + : compute_revents (pfd[i].fd, pfd[i].events,\n> + &rfds, &wfds, &efds));\n> + rc += pfd[i].revents != 0;\n> + }\n> \n> The code before your fix (and before d42461c3) used to assign to\n> pfd[i].revents only when compute_revents() gave us non-zero value,\n> but with d42461c3 in the upstream, it pfd[i].revents is assigned\n> the return value from compute_revents() even if the value returned\n> is 0. And rc is incremented only when that value is non-zero.\n> \n> The result of applying your patch is equivalent, so after all, I\n> tend to think that your patch is the right fix in the context of the\n> code we have (i.e. the older code we borrowed from them).\n> \n> I wonder if we want to refresh the borrowed copy of poll.c to a\n> newer snapshot someday, but that is totally a separate topic. At\n> least, I now know that your fix in this patch will not be broken due\n> to d42461c3 updating the code in a slightly different way ;-)\n> \n> Randall, I forged your Sign-off in the patch below, but please say\n> it is OK, after reading Documentation/SubmittingPatches.\n> \n> Thanks.\n> \n> -- >8 --\n> From: Randall S. Becker \n> Subject: poll.c: always set revents, even if to zero.\n> \n> Match what happens to pfd[i].revents when compute_revents() returns\n> 0 to the upstream gnulib's commit d42461c3 (\"poll: fixes for large\n> fds\", 2015-02-20). The revents field is set to 0, without\n> incrementing the value rc to be returned from the function.\n> \n> This fixes occasional hangs in git-upload-pack on NPE NonStop.\n> \n> Signed-off-by: Randall S. Becker \n> Signed-off-by: Junio C Hamano \n> ---\n> compat/poll/poll.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/compat/poll/poll.c b/compat/poll/poll.c\n> index b10adc780f..ae03b74a6f 100644\n> --- a/compat/poll/poll.c\n> +++ b/compat/poll/poll.c\n> @@ -438,6 +438,10 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)\n> \t pfd[i].revents = happened;\n> \t rc++;\n> \t }\n> +\telse\n> +\t {\n> +\t pfd[i].revents = 0;\n> +\t }\n> }\n> \n> return rc;\n> \n\n"}]}