From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:55884)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <benh@kernel.crashing.org>) id 1ShuOt-0006EZ-Pv
	for qemu-devel@nongnu.org; Thu, 21 Jun 2012 23:19:12 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <benh@kernel.crashing.org>) id 1ShuOs-0006Fv-2w
	for qemu-devel@nongnu.org; Thu, 21 Jun 2012 23:19:11 -0400
Received: from gate.crashing.org ([63.228.1.57]:52858)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <benh@kernel.crashing.org>) id 1ShuOr-0006Fn-Qm
	for qemu-devel@nongnu.org; Thu, 21 Jun 2012 23:19:10 -0400
Message-ID: <1340335138.16104.19.camel@pasglop>
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Fri, 22 Jun 2012 13:18:58 +1000
In-Reply-To: <4FE23FAC.80007@codemonkey.ws>
References: <1340087992-2399-1-git-send-email-benh@kernel.crashing.org>
	<1340087992-2399-10-git-send-email-benh@kernel.crashing.org>
	<4FE23FAC.80007@codemonkey.ws>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Mime-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH 09/13] iommu: Add facility to cancel in-use
 dma memory maps
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Anthony Liguori <anthony@codemonkey.ws>
Cc: qemu-devel@nongnu.org, David Gibson <david@gibson.dropbear.id.au>

On Wed, 2012-06-20 at 16:25 -0500, Anthony Liguori wrote:

> > +static void dma_aio_cancel(BlockDriverAIOCB *acb)
> > +{
> > +    DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common);
> > +
> > +    trace_dma_aio_cancel(dbs);
> > +
> > +    if (dbs->acb) {
> > +        BlockDriverAIOCB *acb = dbs->acb;
> > +        dbs->acb = NULL;
> > +        dbs->in_cancel = true;
> > +        bdrv_aio_cancel(acb);
> > +        dbs->in_cancel = false;
> > +    }
> > +    dbs->common.cb = NULL;
> > +    dma_complete(dbs, 0);
> 
> So this cancellation stuff is hopelessly broken
> 
> It's simply not possible to fully cancel pending DMA in a synchronous callback.
> 
> Indeed, bdrv_aio_cancel ends up having a nasty little loop in it:

Yes, it's broken. Note that the patch didn't add the above function,
only moved it around.

In any case, I've decided to just drop that patch completely from the
series. IE. I'm not adding the dma_memory_map_with_cancel() variant,
there's no point since:

 - Nothing will call the cancel callback today and possibly for a while

 - Nothing passes a cancel callback other than the bdrv stuff and that
callback is hopelessly broken as you mentioned above.

So there's just no point. We will add an optional cancel callback again
later when we eventually decide to sort that problem out properly, it
will be an asynchronous cancel, ie, just "initiate" the cancellation,
and I'll probably add that as an argument to the normal dma_memory_map()
(adding NULL to all callers that don't care) at that point.

For now, let's not add a known to be broken and unused interface.

Cheers,
Ben.