On Sunday 01 March 2009, Arve Hj?nnev?g wrote:
> On Sat, Feb 28, 2009 at 2:53 PM, Rafael J. Wysocki <[email protected]> wrote:
> >> >> What you are talking about here is mostly an optimization of the
> >> >> wakelock api. You have removed timeout support and made each wakelock
> >> >> reference counted.
> >> >
> >> > I also removed the arbitrary number of wakelocks (I really _hate_ the name,
> >> > can we please stop using it from now on?).
> >>
> >> What do you mean by this? You removed the struct wake_lock?
> >
> > Basically, yes.
>
> Why is this better? If you move the stats into devices and tasks, this
> may take up more space than adding an object to the structures or
> tasks that you are protecting.
OK, this is a valid point as long as we're going to use the stats in the
original form (which I'm not sure we want).
Still, as far as the number of "locks" one process can take is limited, it's
fine.
> >> >> I do provide an option to turn off the wakelock stats, which makes
> >> >> wake_lock/unlock significantly faster, but we never run with wakelock
> >> >> stats off. Also, it pushes timeout handling to the drivers. I know may
> >> >> of you don't like timeout support, but ignoring the problem is not a
> >> >> solution. If each driver that needs timeouts uses its own timer, then
> >> >> you will often wakeup from idle just to unlock a wakelock that will
> >> >> not trigger suspend. This wakeup is a thousand times as costly on the
> >> >> msm platform as a wakelock/unlock pair (with wakelock stats enabled).
> >> >
> >> > Well, at least a couple of people told you that the timeouts are hardly
> >> > acceptable and they told you why. Please stop repeating the same arguments you
> >> > have given already for a couple of times. They're not convincing.
> >>
> >> And you keep ignoring them.
> >
> > Not ignoring, but considering them as insufficient. And since they've already
> > been considered as insufficient, there's no point repeating them over and over
> > again. That doesn't make them any better.
>
> The problem is that what you consider insufficient is what allows us
> to ship a product.
This doesn't matter a whit, because the mainline kernel is not just your
product.
By the same rule you could say that every working vendor driver is worth
merging into the mainline kernel, which clearly is not the case.
> >> > I do realize that you consider your current solution as the best thing since
> >> > the sliced bread, but please accept the fact that the other people think
> >> > differently.
> >>
> >> I certainly do not think my current solution is the best, it is very
> >> invasive. I do however think your proposed solution is worse. The only
> >> proposed alternative that we could actually ship a product on today is
> >> to not use suspend at all.
> >
> > Well, I'm sure your code is useful for the Android platform, but the question
> > is whether we want this code in the mainline kernel. For now, the answer is
> > "no, we don't". Moreover, since you're the one who wants the code to be
> > merged, it's your burden to make it acceptable for us. However you're going
> > to do it is up to you, but certainly trying to force your current code on us
> > is not going to work.
>
> I don't think I am the only one who want this code in the mainline
> kernel. Many people want to use the android platform, and support in
> the mainline kernel would be beneficial to some of them. I made many
> requested changes to my code that provides no benefit to us, but I
> have not made any changes that breaks our own use.
OK, please resubmit the patches, then.
> > BTW, I think you handled the thing wrong. If I were you, I wouldn't even try
> > to push the code as you did. I would instead make it as simple as reasonably
> > possible so that the basic idea was clear and understandable to everyone.
> > Then, if there were any doubts with respect to the basic idea, I'd try to
> > clarify them and I'd consider modifying the code to address objections.
> > I'd only try to add more features after the basic idea had been accepted.
>
> The basic concept was developed long before android was a public project.
Please refer to the Ben's message for a good answer to this.
Thanks,
Rafael
On Sun, Mar 1, 2009 at 1:20 AM, Rafael J. Wysocki <[email protected]> wrote:
>> >> >> I do provide an option to turn off the wakelock stats, which makes
>> >> >> wake_lock/unlock significantly faster, but we never run with wakelock
>> >> >> stats off. Also, it pushes timeout handling to the drivers. I know may
>> >> >> of you don't like timeout support, but ignoring the problem is not a
>> >> >> solution. If each driver that needs timeouts uses its own timer, then
>> >> >> you will often wakeup from idle just to unlock a wakelock that will
>> >> >> not trigger suspend. This wakeup is a thousand times as costly on the
>> >> >> msm platform as a wakelock/unlock pair (with wakelock stats enabled).
>> >> >
>> >> > Well, at least a couple of people told you that the timeouts are hardly
>> >> > acceptable and they told you why. ?Please stop repeating the same arguments you
>> >> > have given already for a couple of times. ?They're not convincing.
>> >>
>> >> And you keep ignoring them.
>> >
>> > Not ignoring, but considering them as insufficient. ?And since they've already
>> > been considered as insufficient, there's no point repeating them over and over
>> > again. ?That doesn't make them any better.
>>
>> The problem is that what you consider insufficient is what allows us
>> to ship a product.
>
> This doesn't matter a whit, because the mainline kernel is not just your
> product.
Unless you are saying that changes in the mainline kernel does not
need be usable in practice, then it does matter. If we remove the
feature that allows us to interact with existing code, it will take
much longer before it is usable by anyone.
> By the same rule you could say that every working vendor driver is worth
> merging into the mainline kernel, which clearly is not the case.
Some people seem to think they are.
>
>> >> > I do realize that you consider your current solution as the best thing since
>> >> > the sliced bread, but please accept the fact that the other people think
>> >> > differently.
>> >>
>> >> I certainly do not think my current solution is the best, it is very
>> >> invasive. I do however think your proposed solution is worse. The only
>> >> proposed alternative that we could actually ship a product on today is
>> >> to not use suspend at all.
>> >
>> > Well, I'm sure your code is useful for the Android platform, but the question
>> > is whether we want this code in the mainline kernel. ?For now, the answer is
>> > "no, we don't". ?Moreover, since you're the one who wants the code to be
>> > merged, it's your burden to make it acceptable for us. ?However you're going
>> > to do it is up to you, but certainly trying to force your current code on us
>> > is not going to work.
>>
>> I don't think I am the only one who want this code in the mainline
>> kernel. Many people want to use the android platform, and support in
>> the mainline kernel would be beneficial to some of them. I made many
>> requested changes to my code that provides no benefit to us, but I
>> have not made any changes that breaks our own use.
>
> OK, please resubmit the patches, then.
I submitted them three weeks ago. I'll submit a new set after I rename
the api (presumably to suspend_block(er)) but I would like more
agreement on the timeout issue first.
--
Arve Hj?nnev?g
Hi!
> >> > Not ignoring, but considering them as insufficient. ?And since they've already
> >> > been considered as insufficient, there's no point repeating them over and over
> >> > again. ?That doesn't make them any better.
> >>
> >> The problem is that what you consider insufficient is what allows us
> >> to ship a product.
> >
> > This doesn't matter a whit, because the mainline kernel is not just your
> > product.
>
> Unless you are saying that changes in the mainline kernel does not
> need be usable in practice, then it does matter. If we remove the
> feature that allows us to interact with existing code, it will take
> much longer before it is usable by anyone.
Well, taking longer before "being usable" is good tradeoff if it means
"we get cleaner/actually correct system in mainline sooner".
> >> I don't think I am the only one who want this code in the mainline
> >> kernel. Many people want to use the android platform, and support in
> >> the mainline kernel would be beneficial to some of them. I made many
> >> requested changes to my code that provides no benefit to us, but I
> >> have not made any changes that breaks our own use.
> >
> > OK, please resubmit the patches, then.
>
> I submitted them three weeks ago. I'll submit a new set after I rename
> the api (presumably to suspend_block(er)) but I would like more
> agreement on the timeout issue first.
I do believe that everyone (including you :-) agrees that timeouts are
ugly hack. So just reorder the series so they come at the end.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, Mar 3, 2009 at 5:51 AM, Pavel Machek <[email protected]> wrote:
> Hi!
>
>> >> > Not ignoring, but considering them as insufficient. ?And since they've already
>> >> > been considered as insufficient, there's no point repeating them over and over
>> >> > again. ?That doesn't make them any better.
>> >>
>> >> The problem is that what you consider insufficient is what allows us
>> >> to ship a product.
>> >
>> > This doesn't matter a whit, because the mainline kernel is not just your
>> > product.
>>
>> Unless you are saying that changes in the mainline kernel does not
>> need be usable in practice, then it does matter. If we remove the
>> feature that allows us to interact with existing code, it will take
>> much longer before it is usable by anyone.
>
> Well, taking longer before "being usable" is good tradeoff if it means
> "we get cleaner/actually correct system in mainline sooner".
I think this could go either way. If the system is usable, it may get
more use from developers that know how to improve a specific subsystem
to not use timeouts. Or, it may be considered good enough, and nobody
bothers coming up with a correct solution. I think the latter is what
you are worried will happen.
>
>> >> I don't think I am the only one who want this code in the mainline
>> >> kernel. Many people want to use the android platform, and support in
>> >> the mainline kernel would be beneficial to some of them. I made many
>> >> requested changes to my code that provides no benefit to us, but I
>> >> have not made any changes that breaks our own use.
>> >
>> > OK, please resubmit the patches, then.
>>
>> I submitted them three weeks ago. I'll submit a new set after I rename
>> the api (presumably to suspend_block(er)) but I would like more
>> agreement on the timeout issue first.
>
> I do believe that everyone (including you :-) agrees that timeouts are
> ugly hack. So just reorder the series so they come at the end.
No, I think many uses of timeouts are a ugly hack, not all, but OK I
will try to move timeout support to a separate patch.
--
Arve Hj?nnev?g
On Tue 2009-03-03 16:06:02, Arve Hj?nnev?g wrote:
> On Tue, Mar 3, 2009 at 5:51 AM, Pavel Machek <[email protected]> wrote:
> > Hi!
> >
> >> >> > Not ignoring, but considering them as insufficient. ?And since they've already
> >> >> > been considered as insufficient, there's no point repeating them over and over
> >> >> > again. ?That doesn't make them any better.
> >> >>
> >> >> The problem is that what you consider insufficient is what allows us
> >> >> to ship a product.
> >> >
> >> > This doesn't matter a whit, because the mainline kernel is not just your
> >> > product.
> >>
> >> Unless you are saying that changes in the mainline kernel does not
> >> need be usable in practice, then it does matter. If we remove the
> >> feature that allows us to interact with existing code, it will take
> >> much longer before it is usable by anyone.
> >
> > Well, taking longer before "being usable" is good tradeoff if it means
> > "we get cleaner/actually correct system in mainline sooner".
>
> I think this could go either way. If the system is usable, it may get
> more use from developers that know how to improve a specific subsystem
> to not use timeouts. Or, it may be considered good enough, and nobody
> bothers coming up with a correct solution. I think the latter is what
> you are worried will happen.
Yep.
> >> I submitted them three weeks ago. I'll submit a new set after I rename
> >> the api (presumably to suspend_block(er)) but I would like more
> >> agreement on the timeout issue first.
> >
> > I do believe that everyone (including you :-) agrees that timeouts are
> > ugly hack. So just reorder the series so they come at the end.
>
> No, I think many uses of timeouts are a ugly hack, not all, but OK I
> will try to move timeout support to a separate patch.
Thanks.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html