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
>>> <mchehab@redhat.com> 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 <obi@linuxtv.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Fri, Dec 9, 2011 at 8:37 PM, Andreas Oberritter <obi@linuxtv.org> 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
>>>> <mchehab@redhat.com> 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 <obi@linuxtv.org>
>
> 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.000000000 +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
  

On 10.12.2011 02:59, Devin Heitmueller wrote:
> On Fri, Dec 9, 2011 at 8:37 PM, Andreas Oberritter <obi@linuxtv.org> 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
>>>>> <mchehab@redhat.com> 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 <obi@linuxtv.org>
>>
>> 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.000000000 +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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hello Andreas,

On Fri, Dec 9, 2011 at 9:06 PM, Andreas Oberritter <obi@linuxtv.org> 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
  

On 10-12-2011 00:25, Devin Heitmueller wrote:
> Hello Andreas,
>
> On Fri, Dec 9, 2011 at 9:06 PM, Andreas Oberritter<obi@linuxtv.org>  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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

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
>>>> <mchehab@redhat.com>  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<obi@linuxtv.org>

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.000000000 +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 hurt would be to increase the chance for us to
detect a bug that it is already there.

Devin,

I'll do some tests here with a few devices, but, in principle, I don't see
any reason why not applying this patch. So, except if I detect something wrong
on my tests, of if you you point us for a regression caused by this change,
I'll apply it.

Of course, it would be nice if Andreas could add some comments, but if he doesn't,
I can write something. It won't be the first patch that the maintainer would
need to insert some description.

Regards,
Mauro.

[1] The actual implementation is a more complex than that loop. In this
specific case, as it uses the interruptible version, any signal would
also wake this thread.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hello Mauro,

On Sat, Dec 10, 2011 at 5:28 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> 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
  

On 10-12-2011 11:43, Devin Heitmueller wrote:
> Hello Mauro,
>
> On Sat, Dec 10, 2011 at 5:28 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com>  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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html