vchan helper broken wrt locking

vchan helper broken wrt locking

[Sascha Hauer]

Hi All,

vc->desc_free() used to be called in non atomic context which makes
sense to me. This changed over time and now vc->desc_free() is sometimes
called in atomic context and sometimes not.

The story starts with 13bb26ae8850 ("dmaengine: virt-dma: don't always
free descriptor upon completion"). This introduced a vc->desc_allocated
list which is mostly handled with the lock held, except in vchan_complete().
vchan_complete() moves the completed descs onto a separate list for the sake
of iterating over that list without the lock held allowing to call
vc->desc_free() without lock. 13bb26ae8850 changes this to:

@@ -83,8 +110,10 @@ static void vchan_complete(unsigned long arg)
                cb_data = vd->tx.callback_param;
 
                list_del(&vd->node);
-
-               vc->desc_free(vd);
+               if (dmaengine_desc_test_reuse(&vd->tx))
+                       list_add(&vd->node, &vc->desc_allocated);
+               else
+                       vc->desc_free(vd);

vc->desc_free() is still called without lock, but the list operation is done
without locking as well which is wrong.

Now with 6af149d2b142 ("dmaengine: virt-dma: Add helper to free/reuse a
descriptor") the hunk above was moved to a separate function
(vchan_vdesc_fini()). With 1c7f072d94e8 ("dmaengine: virt-dma: Support for
race free transfer termination") the helper is started to be called with
lock held resulting in vc->desc_free() being called under the lock as
well. It is still called from vchan_complete() without lock.

I think vc->desc_free() being called under a spin_lock is unfortunate as
the i.MX SDMA driver does a dma_free_coherent() there which is required
to be called with interrupts enabled.

I am not sure where to go from here hence I'm writing this mail. Do we
agree that vc->desc_free() should be called without lock?

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: vchan helper broken wrt locking

[Robert Jarzmik]

Sascha Hauer <s.hauer@pengutronix.de> writes:

> Hi All,

Hi Sascha,

The short story : from my understanding, vc->desc_free() shouldn't be called
with vd->lock held. The locking of callbacks called by virt-dma shouldn't be
handled by virt-dma, but be a part of the dma driver's architecture rather than
forced by virt-dma.

The long story :

This is what I remember and my understanding for the virt-dma :
 - virt_dma_chan->lock protects only :
   - all lists within virt_dma_chan
   - virt_dma_desc->node, which is on one of the above lists

As a consequence of this statement, this lock shouldn't be held for other parts
of the code, and therefore not while calling vc->desc_free().

I base the above statement on my "former" understanding, and that is a code I
dealt with years ago, so I might be a little rusty here and there.  The original
code Russell wrote states in virt-dma.h :
 - in structure "struct_dma_desc"
   The "protected by vc.lock" is just for the "node" field
 - in structure "struct virt_dma_chan"
   The "protected by vc.lock" is just above the 4 lists (originally 3, and the I
   added one more)

Now Peter might have more insight, as he has modified the code more recently
that I did.

Cheers.

--
Robert

PS: And yes, there are occurrences of list manipulations in virt-dma that should
be protected by vc.lock as you pointed out.

Re: vchan helper broken wrt locking

[Peter Ujfalusi]

Hi Sascha,

On 03/12/2019 13.50, Sascha Hauer wrote:
> Hi All,
> 
> vc->desc_free() used to be called in non atomic context which makes
> sense to me. This changed over time and now vc->desc_free() is sometimes
> called in atomic context and sometimes not.
> 
> The story starts with 13bb26ae8850 ("dmaengine: virt-dma: don't always
> free descriptor upon completion"). This introduced a vc->desc_allocated
> list which is mostly handled with the lock held, except in vchan_complete().
> vchan_complete() moves the completed descs onto a separate list for the sake
> of iterating over that list without the lock held allowing to call
> vc->desc_free() without lock. 13bb26ae8850 changes this to:
> 
> @@ -83,8 +110,10 @@ static void vchan_complete(unsigned long arg)
>                 cb_data = vd->tx.callback_param;
>  
>                 list_del(&vd->node);
> -
> -               vc->desc_free(vd);
> +               if (dmaengine_desc_test_reuse(&vd->tx))
> +                       list_add(&vd->node, &vc->desc_allocated);
> +               else
> +                       vc->desc_free(vd);
> 
> vc->desc_free() is still called without lock, but the list operation is done
> without locking as well which is wrong.

Hrm, yes all list operation against desc_* should be protected by the
lock, it is a miss.

> Now with 6af149d2b142 ("dmaengine: virt-dma: Add helper to free/reuse a
> descriptor") the hunk above was moved to a separate function
> (vchan_vdesc_fini()). With 1c7f072d94e8 ("dmaengine: virt-dma: Support for
> race free transfer termination") the helper is started to be called with
> lock held resulting in vc->desc_free() being called under the lock as
> well. It is still called from vchan_complete() without lock.

Right.
I think the most elegant way to fix this would be to introduce a new
list_head in virt_dma_chan, let's name it desc_terminated.

We would add the descriptor to this within vchan_terminate_vdesc() (lock
is held).
In vchan_synchronize() we would
list_splice_tail_init(&vc->desc_terminated, &head);
with the lock held and outside of the lock we free them up.

So we would put the terminated descs to the new list and free them up in
synchronize.

This way the vchan_vdesc_fini() would be only called without the lock held.

> I think vc->desc_free() being called under a spin_lock is unfortunate as
> the i.MX SDMA driver does a dma_free_coherent() there which is required
> to be called with interrupts enabled.

In the in review k3-udma driver I use dma_pool or dma_alloc_coherent in
mixed mode depending on the type of the channel.

I did also see the same issue and what I ended up doing is to have
desc_to_purge list and udma_purge_desc_work()
in udma_desc_free() if the descriptor is from the dma_pool, I free it
right away, if it needs dma_free_coherent() then I put it to the
desc_to_purge list and schedule the purge worker to deal with them at a
later time.

In this driver I don't use vchan_terminate_vdesc() because of this.

> I am not sure where to go from here hence I'm writing this mail. Do we
> agree that vc->desc_free() should be called without lock?

I think it should be called without the lock held.

> 
> Sascha
> 
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: vchan helper broken wrt locking

[Vinod Koul]

On 04-12-19, 13:47, Peter Ujfalusi wrote:
> Hi Sascha,
> 
> On 03/12/2019 13.50, Sascha Hauer wrote:
> > Hi All,
> > 
> > vc->desc_free() used to be called in non atomic context which makes
> > sense to me. This changed over time and now vc->desc_free() is sometimes
> > called in atomic context and sometimes not.
> > 
> > The story starts with 13bb26ae8850 ("dmaengine: virt-dma: don't always
> > free descriptor upon completion"). This introduced a vc->desc_allocated
> > list which is mostly handled with the lock held, except in vchan_complete().
> > vchan_complete() moves the completed descs onto a separate list for the sake
> > of iterating over that list without the lock held allowing to call
> > vc->desc_free() without lock. 13bb26ae8850 changes this to:
> > 
> > @@ -83,8 +110,10 @@ static void vchan_complete(unsigned long arg)
> >                 cb_data = vd->tx.callback_param;
> >  
> >                 list_del(&vd->node);
> > -
> > -               vc->desc_free(vd);
> > +               if (dmaengine_desc_test_reuse(&vd->tx))
> > +                       list_add(&vd->node, &vc->desc_allocated);
> > +               else
> > +                       vc->desc_free(vd);
> > 
> > vc->desc_free() is still called without lock, but the list operation is done
> > without locking as well which is wrong.
> 
> Hrm, yes all list operation against desc_* should be protected by the
> lock, it is a miss.
> 
> > Now with 6af149d2b142 ("dmaengine: virt-dma: Add helper to free/reuse a
> > descriptor") the hunk above was moved to a separate function
> > (vchan_vdesc_fini()). With 1c7f072d94e8 ("dmaengine: virt-dma: Support for
> > race free transfer termination") the helper is started to be called with
> > lock held resulting in vc->desc_free() being called under the lock as
> > well. It is still called from vchan_complete() without lock.
> 
> Right.
> I think the most elegant way to fix this would be to introduce a new
> list_head in virt_dma_chan, let's name it desc_terminated.
> 
> We would add the descriptor to this within vchan_terminate_vdesc() (lock
> is held).
> In vchan_synchronize() we would
> list_splice_tail_init(&vc->desc_terminated, &head);
> with the lock held and outside of the lock we free them up.
> 
> So we would put the terminated descs to the new list and free them up in
> synchronize.
> 
> This way the vchan_vdesc_fini() would be only called without the lock held.

This makes sense to me as well. I would like the vc->desc_free() to be
always called with lock and in non-atomic context.
> 
> > I think vc->desc_free() being called under a spin_lock is unfortunate as
> > the i.MX SDMA driver does a dma_free_coherent() there which is required
> > to be called with interrupts enabled.
> 
> In the in review k3-udma driver I use dma_pool or dma_alloc_coherent in
> mixed mode depending on the type of the channel.
> 
> I did also see the same issue and what I ended up doing is to have
> desc_to_purge list and udma_purge_desc_work()
> in udma_desc_free() if the descriptor is from the dma_pool, I free it
> right away, if it needs dma_free_coherent() then I put it to the
> desc_to_purge list and schedule the purge worker to deal with them at a
> later time.
> 
> In this driver I don't use vchan_terminate_vdesc() because of this.
> 
> > I am not sure where to go from here hence I'm writing this mail. Do we
> > agree that vc->desc_free() should be called without lock?
> 
> I think it should be called without the lock held.
> 
> > 
> > Sascha
> > 
> > 
> 
> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

-- 
~Vinod

Re: vchan helper broken wrt locking

[Sascha Hauer]

On Wed, Dec 04, 2019 at 01:47:03PM +0200, Peter Ujfalusi wrote:
> Hi Sascha,
> 
> On 03/12/2019 13.50, Sascha Hauer wrote:
> > Hi All,
> > 
> > vc->desc_free() used to be called in non atomic context which makes
> > sense to me. This changed over time and now vc->desc_free() is sometimes
> > called in atomic context and sometimes not.
> > 
> > The story starts with 13bb26ae8850 ("dmaengine: virt-dma: don't always
> > free descriptor upon completion"). This introduced a vc->desc_allocated
> > list which is mostly handled with the lock held, except in vchan_complete().
> > vchan_complete() moves the completed descs onto a separate list for the sake
> > of iterating over that list without the lock held allowing to call
> > vc->desc_free() without lock. 13bb26ae8850 changes this to:
> > 
> > @@ -83,8 +110,10 @@ static void vchan_complete(unsigned long arg)
> >                 cb_data = vd->tx.callback_param;
> >  
> >                 list_del(&vd->node);
> > -
> > -               vc->desc_free(vd);
> > +               if (dmaengine_desc_test_reuse(&vd->tx))
> > +                       list_add(&vd->node, &vc->desc_allocated);
> > +               else
> > +                       vc->desc_free(vd);
> > 
> > vc->desc_free() is still called without lock, but the list operation is done
> > without locking as well which is wrong.
> 
> Hrm, yes all list operation against desc_* should be protected by the
> lock, it is a miss.
> 
> > Now with 6af149d2b142 ("dmaengine: virt-dma: Add helper to free/reuse a
> > descriptor") the hunk above was moved to a separate function
> > (vchan_vdesc_fini()). With 1c7f072d94e8 ("dmaengine: virt-dma: Support for
> > race free transfer termination") the helper is started to be called with
> > lock held resulting in vc->desc_free() being called under the lock as
> > well. It is still called from vchan_complete() without lock.
> 
> Right.
> I think the most elegant way to fix this would be to introduce a new
> list_head in virt_dma_chan, let's name it desc_terminated.
> 
> We would add the descriptor to this within vchan_terminate_vdesc() (lock
> is held).
> In vchan_synchronize() we would
> list_splice_tail_init(&vc->desc_terminated, &head);
> with the lock held and outside of the lock we free them up.
> 
> So we would put the terminated descs to the new list and free them up in
> synchronize.
> 
> This way the vchan_vdesc_fini() would be only called without the lock held.

Ok, this sounds like a plan. I'll try and make up a patch for this.

Thanks,
  Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |