global mutex in dvb_usercopy (dvbdev.c)

global mutex in dvb_usercopy (dvbdev.c)

[Soby Mathew]

Hi Everyone,
    I have a doubt regarding about the global mutex lock in
dvb_usercopy(drivers/media/dvb-core/dvbdev.c, line 382) .


/* call driver */
mutex_lock(&dvbdev_mutex);
if ((err = func(file, cmd, parg)) == -ENOIOCTLCMD)
err = -EINVAL;
mutex_unlock(&dvbdev_mutex);


Why is this mutex needed? When I check similar functions like
video_usercopy, this kind of global locking is not present when func()
is called.

This global lock will prevent any other ioctl call from being executed
unless the previous blocking ioctl call has returned. If we need to
have a lock why not make it file handle specific ?


Thanks for your help.

Best Regards
Soby Mathew

Best Regards

Re: global mutex in dvb_usercopy (dvbdev.c)

[Nikolaus Schulz]

On Tue, Jan 08, 2013 at 12:05:47PM +0530, Soby Mathew wrote:
> Hi Everyone,
>     I have a doubt regarding about the global mutex lock in
> dvb_usercopy(drivers/media/dvb-core/dvbdev.c, line 382) .
> 
> 
> /* call driver */
> mutex_lock(&dvbdev_mutex);
> if ((err = func(file, cmd, parg)) == -ENOIOCTLCMD)
> err = -EINVAL;
> mutex_unlock(&dvbdev_mutex);
> 
> 
> Why is this mutex needed? When I check similar functions like
> video_usercopy, this kind of global locking is not present when func()
> is called.

I cannot say anything about video_usercopy(), but as it happens, there's
a patch[1] queued for Linux 3.9 that will hopefully replace the mutex in
dvb_usercopy() with more fine-grained locking.

Nikolaus

[1] http://git.linuxtv.org/media_tree.git/commit/30ad64b8ac539459f8975aa186421ef3db0bb5cb

Re: global mutex in dvb_usercopy (dvbdev.c)

[thomas schorpp]

On 09.01.2013 22:30, Nikolaus Schulz wrote:
> On Tue, Jan 08, 2013 at 12:05:47PM +0530, Soby Mathew wrote:
>> Hi Everyone,
>>      I have a doubt regarding about the global mutex lock in
>> dvb_usercopy(drivers/media/dvb-core/dvbdev.c, line 382) .
>>
>>
>> /* call driver */
>> mutex_lock(&dvbdev_mutex);
>> if ((err = func(file, cmd, parg)) == -ENOIOCTLCMD)
>> err = -EINVAL;
>> mutex_unlock(&dvbdev_mutex);
>>
>>
>> Why is this mutex needed? When I check similar functions like
>> video_usercopy, this kind of global locking is not present when func()
>> is called.
>
> I cannot say anything about video_usercopy(), but as it happens, there's
> a patch[1] queued for Linux 3.9 that will hopefully replace the mutex in
> dvb_usercopy() with more fine-grained locking.
>
> Nikolaus
>
> [1] http://git.linuxtv.org/media_tree.git/commit/30ad64b8ac539459f8975aa186421ef3db0bb5cb

"Unfortunately, frontend ioctls can be blocked by the frontend thread for several seconds; this leads to unacceptable lock contention."
Especially the stv0297 signal locking, as it turned out in situations of bad signal input or my cable providers outtage today it has slowed down dvb_ttpci (notable as OSD- output latency and possibly driver buffer overflows of budget source devices) that much that I had to disable tuning with parm --outputonly in vdr-plugin-dvbsddevice.

Can anyone confirm that and have a look at the other frontend drivers for tuners needing as much driver control?

I will try to apply the patch manually to Linux 3.2 and check with Latencytop tomorrow.

y
tom

Re: global mutex in dvb_usercopy (dvbdev.c)

[Mauro Carvalho Chehab]

Em Thu, 10 Jan 2013 03:06:51 +0100
thomas schorpp <thomas.schorpp@gmail.com> escreveu:

