[PATCH] DVB: dvb_frontend: fix delayed thread exit
On 10.12.2011 00:43, Mauro Carvalho Chehab wrote:
> On 09-12-2011 21:37, Mauro Carvalho Chehab wrote:
>> On 09-12-2011 20:33, Devin Heitmueller wrote:
>>> On Fri, Dec 9, 2011 at 5:11 PM, Mauro Carvalho Chehab
>>> wrote:
> Could someone explain reason for that?
I dunno, but I think this needs to be fixed, at least when the frontend
is opened with O_NONBLOCK.
>>>
>>> Are you doing the drx-k firmware load on dvb_init()? That could
>>> easily take 4 seconds.
>>
>> No. The firmware were opened previously.
>
> Maybe the delay is due to this part of dvb_frontend.c:
>
> static int dvb_mfe_wait_time = 5;
> ...
> int mferetry = (dvb_mfe_wait_time << 1);
>
> mutex_unlock (&adapter->mfe_lock);
> while (mferetry-- && (mfedev->users != -1 ||
> mfepriv->thread != NULL)) {
> if(msleep_interruptible(500)) {
> if(signal_pending(current))
> return -EINTR;
> }
> }
I haven't looked at the mfe code, but in case it's waiting for the
frontend thread to exit, there's a problem that causes the thread
not to exit immediately. Here's a patch that's been sitting in my
queue for a while:
---
Signed-off-by: Andreas Oberritter
diff --git a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c
b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c
index 7784d74..6823c2b 100644
--- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-07
12:32:24.0 +0200
+++ a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-13
15:55:48.865742791 +0200
@@ -514,7 +514,7 @@
return 1;
if (fepriv->dvbdev->writers == 1)
- if (time_after(jiffies, fepriv->release_jiffies +
+ if (time_after_eq(jiffies, fepriv->release_jiffies +
dvb_shutdown_timeout * HZ))
return 1;
@@ -2070,12 +2070,15 @@
dprintk ("%s\n", __func__);
- if ((file->f_flags & O_ACCMODE) != O_RDONLY)
+ if ((file->f_flags & O_ACCMODE) != O_RDONLY) {
fepriv->release_jiffies = jiffies;
+ mb();
+ }
ret = dvb_generic_release (inode, file);
if (dvbdev->users == -1) {
+ wake_up(&fepriv->wait_queue);
if (fepriv->exit != DVB_FE_NO_EXIT) {
fops_put(file->f_op);
file->f_op = NULL;
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit
On Fri, Dec 9, 2011 at 8:37 PM, Andreas Oberritter wrote:
> On 10.12.2011 00:43, Mauro Carvalho Chehab wrote:
>> On 09-12-2011 21:37, Mauro Carvalho Chehab wrote:
>>> On 09-12-2011 20:33, Devin Heitmueller wrote:
On Fri, Dec 9, 2011 at 5:11 PM, Mauro Carvalho Chehab
wrote:
>> Could someone explain reason for that?
>
>
> I dunno, but I think this needs to be fixed, at least when the frontend
> is opened with O_NONBLOCK.
Are you doing the drx-k firmware load on dvb_init()? That could
easily take 4 seconds.
>>>
>>> No. The firmware were opened previously.
>>
>> Maybe the delay is due to this part of dvb_frontend.c:
>>
>> static int dvb_mfe_wait_time = 5;
>> ...
>> int mferetry = (dvb_mfe_wait_time << 1);
>>
>> mutex_unlock (&adapter->mfe_lock);
>> while (mferetry-- && (mfedev->users != -1 ||
>> mfepriv->thread != NULL)) {
>> if(msleep_interruptible(500)) {
>> if(signal_pending(current))
>> return -EINTR;
>> }
>> }
>
> I haven't looked at the mfe code, but in case it's waiting for the
> frontend thread to exit, there's a problem that causes the thread
> not to exit immediately. Here's a patch that's been sitting in my
> queue for a while:
>
> ---
>
> Signed-off-by: Andreas Oberritter
>
> diff --git a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c
> b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c
> index 7784d74..6823c2b 100644
> --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-07
> 12:32:24.0 +0200
> +++ a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-13
> 15:55:48.865742791 +0200
> @@ -514,7 +514,7 @@
> return 1;
>
> if (fepriv->dvbdev->writers == 1)
> - if (time_after(jiffies, fepriv->release_jiffies +
> + if (time_after_eq(jiffies, fepriv->release_jiffies +
> dvb_shutdown_timeout * HZ))
> return 1;
>
> @@ -2070,12 +2070,15 @@
>
> dprintk ("%s\n", __func__);
>
> - if ((file->f_flags & O_ACCMODE) != O_RDONLY)
> + if ((file->f_flags & O_ACCMODE) != O_RDONLY) {
> fepriv->release_jiffies = jiffies;
> + mb();
> + }
>
> ret = dvb_generic_release (inode, file);
>
> if (dvbdev->users == -1) {
> + wake_up(&fepriv->wait_queue);
> if (fepriv->exit != DVB_FE_NO_EXIT) {
> fops_put(file->f_op);
> file->f_op = NULL;
This patch needs to have a much better explanation of exactly what it
does and what problem it solves. We have a history of race conditions
in dvb_frontend.c, and it's patches like this with virtually no
details just makes it worse.
I'm not arguing the actual merits of the code change - it *may* be
correct. But without the appropriate background there is no real way
of knowing...
Mauro, this patch should be NACK'd and resubmitted with a detailed
explanation of the current behavior, what the problem is, and how the
code changes proposed solve that problem.
Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit
On 10.12.2011 02:59, Devin Heitmueller wrote:
> On Fri, Dec 9, 2011 at 8:37 PM, Andreas Oberritter wrote:
>> On 10.12.2011 00:43, Mauro Carvalho Chehab wrote:
>>> On 09-12-2011 21:37, Mauro Carvalho Chehab wrote:
On 09-12-2011 20:33, Devin Heitmueller wrote:
> On Fri, Dec 9, 2011 at 5:11 PM, Mauro Carvalho Chehab
> wrote:
>>> Could someone explain reason for that?
>>
>>
>> I dunno, but I think this needs to be fixed, at least when the frontend
>> is opened with O_NONBLOCK.
>
> Are you doing the drx-k firmware load on dvb_init()? That could
> easily take 4 seconds.
No. The firmware were opened previously.
>>>
>>> Maybe the delay is due to this part of dvb_frontend.c:
>>>
>>> static int dvb_mfe_wait_time = 5;
>>> ...
>>> int mferetry = (dvb_mfe_wait_time << 1);
>>>
>>> mutex_unlock (&adapter->mfe_lock);
>>> while (mferetry-- && (mfedev->users != -1 ||
>>> mfepriv->thread != NULL)) {
>>> if(msleep_interruptible(500)) {
>>> if(signal_pending(current))
>>> return -EINTR;
>>> }
>>> }
>>
>> I haven't looked at the mfe code, but in case it's waiting for the
>> frontend thread to exit, there's a problem that causes the thread
>> not to exit immediately. Here's a patch that's been sitting in my
>> queue for a while:
>>
>> ---
>>
>> Signed-off-by: Andreas Oberritter
>>
>> diff --git a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c
>> b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c
>> index 7784d74..6823c2b 100644
>> --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-07
>> 12:32:24.0 +0200
>> +++ a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-13
>> 15:55:48.865742791 +0200
>> @@ -514,7 +514,7 @@
>>return 1;
>>
>>if (fepriv->dvbdev->writers == 1)
>> - if (time_after(jiffies, fepriv->release_jiffies +
>> + if (time_after_eq(jiffies, fepriv->release_jiffies +
>> dvb_shutdown_timeout * HZ))
>>return 1;
>>
>> @@ -2070,12 +2070,15 @@
>>
>>dprintk ("%s\n", __func__);
>>
>> - if ((file->f_flags & O_ACCMODE) != O_RDONLY)
>> + if ((file->f_flags & O_ACCMODE) != O_RDONLY) {
>>fepriv->release_jiffies = jiffies;
>> + mb();
>> + }
>>
>>ret = dvb_generic_release (inode, file);
>>
>>if (dvbdev->users == -1) {
>> + wake_up(&fepriv->wait_queue);
>>if (fepriv->exit != DVB_FE_NO_EXIT) {
>>fops_put(file->f_op);
>>file->f_op = NULL;
>
> This patch needs to have a much better explanation of exactly what it
> does and what problem it solves. We have a history of race conditions
> in dvb_frontend.c, and it's patches like this with virtually no
> details just makes it worse.
>
> I'm not arguing the actual merits of the code change - it *may* be
> correct. But without the appropriate background there is no real way
> of knowing...
>
> Mauro, this patch should be NACK'd and resubmitted with a detailed
> explanation of the current behavior, what the problem is, and how the
> code changes proposed solve that problem.
WTF, Devin, you again? I haven't asked anyone to upstream it. Feel free
to analyze the code and resubmit it.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit
Hello Andreas, On Fri, Dec 9, 2011 at 9:06 PM, Andreas Oberritter wrote: > WTF, Devin, you again? I haven't asked anyone to upstream it. Feel free > to analyze the code and resubmit it. 1. It's marked with a subject line that starts with "[PATCH]" 2. It has your SIgned-Off-By line. 3. it was sent to the mailing list. 4. It doesn't have any keywords like "RFC" or "proposed". If you don't intend for it to go upstream then don't do all of the above. I'm not sure if your "WTF, Devin, you again?" is some indication that I'm annoying you. The last patch you submitted that touches the threading in dvb_frontend.c had a host of problems and was clearly not well researched (i.e. "DVB: dvb_frontend: convert semaphore to mutex"). As in this case, there is no background indicating that this patch has been fully thought out and due diligence has been done. Maybe you have thoroughly researched the change, taken the time to fully understand its effects, and tested it with a variety of boards and scenarios. Without a good patch description, there is no way to know. I apologize if you're inconvenienced if I'm making an active effort to prevent poorly documented changes from getting merged (which often result in regressions). Oh wait, I'm not sorry at all. Nevermind. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit
On 10-12-2011 00:25, Devin Heitmueller wrote: Hello Andreas, On Fri, Dec 9, 2011 at 9:06 PM, Andreas Oberritter wrote: WTF, Devin, you again? I haven't asked anyone to upstream it. Feel free to analyze the code and resubmit it. 1. It's marked with a subject line that starts with "[PATCH]" 2. It has your SIgned-Off-By line. 3. it was sent to the mailing list. 4. It doesn't have any keywords like "RFC" or "proposed". Devin, You're over-reacting. This patch were a reply from Andreas to a thread, and not a separate patch submission. Patches like are generally handled as RFC, especially since it doesn't contain a description. If you don't intend for it to go upstream then don't do all of the above. I'm not sure if your "WTF, Devin, you again?" is some indication that I'm annoying you. The last patch you submitted that touches the threading in dvb_frontend.c had a host of problems and was clearly not well researched (i.e. "DVB: dvb_frontend: convert semaphore to mutex"). As in this case, there is no background indicating that this patch has been fully thought out and due diligence has been done. Maybe you have thoroughly researched the change, taken the time to fully understand its effects, and tested it with a variety of boards and scenarios. Without a good patch description, there is no way to know. I apologize if you're inconvenienced if I'm making an active effort to prevent poorly documented changes from getting merged (which often result in regressions). Oh wait, I'm not sorry at all. Nevermind. Devin Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit
On 09-12-2011 23:37, Andreas Oberritter wrote:
On 10.12.2011 00:43, Mauro Carvalho Chehab wrote:
On 09-12-2011 21:37, Mauro Carvalho Chehab wrote:
On 09-12-2011 20:33, Devin Heitmueller wrote:
On Fri, Dec 9, 2011 at 5:11 PM, Mauro Carvalho Chehab
wrote:
Could someone explain reason for that?
I dunno, but I think this needs to be fixed, at least when the frontend
is opened with O_NONBLOCK.
Are you doing the drx-k firmware load on dvb_init()? That could
easily take 4 seconds.
No. The firmware were opened previously.
Maybe the delay is due to this part of dvb_frontend.c:
static int dvb_mfe_wait_time = 5;
...
int mferetry = (dvb_mfe_wait_time<< 1);
mutex_unlock (&adapter->mfe_lock);
while (mferetry--&& (mfedev->users != -1 ||
mfepriv->thread != NULL)) {
if(msleep_interruptible(500)) {
if(signal_pending(current))
return -EINTR;
}
}
I haven't looked at the mfe code, but in case it's waiting for the
frontend thread to exit, there's a problem that causes the thread
not to exit immediately. Here's a patch that's been sitting in my
queue for a while:
---
Signed-off-by: Andreas Oberritter
Andreas,
Thanks for the patch!
Devin,
diff --git a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c
b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c
index 7784d74..6823c2b 100644
--- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-07
12:32:24.0 +0200
+++ a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c 2011-09-13
15:55:48.865742791 +0200
@@ -514,7 +514,7 @@
return 1;
if (fepriv->dvbdev->writers == 1)
- if (time_after(jiffies, fepriv->release_jiffies +
+ if (time_after_eq(jiffies, fepriv->release_jiffies +
dvb_shutdown_timeout * HZ))
The only change here is that it will now use dvb_shutdown_timeout instead of
(dvb_shutdown_timeout * HZ + 1).
This makes sense.
return 1;
@@ -2070,12 +2070,15 @@
dprintk ("%s\n", __func__);
- if ((file->f_flags& O_ACCMODE) != O_RDONLY)
+ if ((file->f_flags& O_ACCMODE) != O_RDONLY) {
fepriv->release_jiffies = jiffies;
+ mb();
This is just a memory barrier to warrant that all CPU's will consider the new
value for release_jiffies.
Probably Andreas added it because he noticed some race condition.
In any case, this won't cause any regressions.
+ }
ret = dvb_generic_release (inode, file);
if (dvbdev->users == -1) {
+ wake_up(&fepriv->wait_queue);
This is the only hook that changes the core behavior.
if (fepriv->exit != DVB_FE_NO_EXIT) {
fops_put(file->f_op);
file->f_op = NULL;
With this change, the current code at dvb_frontend_release() wil; be:
ret = dvb_generic_release (inode, file);
if (dvbdev->users == -1) {
wake_up(&fepriv->wait_queue);
if (fepriv->exit != DVB_FE_NO_EXIT) {
fops_put(file->f_op);
file->f_op = NULL;
wake_up(&dvbdev->wait_queue);
}
if (fe->ops.ts_bus_ctrl)
fe->ops.ts_bus_ctrl(fe, 0);
}
The addition of a wake_up there is that the wake_up thread will be called
also when fepriv->exit == DVB_FE_NO_EXIT. This seems to make sense, as
dvb_frontend_thread() explicitly tests for it at:
wait_event_interruptible_timeout(fepriv->wait_queue,
dvb_frontend_should_wakeup(fe) || kthread_should_stop()
|| freezing(current),
fepriv->delay);
as dvb_frontend_should_wakeup(fe) is defined (after applying this patch) as:
static int dvb_frontend_is_exiting(struct dvb_frontend *fe)
{
struct dvb_frontend_private *fepriv = fe->frontend_priv;
if (fepriv->exit != DVB_FE_NO_EXIT)
return 1;
if (fepriv->dvbdev->writers == 1)
if (time_after_eq(jiffies, fepriv->release_jiffies +
dvb_shutdown_timeout * HZ))
return 1;
return 0;
}
static int dvb_frontend_should_wakeup(struct dvb_frontend *fe)
{
struct dvb_frontend_private *fepriv = fe->frontend_priv;
if (fepriv->wakeup) {
fepriv->wakeup = 0;
return 1;
}
return dvb_frontend_is_exiting(fe);
}
So, this code makes sense to me. Btw, a wait queue can wait even without
an explicit call, since it is just something like [1]:
do schedule() while (!condition);
So, all this patch would
Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit
Hello Mauro, On Sat, Dec 10, 2011 at 5:28 AM, Mauro Carvalho Chehab wrote: > Devin, > > You're over-reacting. This patch were a reply from Andreas to a thread, > and not a separate patch submission. > > Patches like are generally handled as RFC, especially since it doesn't > contain a description. Any email that starts with "WTF, Devin, you again?" will probably not get a very polite response. I agree there's been some overreaction, but it hasn't been on my part. He took the time to split it onto a new thread, add the subject line "PATCH", as well as adding an SOB. Even if his intent was only to get it reviewed, why should I waste half an hour of my time analyzing his patch to try to figure out his intent if he isn't willing to simply document it? You have a history of blindly accepting such patches without review. My only intent was to flag this patch to ensure that this didn't happen here. I've spent way more time than I should have to fixing obscure race conditions in dvb core. If the author of a patch cannot take the time to document his findings to provide context then the patch should be rejected without review until he does so. And why isn't this broken into a patch series? Even after you analyzed the patch you still don't understand what the changes do and why there are being made. Your explanation for why he added the "mb()" call was because "Probably Andreas added it because he noticed some race condition". What is the race condition? Did he find multiple race conditions? Is this patch multiple fixes for a race condition and some other crap at the same time? If a developer wants a patch reviewed (as Andreas suggested was the case here after-the-fact), then here's my feedback: break this into a series of small incremental patches which *in detail* describe the problem that was found and how each patch addresses the issue. Once we have that, the maintainer can do a more in-depth analysis of whether the patch should be accepted. Code whose function cannot be explicitly justified but simply 'looks better' should not be mixed in with real functional changes. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit
On 10-12-2011 11:43, Devin Heitmueller wrote: Hello Mauro, On Sat, Dec 10, 2011 at 5:28 AM, Mauro Carvalho Chehab wrote: Devin, You're over-reacting. This patch were a reply from Andreas to a thread, and not a separate patch submission. Patches like are generally handled as RFC, especially since it doesn't contain a description. Any email that starts with "WTF, Devin, you again?" will probably not get a very polite response. I agree there's been some overreaction, but it hasn't been on my part. He took the time to split it onto a new thread, add the subject line "PATCH", as well as adding an SOB. Even if his intent was only to get it reviewed, why should I waste half an hour of my time analyzing his patch to try to figure out his intent if he isn't willing to simply document it? Both overacted, but this doesn't bring anything good. You have a history of blindly accepting such patches without review. No. I always review all patches I receive. Yeah, I have to confess: I'm not a robot, I'm not infallible ;) (well, even a robot would hardly be infallible, anyway). My only intent was to flag this patch to ensure that this didn't happen here. I've spent way more time than I should have to fixing obscure race conditions in dvb core. If the author of a patch cannot take the time to document his findings to provide context then the patch should be rejected without review until he does so. And why isn't this broken into a patch series? Even after you analyzed the patch you still don't understand what the changes do and why there are being made. Your explanation for why he added the "mb()" call was because "Probably Andreas added it because he noticed some race condition". What is the race condition? Did he find multiple race conditions? Is this patch multiple fixes for a race condition and some other crap at the same time? If a developer wants a patch reviewed (as Andreas suggested was the case here after-the-fact), then here's my feedback: break this into a series of small incremental patches which *in detail* describe the problem that was found and how each patch addresses the issue. Once we have that, the maintainer can do a more in-depth analysis of whether the patch should be accepted. Code whose function cannot be explicitly justified but simply 'looks better' should not be mixed in with real functional changes. I understand that you want patches better documented, so do I, and it would be great if this patch had a better description since the beginning. Yet, I don't agree that this patch should be split. It does just one thing: it fixes the timeout handling for the dvb core frontend thread. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
