Re: Throttle::wait use case clarification
Merged, thanks a bunch for the contributions. :) -Greg On Tue, Feb 5, 2013 at 11:10 AM, Loic Dachary wrote: > Hi, > > Here is a new pull request containing the patch and the associated unit > tests, all together. Thanks a lot for reviewing them :-) > > https://github.com/ceph/ceph/pull/39 > > Cheers > > On 02/05/2013 01:22 AM, Gregory Farnum wrote: >> Loic, >> Sorry for the delay in getting back to you about these patches. :( I >> finally got some time to look over them, and in general it's all good! >> I do have some comments, though. >> >> On Mon, Jan 21, 2013 at 5:44 AM, Loic Dachary wrote: Looking through the history of that test (in _reset_max), I think it's an accident and we actually want to be waking up the front if the maximum increases (or possibly in all cases, in case the front is a very large request we're going to let through anyway). Want to submit a patch? :) >>> :-) Here it is. "make check" does not complain. I've not run teuthology + >>> qa-suite though. I figured out how to run teuthology but did not yet try >>> qa-suite. >>> >>> http://marc.info/?l=ceph-devel&m=135877502606311&w=4 >> >> This patch to reverse the conditional is obviously fine. >> The other possibility I was trying to investigate is that it had something to do with handling get() requests larger than the max correctly, but I can't find any evidence of that one... >>> I've run the Throttle unit tests after uncommenting >>> https://github.com/ceph/ceph/pull/34/files#L3R269 >>> and commenting out >>> https://github.com/ceph/ceph/pull/34/files#L3R266 >>> and it passes. >> >> Regarding these unit tests, I have a few questions which I left on >> Github. Can you address them and then give a single pull request which >> includes both the Throttle fix and the tests? :) >> Thanks! >> -Greg >> -- >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Loïc Dachary, Artisan Logiciel Libre > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Throttle::wait use case clarification
Hi, Here is a new pull request containing the patch and the associated unit tests, all together. Thanks a lot for reviewing them :-) https://github.com/ceph/ceph/pull/39 Cheers On 02/05/2013 01:22 AM, Gregory Farnum wrote: > Loic, > Sorry for the delay in getting back to you about these patches. :( I > finally got some time to look over them, and in general it's all good! > I do have some comments, though. > > On Mon, Jan 21, 2013 at 5:44 AM, Loic Dachary wrote: >>> Looking through the history of that test (in _reset_max), I think it's an >>> accident and we actually want to be waking up the front if the maximum >>> increases (or possibly in all cases, in case the front is a very large >>> request we're going to let through anyway). Want to submit a patch? :) >> :-) Here it is. "make check" does not complain. I've not run teuthology + >> qa-suite though. I figured out how to run teuthology but did not yet try >> qa-suite. >> >> http://marc.info/?l=ceph-devel&m=135877502606311&w=4 > > This patch to reverse the conditional is obviously fine. > >>> The other possibility I was trying to investigate is that it had something >>> to do with handling get() requests larger than the max correctly, but I >>> can't find any evidence of that one... >> I've run the Throttle unit tests after uncommenting >> https://github.com/ceph/ceph/pull/34/files#L3R269 >> and commenting out >> https://github.com/ceph/ceph/pull/34/files#L3R266 >> and it passes. > > Regarding these unit tests, I have a few questions which I left on > Github. Can you address them and then give a single pull request which > includes both the Throttle fix and the tests? :) > Thanks! > -Greg > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Loïc Dachary, Artisan Logiciel Libre signature.asc Description: OpenPGP digital signature
Re: Throttle::wait use case clarification
On 02/05/2013 01:22 AM, Gregory Farnum wrote: > Loic, > Sorry for the delay in getting back to you about these patches. :( I > finally got some time to look over them, and in general it's all good! > I do have some comments, though. > > On Mon, Jan 21, 2013 at 5:44 AM, Loic Dachary wrote: >>> Looking through the history of that test (in _reset_max), I think it's an >>> accident and we actually want to be waking up the front if the maximum >>> increases (or possibly in all cases, in case the front is a very large >>> request we're going to let through anyway). Want to submit a patch? :) >> :-) Here it is. "make check" does not complain. I've not run teuthology + >> qa-suite though. I figured out how to run teuthology but did not yet try >> qa-suite. >> >> http://marc.info/?l=ceph-devel&m=135877502606311&w=4 > > This patch to reverse the conditional is obviously fine. > >>> The other possibility I was trying to investigate is that it had something >>> to do with handling get() requests larger than the max correctly, but I >>> can't find any evidence of that one... >> I've run the Throttle unit tests after uncommenting >> https://github.com/ceph/ceph/pull/34/files#L3R269 >> and commenting out >> https://github.com/ceph/ceph/pull/34/files#L3R266 >> and it passes. > > Regarding these unit tests, I have a few questions which I left on > Github. Can you address them and then give a single pull request which > includes both the Throttle fix and the tests? :) I will, thanks :-) -- Loïc Dachary, Artisan Logiciel Libre signature.asc Description: OpenPGP digital signature
Re: Throttle::wait use case clarification
Loic, Sorry for the delay in getting back to you about these patches. :( I finally got some time to look over them, and in general it's all good! I do have some comments, though. On Mon, Jan 21, 2013 at 5:44 AM, Loic Dachary wrote: >> Looking through the history of that test (in _reset_max), I think it's an >> accident and we actually want to be waking up the front if the maximum >> increases (or possibly in all cases, in case the front is a very large >> request we're going to let through anyway). Want to submit a patch? :) > :-) Here it is. "make check" does not complain. I've not run teuthology + > qa-suite though. I figured out how to run teuthology but did not yet try > qa-suite. > > http://marc.info/?l=ceph-devel&m=135877502606311&w=4 This patch to reverse the conditional is obviously fine. >> The other possibility I was trying to investigate is that it had something >> to do with handling get() requests larger than the max correctly, but I >> can't find any evidence of that one... > I've run the Throttle unit tests after uncommenting > https://github.com/ceph/ceph/pull/34/files#L3R269 > and commenting out > https://github.com/ceph/ceph/pull/34/files#L3R266 > and it passes. Regarding these unit tests, I have a few questions which I left on Github. Can you address them and then give a single pull request which includes both the Throttle fix and the tests? :) Thanks! -Greg -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Throttle::wait use case clarification
On Monday, January 21, 2013 at 5:44 AM, Loic Dachary wrote:
>
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> On 01/21/2013 12:02 AM, Gregory Farnum wrote:
> > On Sunday, January 20, 2013 at 5:39 AM, Loic Dachary wrote:
> > > Hi,
> > >
> > > While working on unit tests for Throttle.{cc,h} I tried to figure out a
> > > use case related to the Throttle::wait method but couldn't
> > >
> > > https://github.com/ceph/ceph/pull/34/files#L3R258
> > >
> > > Although it was not a blocker and I managed to reach 100% coverage
> > > anyway, it got me curious and I would very much appreciate pointers to
> > > understand the rationale.
> > >
> > > wait() can be called to set a new maximum before waiting for all pending
> > > threads to get get what they asked for. Since the maximum has changed,
> > > wait() wakes up the first thread : the conditions under which it decided
> > > to go to sleep have changed and the conclusion may be different.
> > >
> > > However, it only does so when the new maximum is less than current one.
> > > For instance
> > >
> > > A) decision does not change
> > >
> > > max = 10, current 9
> > > thread 1 tries to get 5 but only 1 is available, it goes to sleep
> > > wait(8)
> > > max = 8, current 9
> > > wakes up thread 1
> > > thread 1 tries to get 5 but current is already beyond the maximum, it
> > > goes to sleep
> > >
> > > B) decision changes
> > >
> > > max = 10, current 1
> > > thread 1 tries to get 10 but only 9 is available, it goes to sleep
> > > wait(9)
> > > max = 9, current 1
> > > wakes up thread 1
> > > thread 1 tries to get 10 which is above the maximum : it succeeds because
> > > current is below the new maximum
> > >
> > > It will not wake up a thread if the maximum increases, for instance:
> > >
> > > max = 10, current 9
> > > thread 1 tries to get 5 but only 1 is available, it goes to sleep
> > > wait(20)
> > > max = 20, current 9
> > > does *not* wake up thread 1
> > > keeps waiting until another thread put(N) with N >= 0 although there now
> > > is 11 available and it would allow it to get 5 out of it
> > >
> > > Why is it not desirable for thread 1 to wake up in this case ? When
> > > debugging a real world situation, I think it would show as a thread
> > > blocked although the throttle it is waiting on has enough to satisfy its
> > > request. What am I missing ?
> > >
> > > Cheers
> > >
> > >
> > > Attachments:
> > > - loic.vcf
> >
> >
> >
> >
> > Looking through the history of that test (in _reset_max), I think it's an
> > accident and we actually want to be waking up the front if the maximum
> > increases (or possibly in all cases, in case the front is a very large
> > request we're going to let through anyway). Want to submit a patch? :)
> :-) Here it is. "make check" does not complain. I've not run teuthology +
> qa-suite though. I figured out how to run teuthology but did not yet try
> qa-suite.
>
> http://marc.info/?l=ceph-devel&m=135877502606311&w=4
>
> >
> > The other possibility I was trying to investigate is that it had something
> > to do with handling get() requests larger than the max correctly, but I
> > can't find any evidence of that one...
> I've run the Throttle unit tests after uncommenting
> https://github.com/ceph/ceph/pull/34/files#L3R269
> and commenting out
> https://github.com/ceph/ceph/pull/34/files#L3R266
> and it passes.
>
> I'm not sure if I should have posted the proposed Throttle unit test to the
> list instead of proposing it as a pull request
> https://github.com/ceph/ceph/pull/34
>
> What is best ?
Pull requests are good; you just sent it in on a weekend and we've all got a
queue before we evaluate code pulls. :)
Thanks!
-Greg
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Throttle::wait use case clarification
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 01/21/2013 12:02 AM, Gregory Farnum wrote:
> On Sunday, January 20, 2013 at 5:39 AM, Loic Dachary wrote:
>> Hi,
>>
>> While working on unit tests for Throttle.{cc,h} I tried to figure out a use
>> case related to the Throttle::wait method but couldn't
>>
>> https://github.com/ceph/ceph/pull/34/files#L3R258
>>
>> Although it was not a blocker and I managed to reach 100% coverage anyway,
>> it got me curious and I would very much appreciate pointers to understand
>> the rationale.
>>
>> wait() can be called to set a new maximum before waiting for all pending
>> threads to get get what they asked for. Since the maximum has changed,
>> wait() wakes up the first thread : the conditions under which it decided to
>> go to sleep have changed and the conclusion may be different.
>>
>> However, it only does so when the new maximum is less than current one. For
>> instance
>>
>> A) decision does not change
>>
>> max = 10, current 9
>> thread 1 tries to get 5 but only 1 is available, it goes to sleep
>> wait(8)
>> max = 8, current 9
>> wakes up thread 1
>> thread 1 tries to get 5 but current is already beyond the maximum, it goes
>> to sleep
>>
>> B) decision changes
>>
>> max = 10, current 1
>> thread 1 tries to get 10 but only 9 is available, it goes to sleep
>> wait(9)
>> max = 9, current 1
>> wakes up thread 1
>> thread 1 tries to get 10 which is above the maximum : it succeeds because
>> current is below the new maximum
>>
>> It will not wake up a thread if the maximum increases, for instance:
>>
>> max = 10, current 9
>> thread 1 tries to get 5 but only 1 is available, it goes to sleep
>> wait(20)
>> max = 20, current 9
>> does *not* wake up thread 1
>> keeps waiting until another thread put(N) with N >= 0 although there now is
>> 11 available and it would allow it to get 5 out of it
>>
>> Why is it not desirable for thread 1 to wake up in this case ? When
>> debugging a real world situation, I think it would show as a thread blocked
>> although the throttle it is waiting on has enough to satisfy its request.
>> What am I missing ?
>>
>> Cheers
>>
>>
>> Attachments:
>> - loic.vcf
>>
>
>
> Looking through the history of that test (in _reset_max), I think it's an
> accident and we actually want to be waking up the front if the maximum
> increases (or possibly in all cases, in case the front is a very large
> request we're going to let through anyway). Want to submit a patch? :)
:-) Here it is. "make check" does not complain. I've not run teuthology +
qa-suite though. I figured out how to run teuthology but did not yet try
qa-suite.
http://marc.info/?l=ceph-devel&m=135877502606311&w=4
>
> The other possibility I was trying to investigate is that it had something to
> do with handling get() requests larger than the max correctly, but I can't
> find any evidence of that one...
I've run the Throttle unit tests after uncommenting
https://github.com/ceph/ceph/pull/34/files#L3R269
and commenting out
https://github.com/ceph/ceph/pull/34/files#L3R266
and it passes.
I'm not sure if I should have posted the proposed Throttle unit test to the
list instead of proposing it as a pull request
https://github.com/ceph/ceph/pull/34
What is best ?
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAlD9RjUACgkQ8dLMyEl6F20boACggzHH3Dw+/kM+awkD5POxyQB4
WosAn02bfzTUnItoTlwKtU0cDlWnckGv
=SsIe
-END PGP SIGNATURE-
<>
Re: Throttle::wait use case clarification
On Sunday, January 20, 2013 at 5:39 AM, Loic Dachary wrote:
> Hi,
>
> While working on unit tests for Throttle.{cc,h} I tried to figure out a use
> case related to the Throttle::wait method but couldn't
>
> https://github.com/ceph/ceph/pull/34/files#L3R258
>
> Although it was not a blocker and I managed to reach 100% coverage anyway, it
> got me curious and I would very much appreciate pointers to understand the
> rationale.
>
> wait() can be called to set a new maximum before waiting for all pending
> threads to get get what they asked for. Since the maximum has changed, wait()
> wakes up the first thread : the conditions under which it decided to go to
> sleep have changed and the conclusion may be different.
>
> However, it only does so when the new maximum is less than current one. For
> instance
>
> A) decision does not change
>
> max = 10, current 9
> thread 1 tries to get 5 but only 1 is available, it goes to sleep
> wait(8)
> max = 8, current 9
> wakes up thread 1
> thread 1 tries to get 5 but current is already beyond the maximum, it goes to
> sleep
>
> B) decision changes
>
> max = 10, current 1
> thread 1 tries to get 10 but only 9 is available, it goes to sleep
> wait(9)
> max = 9, current 1
> wakes up thread 1
> thread 1 tries to get 10 which is above the maximum : it succeeds because
> current is below the new maximum
>
> It will not wake up a thread if the maximum increases, for instance:
>
> max = 10, current 9
> thread 1 tries to get 5 but only 1 is available, it goes to sleep
> wait(20)
> max = 20, current 9
> does *not* wake up thread 1
> keeps waiting until another thread put(N) with N >= 0 although there now is
> 11 available and it would allow it to get 5 out of it
>
> Why is it not desirable for thread 1 to wake up in this case ? When debugging
> a real world situation, I think it would show as a thread blocked although
> the throttle it is waiting on has enough to satisfy its request. What am I
> missing ?
>
> Cheers
>
>
> Attachments:
> - loic.vcf
>
Looking through the history of that test (in _reset_max), I think it's an
accident and we actually want to be waking up the front if the maximum
increases (or possibly in all cases, in case the front is a very large request
we're going to let through anyway). Want to submit a patch? :)
The other possibility I was trying to investigate is that it had something to
do with handling get() requests larger than the max correctly, but I can't find
any evidence of that one...
-Greg
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Throttle::wait use case clarification
Hi,
While working on unit tests for Throttle.{cc,h} I tried to figure out a use
case related to the Throttle::wait method but couldn't
https://github.com/ceph/ceph/pull/34/files#L3R258
Although it was not a blocker and I managed to reach 100% coverage anyway, it
got me curious and I would very much appreciate pointers to understand the
rationale.
wait() can be called to set a new maximum before waiting for all pending
threads to get get what they asked for. Since the maximum has changed, wait()
wakes up the first thread : the conditions under which it decided to go to
sleep have changed and the conclusion may be different.
However, it only does so when the new maximum is less than current one. For
instance
A) decision does not change
max = 10, current 9
thread 1 tries to get 5 but only 1 is available, it goes to sleep
wait(8)
max = 8, current 9
wakes up thread 1
thread 1 tries to get 5 but current is already beyond the maximum, it goes to
sleep
B) decision changes
max = 10, current 1
thread 1 tries to get 10 but only 9 is available, it goes to sleep
wait(9)
max = 9, current 1
wakes up thread 1
thread 1 tries to get 10 which is above the maximum : it succeeds because
current is below the new maximum
It will not wake up a thread if the maximum increases, for instance:
max = 10, current 9
thread 1 tries to get 5 but only 1 is available, it goes to sleep
wait(20)
max = 20, current 9
does *not* wake up thread 1
keeps waiting until another thread put(N) with N >= 0 although there now is
11 available and it would allow it to get 5 out of it
Why is it not desirable for thread 1 to wake up in this case ? When debugging a
real world situation, I think it would show as a thread blocked although the
throttle it is waiting on has enough to satisfy its request. What am I missing ?
Cheers
<>
signature.asc
Description: OpenPGP digital signature
