schedule inside spin_lock_irqsave?

schedule inside spin_lock_irqsave?

[Richard Zidlicky]

Hi,

came across following snippet of code (2.6.34:drivers/media/dvb/siano/smscoreapi.c) and 
since prepare_to_wait is new for me I am wondering if this is can work?

struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
{
	struct smscore_buffer_t *cb = NULL;
	unsigned long flags;

	DEFINE_WAIT(wait);

	spin_lock_irqsave(&coredev->bufferslock, flags);

	/* This function must return a valid buffer, since the buffer list is
	 * finite, we check that there is an available buffer, if not, we wait
	 * until such buffer become available.
	 */

	prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);

	if (list_empty(&coredev->buffers))
		schedule();

	finish_wait(&coredev->buffer_mng_waitq, &wait);

	cb = (struct smscore_buffer_t *) coredev->buffers.next;
	list_del(&cb->entry);

	spin_unlock_irqrestore(&coredev->bufferslock, flags);

	return cb;
}

Regards
Richard

Re: schedule inside spin_lock_irqsave?

[Jiri Slaby]

On 05/30/2010 04:52 PM, Richard Zidlicky wrote:
> Hi,
> 
> came across following snippet of code (2.6.34:drivers/media/dvb/siano/smscoreapi.c) and 
> since prepare_to_wait is new for me I am wondering if this is can work?
> 
> struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
> {
> 	struct smscore_buffer_t *cb = NULL;
> 	unsigned long flags;
> 
> 	DEFINE_WAIT(wait);
> 
> 	spin_lock_irqsave(&coredev->bufferslock, flags);
> 
> 	/* This function must return a valid buffer, since the buffer list is
> 	 * finite, we check that there is an available buffer, if not, we wait
> 	 * until such buffer become available.
> 	 */
> 
> 	prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);
> 
> 	if (list_empty(&coredev->buffers))
> 		schedule();
> 
> 	finish_wait(&coredev->buffer_mng_waitq, &wait);
> 
> 	cb = (struct smscore_buffer_t *) coredev->buffers.next;
> 	list_del(&cb->entry);
> 
> 	spin_unlock_irqrestore(&coredev->bufferslock, flags);


Yep, that's a double bug.
1) If the waiting is interrupted, it will die because the list is still
empty.
2) If there is no entry in the list, it will deadlock at least on UP.
This should be
wait_event(&coredev->buffer_mng_waitq, cb = get_entry());
with get_entry like:
struct smscore_buffer_t *get_entry(void)
{
  struct smscore_buffer_t *cb = NULL;
  spin_lock_irqsave(&coredev->bufferslock, flags);
  if (!list_empty(&coredev->buffers)) {
    cb = (struct smscore_buffer_t *) coredev->buffers.next;
    list_del(&cb->entry);
  }
  spin_unlock_irqrestore(&coredev->bufferslock, flags);
  return cb;
}

regards,
-- 
js

Re: schedule inside spin_lock_irqsave?

[Jiri Slaby]

On 05/30/2010 05:24 PM, Jiri Slaby wrote:
> struct smscore_buffer_t *get_entry(void)
> {
>   struct smscore_buffer_t *cb = NULL;
>   spin_lock_irqsave(&coredev->bufferslock, flags);
>   if (!list_empty(&coredev->buffers)) {
>     cb = (struct smscore_buffer_t *) coredev->buffers.next;

Looking at the smscore_buffer_t definition, this is really ugly since it
relies on entry being the first in the structure. It should be
list_first_entry(&coredev->buffers, ...) instead, cast-less.

>     list_del(&cb->entry);
>   }
>   spin_unlock_irqrestore(&coredev->bufferslock, flags);
>   return cb;
> }


-- 
js

Re: schedule inside spin_lock_irqsave?

[Jiri Slaby]

