Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6719026:
Fix faulty renice: -ve priority numbers mean more scheduling priority. (Closed)
|
Created:
9 years, 9 months ago by gauravsh Modified:
9 years, 6 months ago CC:
chromium-os-reviews_chromium.org, sosa+cc_chromium.org, seano+cc_chromium.org, ericli Visibility:
Public. |
DescriptionFix faulty renice: -ve priority numbers mean more scheduling priority.
Get rid of re-nice completely since it is not needed.
BUG=chromium-os:12846
TEST=(man renice)
Change-Id: I664bfe643c0ad682605505de03abaf6eeb184985
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=43c7779
Patch Set 1 #Patch Set 2 : get rid of renice completely #Patch Set 3 : . #Messages
Total messages: 9 (0 generated)
If we've been nicing in the wrong direction, then perhaps this code isn't actually doing what was intended and should just be removed? On Sun, Mar 27, 2011 at 5:43 PM, <gauravsh@chromium.org> wrote: > Reviewers: dalec, Chris Masone, > > Description: > Fix faulty renice: -ve priority numbers mean more scheduling priority. > > BUG=chromium-os:12846 > TEST=(man renice) > > Change-Id: I664bfe643c0ad682605505de03abaf6eeb184985 > > Please review this at http://codereview.chromium.org/6719026/ > > SVN Base: ssh://git@gitrw.chromium.org:9222/autotest.git@master > > Affected files: > M client/cros/login.py > > > Index: client/cros/login.py > diff --git a/client/cros/login.py b/client/cros/login.py > index > 8cc7fd95781ef284e94952c0adaa0f8c632af2b1..a25b6148a10a3112a6304966962501fe74be11e7 > 100644 > --- a/client/cros/login.py > +++ b/client/cros/login.py > @@ -153,7 +153,7 @@ def attempt_login(username, password, > timeout=_DEFAULT_TIMEOUT): > > # Up our priority so we don't get descheduled in the middle of sending > key > # press and key release events. > - utils.system('renice +%d -p %d' % (_LOGIN_NICE, os.getpid())) > + utils.system('renice -%d -p %d' % (_LOGIN_NICE, os.getpid())) > try: > ax = cros_ui.get_autox() > # navigate to login screen > @@ -172,7 +172,7 @@ def attempt_login(username, password, > timeout=_DEFAULT_TIMEOUT): > else: > ax.send_hotkey("Alt+B") # Browse without signing-in > finally: > - utils.system('renice -%d -p %d' % (_LOGIN_NICE, os.getpid())) > + utils.system('renice +%d -p %d' % (_LOGIN_NICE, os.getpid())) > > wait_for_condition(condition=logged_in, > timeout_msg='Timed out waiting for login', > > >
I will let Dale comment on why this was added in the first place. But yeah, if re-nicing to the wrong extreme didn't hurt things, normal process priority should be sufficient. On Sun, Mar 27, 2011 at 7:00 PM, Chris Masone <cmasone@chromium.org> wrote: > If we've been nicing in the wrong direction, then perhaps this code isn't > actually doing what was intended and should just be removed? > > On Sun, Mar 27, 2011 at 5:43 PM, <gauravsh@chromium.org> wrote: >> >> Reviewers: dalec, Chris Masone, >> >> Description: >> Fix faulty renice: -ve priority numbers mean more scheduling priority. >> >> BUG=chromium-os:12846 >> TEST=(man renice) >> >> Change-Id: I664bfe643c0ad682605505de03abaf6eeb184985 >> >> Please review this at http://codereview.chromium.org/6719026/ >> >> SVN Base: ssh://git@gitrw.chromium.org:9222/autotest.git@master >> >> Affected files: >> M client/cros/login.py >> >> >> Index: client/cros/login.py >> diff --git a/client/cros/login.py b/client/cros/login.py >> index >> 8cc7fd95781ef284e94952c0adaa0f8c632af2b1..a25b6148a10a3112a6304966962501fe74be11e7 >> 100644 >> --- a/client/cros/login.py >> +++ b/client/cros/login.py >> @@ -153,7 +153,7 @@ def attempt_login(username, password, >> timeout=_DEFAULT_TIMEOUT): >> >> # Up our priority so we don't get descheduled in the middle of sending >> key >> # press and key release events. >> - utils.system('renice +%d -p %d' % (_LOGIN_NICE, os.getpid())) >> + utils.system('renice -%d -p %d' % (_LOGIN_NICE, os.getpid())) >> try: >> ax = cros_ui.get_autox() >> # navigate to login screen >> @@ -172,7 +172,7 @@ def attempt_login(username, password, >> timeout=_DEFAULT_TIMEOUT): >> else: >> ax.send_hotkey("Alt+B") # Browse without signing-in >> finally: >> - utils.system('renice -%d -p %d' % (_LOGIN_NICE, os.getpid())) >> + utils.system('renice +%d -p %d' % (_LOGIN_NICE, os.getpid())) >> >> wait_for_condition(condition=logged_in, >> timeout_msg='Timed out waiting for login', >> >> > > -- -g
Whoops. The code definitely fixes the interrupted key presses however; I ran a lot of A/B testing with a faulty image. I imagine it still works since the system runs max priority (+20) only when nothing else wants to run. Which likely indicates the original problem was indeed one of interruption. I don't think you need a full -20 here. If it's worth the time, I would experiment with -1, -5, -10, etc. to see if that's sufficient. In any case, you'll need to re-run the login_LoginSuccess test at least 20 or 30 times to make this works properly. On 2011/03/28 02:32:33, gauravsh wrote: > I will let Dale comment on why this was added in the first place. But > yeah, if re-nicing to the wrong extreme didn't hurt things, normal > process priority should be sufficient. > > On Sun, Mar 27, 2011 at 7:00 PM, Chris Masone <mailto:cmasone@chromium.org> wrote: > > If we've been nicing in the wrong direction, then perhaps this code isn't > > actually doing what was intended and should just be removed? > > > > On Sun, Mar 27, 2011 at 5:43 PM, <mailto:gauravsh@chromium.org> wrote: > >> > >> Reviewers: dalec, Chris Masone, > >> > >> Description: > >> Fix faulty renice: -ve priority numbers mean more scheduling priority. > >> > >> BUG=chromium-os:12846 > >> TEST=(man renice) > >> > >> Change-Id: I664bfe643c0ad682605505de03abaf6eeb184985 > >> > >> Please review this at http://codereview.chromium.org/6719026/ > >> > >> SVN Base: ssh://git@gitrw.chromium.org:9222/autotest.git@master > >> > >> Affected files: > >> M client/cros/login.py > >> > >> > >> Index: client/cros/login.py > >> diff --git a/client/cros/login.py b/client/cros/login.py > >> index > >> > 8cc7fd95781ef284e94952c0adaa0f8c632af2b1..a25b6148a10a3112a6304966962501fe74be11e7 > >> 100644 > >> --- a/client/cros/login.py > >> +++ b/client/cros/login.py > >> @@ -153,7 +153,7 @@ def attempt_login(username, password, > >> timeout=_DEFAULT_TIMEOUT): > >> > >> # Up our priority so we don't get descheduled in the middle of sending > >> key > >> # press and key release events. > >> - utils.system('renice +%d -p %d' % (_LOGIN_NICE, os.getpid())) > >> + utils.system('renice -%d -p %d' % (_LOGIN_NICE, os.getpid())) > >> try: > >> ax = cros_ui.get_autox() > >> # navigate to login screen > >> @@ -172,7 +172,7 @@ def attempt_login(username, password, > >> timeout=_DEFAULT_TIMEOUT): > >> else: > >> ax.send_hotkey("Alt+B") # Browse without signing-in > >> finally: > >> - utils.system('renice -%d -p %d' % (_LOGIN_NICE, os.getpid())) > >> + utils.system('renice +%d -p %d' % (_LOGIN_NICE, os.getpid())) > >> > >> wait_for_condition(condition=logged_in, > >> timeout_msg='Timed out waiting for login', > >> > >> > > > > > > > > -- > -g
On Mon, Mar 28, 2011 at 8:52 AM, <dalecurtis@chromium.org> wrote: > Whoops. The code definitely fixes the interrupted key presses however; I ran > a > lot of A/B testing with a faulty image. I imagine it still works since the > system runs max priority (+20) only when nothing else wants to run. Which > likely > indicates the original problem was indeed one of interruption. I don't think that changing the priority to effectively be the minimum possible would help things at all. A process running at a nice value of 20 is more likely to be interrupted. Tt will start running when nothing else wants to run but that doesn't mean something else would want to run very soon (and there is always something). In any case, I ran 20 iterations of login_LoginSuccess on a Cr-48 with the following results: renice to -20: All iterations PASS. Time taken = 22m renice to 0 (aka no-op): All iterations PASS. Time taken = 23m51s no renice: 25m25s I think the variation is just noise and given this, I'd vote for removing the renice completely unless there is a compelling reason not to. I have uploaded a new patchset that does so. PTAL. > > I don't think you need a full -20 here. If it's worth the time, I would > experiment with -1, -5, -10, etc. to see if that's sufficient. > > In any case, you'll need to re-run the login_LoginSuccess test at least 20 > or 30 > times to make this works properly. > > On 2011/03/28 02:32:33, gauravsh wrote: >> >> I will let Dale comment on why this was added in the first place. But >> yeah, if re-nicing to the wrong extreme didn't hurt things, normal >> process priority should be sufficient. > >> On Sun, Mar 27, 2011 at 7:00 PM, Chris Masone >> <mailto:cmasone@chromium.org> > > wrote: >> >> > If we've been nicing in the wrong direction, then perhaps this code >> > isn't >> > actually doing what was intended and should just be removed? >> > >> > On Sun, Mar 27, 2011 at 5:43 PM, <mailto:gauravsh@chromium.org> wrote: >> >> >> >> Reviewers: dalec, Chris Masone, >> >> >> >> Description: >> >> Fix faulty renice: -ve priority numbers mean more scheduling priority. >> >> >> >> BUG=chromium-os:12846 >> >> TEST=(man renice) >> >> >> >> Change-Id: I664bfe643c0ad682605505de03abaf6eeb184985 >> >> >> >> Please review this at http://codereview.chromium.org/6719026/ >> >> >> >> SVN Base: ssh://git@gitrw.chromium.org:9222/autotest.git@master >> >> >> >> Affected files: >> >> M client/cros/login.py >> >> >> >> >> >> Index: client/cros/login.py >> >> diff --git a/client/cros/login.py b/client/cros/login.py >> >> index >> >> > > 8cc7fd95781ef284e94952c0adaa0f8c632af2b1..a25b6148a10a3112a6304966962501fe74be11e7 >> >> >> 100644 >> >> --- a/client/cros/login.py >> >> +++ b/client/cros/login.py >> >> @@ -153,7 +153,7 @@ def attempt_login(username, password, >> >> timeout=_DEFAULT_TIMEOUT): >> >> >> >> # Up our priority so we don't get descheduled in the >> >> middle > > of sending >> >> >> key >> >> # press and key release events. >> >> - utils.system('renice +%d -p %d' % (_LOGIN_NICE, > > os.getpid())) >> >> >> + utils.system('renice -%d -p %d' % (_LOGIN_NICE, > > os.getpid())) >> >> >> try: >> >> ax = cros_ui.get_autox() >> >> # navigate to login screen >> >> @@ -172,7 +172,7 @@ def attempt_login(username, password, >> >> timeout=_DEFAULT_TIMEOUT): >> >> else: >> >> ax.send_hotkey("Alt+B") >> >> # > > Browse without signing-in >> >> >> finally: >> >> - utils.system('renice -%d -p %d' % > > (_LOGIN_NICE, os.getpid())) >> >> >> + utils.system('renice +%d -p %d' % > > (_LOGIN_NICE, os.getpid())) >> >> >> >> >> wait_for_condition(condition=logged_in, >> >> > > timeout_msg='Timed out waiting for login', >> >> >> >> >> >> > >> > > > > >> -- >> -g > > > > http://codereview.chromium.org/6719026/ > -- -g
I'm hesitant to remove it completely as we dealt with the repeated keystrokes problem for weeks before fixing it with this change... though your testing seems to indicate the issue is no longer present. Can you run one last test? Take an R12 test image from go/chromeos-images and run the login_LoginSuccess test against it another 20 times. I want to make sure there isn't a discrepancy between your dev build and an official test image. I'll also spin a cycle up on my end with your patch. On 2011/03/28 21:27:54, gauravsh wrote: > On Mon, Mar 28, 2011 at 8:52 AM, <mailto:dalecurtis@chromium.org> wrote: > > Whoops. The code definitely fixes the interrupted key presses however; I ran > > a > > lot of A/B testing with a faulty image. I imagine it still works since the > > system runs max priority (+20) only when nothing else wants to run. Which > > likely > > indicates the original problem was indeed one of interruption. > > I don't think that changing the priority to effectively be the minimum > possible would help things at all. A process running at a nice value > of 20 is more likely to be interrupted. Tt will start running when > nothing else wants to run but that doesn't mean something else would > want to run very soon (and there is always something). > > In any case, I ran 20 iterations of login_LoginSuccess on a Cr-48 with > the following results: > > renice to -20: All iterations PASS. Time taken = 22m > renice to 0 (aka no-op): All iterations PASS. Time taken = 23m51s > no renice: 25m25s > > I think the variation is just noise and given this, I'd vote for > removing the renice completely unless there is a compelling reason not > to. I have uploaded a new patchset that does so. PTAL. > >
On Mon, Mar 28, 2011 at 2:39 PM, <dalecurtis@chromium.org> wrote: > I'm hesitant to remove it completely as we dealt with the repeated > keystrokes > problem for weeks before fixing it with this change... though your testing > seems > to indicate the issue is no longer present. > > Can you run one last test? Take an R12 test image from go/chromeos-images > and I am actually using an official 0.12.313 test image for this. > run the login_LoginSuccess test against it another 20 times. I want to make > sure > there isn't a discrepancy between your dev build and an official test image. > I'll also spin a cycle up on my end with your patch. > > On 2011/03/28 21:27:54, gauravsh wrote: >> >> On Mon, Mar 28, 2011 at 8:52 AM, <mailto:dalecurtis@chromium.org> wrote: >> > Whoops. The code definitely fixes the interrupted key presses however; I >> > ran >> > a >> > lot of A/B testing with a faulty image. I imagine it still works since >> > the >> > system runs max priority (+20) only when nothing else wants to run. >> > Which >> > likely >> > indicates the original problem was indeed one of interruption. > >> I don't think that changing the priority to effectively be the minimum >> possible would help things at all. A process running at a nice value >> of 20 is more likely to be interrupted. Tt will start running when >> nothing else wants to run but that doesn't mean something else would >> want to run very soon (and there is always something). > >> In any case, I ran 20 iterations of login_LoginSuccess on a Cr-48 with >> the following results: > >> renice to -20: All iterations PASS. Time taken = 22m >> renice to 0 (aka no-op): All iterations PASS. Time taken = 23m51s >> no renice: 25m25s > >> I think the variation is just noise and given this, I'd vote for >> removing the renice completely unless there is a compelling reason not >> to. I have uploaded a new patchset that does so. PTAL. > > > > > http://codereview.chromium.org/6719026/ > -- -g
Okay, I will just cycle in the lab for 20 iterations and okay the change if it passes. On 2011/03/28 21:43:19, gauravsh wrote: > On Mon, Mar 28, 2011 at 2:39 PM, <mailto:dalecurtis@chromium.org> wrote: > > I'm hesitant to remove it completely as we dealt with the repeated > > keystrokes > > problem for weeks before fixing it with this change... though your testing > > seems > > to indicate the issue is no longer present. > > > > Can you run one last test? Take an R12 test image from go/chromeos-images > > and > > I am actually using an official 0.12.313 test image for this. > > > run the login_LoginSuccess test against it another 20 times. I want to make > > sure > > there isn't a discrepancy between your dev build and an official test image. > > I'll also spin a cycle up on my end with your patch. > > > > On 2011/03/28 21:27:54, gauravsh wrote: > >> > >> On Mon, Mar 28, 2011 at 8:52 AM, <mailto:dalecurtis@chromium.org> wrote: > >> > Whoops. The code definitely fixes the interrupted key presses however; I > >> > ran > >> > a > >> > lot of A/B testing with a faulty image. I imagine it still works since > >> > the > >> > system runs max priority (+20) only when nothing else wants to run. > >> > Which > >> > likely > >> > indicates the original problem was indeed one of interruption. > > > >> I don't think that changing the priority to effectively be the minimum > >> possible would help things at all. A process running at a nice value > >> of 20 is more likely to be interrupted. Tt will start running when > >> nothing else wants to run but that doesn't mean something else would > >> want to run very soon (and there is always something). > > > >> In any case, I ran 20 iterations of login_LoginSuccess on a Cr-48 with > >> the following results: > > > >> renice to -20: All iterations PASS. Time taken = 22m > >> renice to 0 (aka no-op): All iterations PASS. Time taken = 23m51s > >> no renice: 25m25s > > > >> I think the variation is just noise and given this, I'd vote for > >> removing the renice completely unless there is a compelling reason not > >> to. I have uploaded a new patchset that does so. PTAL. > > > > > > > > > > http://codereview.chromium.org/6719026/ > > > > > > -- > -g
LGTM. 20 runs in the lab without failure and no nice. I wish I knew what was fixed, but the code looks unnecessary right now. On 2011/03/28 21:45:37, dalec wrote: > Okay, I will just cycle in the lab for 20 iterations and okay the change if it > passes. > > On 2011/03/28 21:43:19, gauravsh wrote: > > On Mon, Mar 28, 2011 at 2:39 PM, <mailto:dalecurtis@chromium.org> wrote: > > > I'm hesitant to remove it completely as we dealt with the repeated > > > keystrokes > > > problem for weeks before fixing it with this change... though your testing > > > seems > > > to indicate the issue is no longer present. > > > > > > Can you run one last test? Take an R12 test image from go/chromeos-images > > > and > > > > I am actually using an official 0.12.313 test image for this. > > > > > run the login_LoginSuccess test against it another 20 times. I want to make > > > sure > > > there isn't a discrepancy between your dev build and an official test image. > > > I'll also spin a cycle up on my end with your patch. > > > > > > On 2011/03/28 21:27:54, gauravsh wrote: > > >> > > >> On Mon, Mar 28, 2011 at 8:52 AM, <mailto:dalecurtis@chromium.org> > wrote: > > >> > Whoops. The code definitely fixes the interrupted key presses however; I > > >> > ran > > >> > a > > >> > lot of A/B testing with a faulty image. I imagine it still works since > > >> > the > > >> > system runs max priority (+20) only when nothing else wants to run. > > >> > Which > > >> > likely > > >> > indicates the original problem was indeed one of interruption. > > > > > >> I don't think that changing the priority to effectively be the minimum > > >> possible would help things at all. A process running at a nice value > > >> of 20 is more likely to be interrupted. Tt will start running when > > >> nothing else wants to run but that doesn't mean something else would > > >> want to run very soon (and there is always something). > > > > > >> In any case, I ran 20 iterations of login_LoginSuccess on a Cr-48 with > > >> the following results: > > > > > >> renice to -20: All iterations PASS. Time taken = 22m > > >> renice to 0 (aka no-op): All iterations PASS. Time taken = 23m51s > > >> no renice: 25m25s > > > > > >> I think the variation is just noise and given this, I'd vote for > > >> removing the renice completely unless there is a compelling reason not > > >> to. I have uploaded a new patchset that does so. PTAL. > > > > > > > > > > > > > > > http://codereview.chromium.org/6719026/ > > > > > > > > > > > -- > > -g |