> On 09.01.2013 22:30, Nikolaus Schulz wrote:
> > On Tue, Jan 08, 2013 at 12:05:47PM +0530, Soby Mathew wrote:
> >> Hi Everyone,
> >>      I have a doubt regarding about the global mutex lock in
> >> dvb_usercopy(drivers/media/dvb-core/dvbdev.c, line 382) .
> >>
> >>
> >> /* call driver */
> >> mutex_lock(&dvbdev_mutex);
> >> if ((err = func(file, cmd, parg)) == -ENOIOCTLCMD)
> >> err = -EINVAL;
> >> mutex_unlock(&dvbdev_mutex);
> >>
> >>
> >> Why is this mutex needed? When I check similar functions like
> >> video_usercopy, this kind of global locking is not present when func()
> >> is called.
> >
> > I cannot say anything about video_usercopy(), but as it happens, there's
> > a patch[1] queued for Linux 3.9 that will hopefully replace the mutex in
> > dvb_usercopy() with more fine-grained locking.
> >
> > Nikolaus
> >
> > [1] http://git.linuxtv.org/media_tree.git/commit/30ad64b8ac539459f8975aa186421ef3db0bb5cb
> 
> "Unfortunately, frontend ioctls can be blocked by the frontend thread for several seconds; this leads to unacceptable lock contention."
> Especially the stv0297 signal locking, as it turned out in situations of bad signal input or my cable providers outtage today it has slowed down dvb_ttpci (notable as OSD- output latency and possibly driver buffer overflows of budget source devices) that much that I had to disable tuning with parm --outputonly in vdr-plugin-dvbsddevice.
> 
> Can anyone confirm that and have a look at the other frontend drivers for tuners needing as much driver control?
> 
> I will try to apply the patch manually to Linux 3.2 and check with Latencytop tomorrow.

Well, an ioctl's should not block for a long time, if the device is opened
with O_NONBLOCK. Unfortunately, not all drivers follow this rule, and
blocks.

The right fix seem to have a logic at stv0297 that would do the signal 
locking in background, or to use the already-existent DVB frontend
thread, and answering to userspace the last cached result, instead of
actively talking with the frontend device driver.

Both approaches have advantages and disadvantages. In any case, a change
like that at dvb core has the potential of causing troubles to userspace,
although I think it is the better thing to do, at the long term.

> 
> y
> tom
> 
> 
> 
> 
> 
> --
> 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


-- 

Cheers,
Mauro

Re: global mutex in dvb_usercopy (dvbdev.c)

[thomas schorpp]

On 10.01.2013 15:25, Mauro Carvalho Chehab wrote:
> Em Thu, 10 Jan 2013 03:06:51 +0100
> thomas schorpp <thomas.schorpp@gmail.com> escreveu:
>
>> On 09.01.2013 22:30, Nikolaus Schulz wrote:
>>> On Tue, Jan 08, 2013 at 12:05:47PM +0530, Soby Mathew wrote:
>>>> Hi Everyone,
>>>>       I have a doubt regarding about the global mutex lock in
>>>> dvb_usercopy(drivers/media/dvb-core/dvbdev.c, line 382) .
>>>>
>>>>
>>>> /* call driver */
>>>> mutex_lock(&dvbdev_mutex);
>>>> if ((err = func(file, cmd, parg)) == -ENOIOCTLCMD)
>>>> err = -EINVAL;
>>>> mutex_unlock(&dvbdev_mutex);
>>>>
>>>>
>>>> Why is this mutex needed? When I check similar functions like
>>>> video_usercopy, this kind of global locking is not present when func()
>>>> is called.
>>>
>>> I cannot say anything about video_usercopy(), but as it happens, there's
>>> a patch[1] queued for Linux 3.9 that will hopefully replace the mutex in
>>> dvb_usercopy() with more fine-grained locking.
>>>
>>> Nikolaus
>>>
>>> [1] http://git.linuxtv.org/media_tree.git/commit/30ad64b8ac539459f8975aa186421ef3db0bb5cb
>>
>> "Unfortunately, frontend ioctls can be blocked by the frontend thread for several seconds; this leads to unacceptable lock contention."
>> Especially the stv0297 signal locking, as it turned out in situations of bad signal input or my cable providers outtage today it has slowed down dvb_ttpci (notable as OSD- output latency and possibly driver buffer overflows of budget source devices) that much that I had to disable tuning with parm --outputonly in vdr-plugin-dvbsddevice.
>>
>> Can anyone confirm that and have a look at the other frontend drivers for tuners needing as much driver control?
>>
>> I will try to apply the patch manually to Linux 3.2 and check with Latencytop tomorrow.
>
> Well, an ioctl's should not block for a long time, if the device is opened
> with O_NONBLOCK. Unfortunately, not all drivers follow this rule, and
> blocks.
>
> The right fix seem to have a logic at stv0297 that would do the signal
> locking in background, or to use the already-existent DVB frontend
> thread, and answering to userspace the last cached result, instead of
> actively talking with the frontend device driver.
>
> Both approaches have advantages and disadvantages. In any case, a change
> like that at dvb core has the potential of causing troubles to userspace,
> although I think it is the better thing to do, at the long term.

Could You or another with write access commit this in http://git.linuxtv.org/media_build.git if applicable, too, please?
I'look into but I'm not yet worked in in kernel threading methods, need to read the O'reilly linux Driver book first about ;-)

y
tom