On 05/30/2010 05:24 PM, Jiri Slaby wrote:
> struct smscore_buffer_t *get_entry(void)
> {
>   struct smscore_buffer_t *cb = NULL;
>   spin_lock_irqsave(&coredev->bufferslock, flags);
>   if (!list_empty(&coredev->buffers)) {
>     cb = (struct smscore_buffer_t *) coredev->buffers.next;

Looking at the smscore_buffer_t definition, this is really ugly since it
relies on entry being the first in the structure. It should be
list_first_entry(&coredev->buffers, ...) instead, cast-less.

>     list_del(&cb->entry);
>   }
>   spin_unlock_irqrestore(&coredev->bufferslock, flags);
>   return cb;
> }


-- 
js

Re: schedule inside spin_lock_irqsave?

[Richard Zidlicky]

On Sun, May 30, 2010 at 05:24:38PM +0200, Jiri Slaby wrote:

Hi,

> > came across following snippet of code (2.6.34:drivers/media/dvb/siano/smscoreapi.c) and 

...
...
...

> This should be
> wait_event(&coredev->buffer_mng_waitq, cb = get_entry());
> with get_entry like:
> struct smscore_buffer_t *get_entry(void)
> {
>   struct smscore_buffer_t *cb = NULL;
>   spin_lock_irqsave(&coredev->bufferslock, flags);
>   if (!list_empty(&coredev->buffers)) {
>     cb = (struct smscore_buffer_t *) coredev->buffers.next;
>     list_del(&cb->entry);
>   }
>   spin_unlock_irqrestore(&coredev->bufferslock, flags);
>   return cb;
> }

...
...
...


> 
> Looking at the smscore_buffer_t definition, this is really ugly since it
> relies on entry being the first in the structure. It should be
> list_first_entry(&coredev->buffers, ...) instead, cast-less.
> 
> >     list_del(&cb->entry);
> >   }
> >   spin_unlock_irqrestore(&coredev->bufferslock, flags);
> >   return cb;
> > }

thanks for the suggestions, is it on anyones TODO list or should I cook up my own patch? 
Would take a few more days till I can get at it.

BTW does anyone have knowledge how to enable IR receiver code for the smsxxx devices? 
Looks like its just the matter of setting sms_board_gpio_cfg.ir to the "right" value
- which value?

Richard

[PATCH 2.6.34] schedule inside spin_lock_irqsave

[Richard Zidlicky]

Hi,

I have done a minimaly invasive patch for the stable 2.6.34 kernel and stress-tested 
it for many hours, definitely seems to improve the behaviour.

I have left out your beautification suggestion for now, want to do more playing with
other aspects of the driver. There still seem to be issues when the device is unplugged 
while in use and such.

--- linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c.rz	2010-06-03 21:58:11.000000000 +0200
+++ linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c	2010-06-04 23:00:35.000000000 +0200
@@ -1100,31 +1100,26 @@
  *
  * @return pointer to descriptor on success, NULL on error.
  */
-struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
+
+struct smscore_buffer_t *get_entry(void)
 {
 	struct smscore_buffer_t *cb = NULL;
 	unsigned long flags;
 
-	DEFINE_WAIT(wait);
-
 	spin_lock_irqsave(&coredev->bufferslock, flags);
-
-	/* This function must return a valid buffer, since the buffer list is
-	 * finite, we check that there is an available buffer, if not, we wait
-	 * until such buffer become available.
-	 */
-
-	prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);
-
-	if (list_empty(&coredev->buffers))
-		schedule();
-
-	finish_wait(&coredev->buffer_mng_waitq, &wait);
-
+	if (!list_empty(&coredev->buffers)) {
 	cb = (struct smscore_buffer_t *) coredev->buffers.next;
 	list_del(&cb->entry);
-
+	}
 	spin_unlock_irqrestore(&coredev->bufferslock, flags);
+	return cb;
+}
+
+struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
+{
+	struct smscore_buffer_t *cb = NULL;
+
+	wait_event(coredev->buffer_mng_waitq, (cb = get_entry()));
 
 	return cb;
 }


Richard

Re: [PATCH 2.6.34] schedule inside spin_lock_irqsave

[Jiri Slaby]

On 06/06/2010 02:43 PM, Richard Zidlicky wrote:
> Hi,
> 
> I have done a minimaly invasive patch for the stable 2.6.34 kernel and stress-tested 
> it for many hours, definitely seems to improve the behaviour.
> 
> I have left out your beautification suggestion for now, want to do more playing with
> other aspects of the driver. There still seem to be issues when the device is unplugged 
> while in use and such.
> 
> --- linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c.rz	2010-06-03 21:58:11.000000000 +0200
> +++ linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c	2010-06-04 23:00:35.000000000 +0200
> @@ -1100,31 +1100,26 @@
>   *
>   * @return pointer to descriptor on success, NULL on error.
>   */
> -struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
> +
> +struct smscore_buffer_t *get_entry(void)
>  {
>  	struct smscore_buffer_t *cb = NULL;
>  	unsigned long flags;
>  
> -	DEFINE_WAIT(wait);
> -
>  	spin_lock_irqsave(&coredev->bufferslock, flags);

Sorry, maybe I'm just blind, but where is 'coredev' defined in this
scope? You probably forgot to pass it to get_entry?

How could this be compiled? Is there coredev defined globally?

> -
> -	/* This function must return a valid buffer, since the buffer list is
> -	 * finite, we check that there is an available buffer, if not, we wait
> -	 * until such buffer become available.
> -	 */
> -
> -	prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);
> -
> -	if (list_empty(&coredev->buffers))
> -		schedule();
> -
> -	finish_wait(&coredev->buffer_mng_waitq, &wait);
> -
> +	if (!list_empty(&coredev->buffers)) {
>  	cb = (struct smscore_buffer_t *) coredev->buffers.next;
>  	list_del(&cb->entry);
> -
> +	}
>  	spin_unlock_irqrestore(&coredev->bufferslock, flags);
> +	return cb;
> +}
> +
> +struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
> +{
> +	struct smscore_buffer_t *cb = NULL;
> +
> +	wait_event(coredev->buffer_mng_waitq, (cb = get_entry()));
>  
>  	return cb;
>  }

-- 
js

Re: [PATCH 2.6.34] schedule inside spin_lock_irqsave

[Richard Zidlicky]

On Sun, Jun 06, 2010 at 07:51:56PM +0200, Jiri Slaby wrote:
> On 06/06/2010 02:43 PM, Richard Zidlicky wrote:
> > Hi,
> > 
> > I have done a minimaly invasive patch for the stable 2.6.34 kernel and stress-tested 
> > it for many hours, definitely seems to improve the behaviour.
> > 
> > I have left out your beautification suggestion for now, want to do more playing with
> > other aspects of the driver. There still seem to be issues when the device is unplugged 
> > while in use and such.
> > 
> > --- linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c.rz	2010-06-03 21:58:11.000000000 +0200
> > +++ linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c	2010-06-04 23:00:35.000000000 +0200
> > @@ -1100,31 +1100,26 @@
> >   *
> >   * @return pointer to descriptor on success, NULL on error.
> >   */
> > -struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
> > +
> > +struct smscore_buffer_t *get_entry(void)
> >  {
> >  	struct smscore_buffer_t *cb = NULL;
> >  	unsigned long flags;
> >  
> > -	DEFINE_WAIT(wait);
> > -
> >  	spin_lock_irqsave(&coredev->bufferslock, flags);
> 
> Sorry, maybe I'm just blind, but where is 'coredev' defined in this
> scope? You probably forgot to pass it to get_entry?
> 
> How could this be compiled? Is there coredev defined globally?

good catch. I think it failed and despite a different kernel id the old module was
loaded.

Here is the new version, this time lightly tested

--- linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c.rz	2010-06-03 21:58:11.000000000 +0200
+++ linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c	2010-06-07 14:32:06.000000000 +0200
@@ -1100,31 +1100,26 @@
  *
  * @return pointer to descriptor on success, NULL on error.
  */
-struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
+
+struct smscore_buffer_t *get_entry(struct smscore_device_t *coredev)
 {
 	struct smscore_buffer_t *cb = NULL;
 	unsigned long flags;
 
-	DEFINE_WAIT(wait);
-
 	spin_lock_irqsave(&coredev->bufferslock, flags);
-
-	/* This function must return a valid buffer, since the buffer list is
-	 * finite, we check that there is an available buffer, if not, we wait
-	 * until such buffer become available.
-	 */
-
-	prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);
-
-	if (list_empty(&coredev->buffers))
-		schedule();
-
-	finish_wait(&coredev->buffer_mng_waitq, &wait);
-
+	if (!list_empty(&coredev->buffers)) {
 	cb = (struct smscore_buffer_t *) coredev->buffers.next;
 	list_del(&cb->entry);
-
+	}
 	spin_unlock_irqrestore(&coredev->bufferslock, flags);
+	return cb;
+}
+
+struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
+{
+	struct smscore_buffer_t *cb = NULL;
+
+	wait_event(coredev->buffer_mng_waitq, (cb = get_entry(coredev)));
 
 	return cb;
 }


Richard

Re: [PATCH 2.6.34] schedule inside spin_lock_irqsave

[Mauro Carvalho Chehab]

Em 07-06-2010 10:00, Richard Zidlicky escreveu:
> On Sun, Jun 06, 2010 at 07:51:56PM +0200, Jiri Slaby wrote:
>> On 06/06/2010 02:43 PM, Richard Zidlicky wrote:
>>> Hi,
>>>
>>> I have done a minimaly invasive patch for the stable 2.6.34 kernel and stress-tested 
>>> it for many hours, definitely seems to improve the behaviour.
>>>
>>> I have left out your beautification suggestion for now, want to do more playing with
>>> other aspects of the driver. There still seem to be issues when the device is unplugged 
>>> while in use and such.
>>>
>>> --- linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c.rz	2010-06-03 21:58:11.000000000 +0200
>>> +++ linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c	2010-06-04 23:00:35.000000000 +0200
>>> @@ -1100,31 +1100,26 @@
>>>   *
>>>   * @return pointer to descriptor on success, NULL on error.
>>>   */
>>> -struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
>>> +
>>> +struct smscore_buffer_t *get_entry(void)
>>>  {
>>>  	struct smscore_buffer_t *cb = NULL;
>>>  	unsigned long flags;
>>>  
>>> -	DEFINE_WAIT(wait);
>>> -
>>>  	spin_lock_irqsave(&coredev->bufferslock, flags);
>>
>> Sorry, maybe I'm just blind, but where is 'coredev' defined in this
>> scope? You probably forgot to pass it to get_entry?
>>
>> How could this be compiled? Is there coredev defined globally?
> 
> good catch. I think it failed and despite a different kernel id the old module was
> loaded.
> 
> Here is the new version, this time lightly tested

Could you please fix the indentation and send your SOB for this patch?

Cheers,
Mauro.
> 
> --- linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c.rz	2010-06-03 21:58:11.000000000 +0200
> +++ linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c	2010-06-07 14:32:06.000000000 +0200
> @@ -1100,31 +1100,26 @@
>   *
>   * @return pointer to descriptor on success, NULL on error.
>   */
> -struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
> +
> +struct smscore_buffer_t *get_entry(struct smscore_device_t *coredev)
>  {
>  	struct smscore_buffer_t *cb = NULL;
>  	unsigned long flags;
>  
> -	DEFINE_WAIT(wait);
> -
>  	spin_lock_irqsave(&coredev->bufferslock, flags);
> -
> -	/* This function must return a valid buffer, since the buffer list is
> -	 * finite, we check that there is an available buffer, if not, we wait
> -	 * until such buffer become available.
> -	 */
> -
> -	prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);
> -
> -	if (list_empty(&coredev->buffers))
> -		schedule();
> -
> -	finish_wait(&coredev->buffer_mng_waitq, &wait);
> -
> +	if (!list_empty(&coredev->buffers)) {
>  	cb = (struct smscore_buffer_t *) coredev->buffers.next;
>  	list_del(&cb->entry);
> -
> +	}
>  	spin_unlock_irqrestore(&coredev->bufferslock, flags);
> +	return cb;
> +}
> +
> +struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
> +{
> +	struct smscore_buffer_t *cb = NULL;
> +
> +	wait_event(coredev->buffer_mng_waitq, (cb = get_entry(coredev)));
>  
>  	return cb;
>  }
> 
> 
> Richard
> --
> 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