RE: [RFC v3 4/7] dmaengine: Add slave DMA interface

[Nelson, Shannon]

>From: Haavard Skinnemoen [mailto:hskinnemoen@xxxxxxxxxxxxxxxx] 
>Sent: Friday, February 15, 2008 1:53 AM
>To: Haavard Skinnemoen
>Cc: Williams, Dan J; linux-kernel@xxxxxxxxxxxxxxx; Nelson, 
>Shannon; David Brownell; kernel@xxxxxxxxxxxxxx; Francis 
>Moreau; Paul Mundt; Vladimir A. Barinov; Pierre Ossman
>Subject: Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
>
>On Wed, 13 Feb 2008 20:24:02 +0100
>Haavard Skinnemoen <hskinnemoen@xxxxxxxxx> wrote:
>
>> But looking at your latest patch series, I guess we can use the new
>> "next" field instead. It's not like we really need the full
>> capabilities of list_head.
>
>On second thought, if we do this, we would be using the "next" field in
>an undocumented way. It is currently documented as follows:
>
> * @next: at completion submit this descriptor
>
>But that's not how we're going to use it when doing slave transfers:
>We're using it to keep track of all the descriptors that have already
>been submitted.
>
>I think it would actually be better to go the other way and have the
>async_tx API extend the descriptor as well for its private fields. That
>way, we get the pointer we need, but we can document it differently.
>
>So how about we do something along the lines of the patch below? We
>need to update all the users too of course, but apart from making the
>dma_slave_descriptor struct smaller, I don't think it will change the
>actual memory layout at all.
>
>Haavard
>
>diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>index 9bb3407..abaf324 100644
>--- a/include/linux/dmaengine.h
>+++ b/include/linux/dmaengine.h
>@@ -267,8 +267,7 @@ struct dma_client {
> 
> typedef void (*dma_async_tx_callback)(void *dma_async_param);
> /**
>- * struct dma_async_tx_descriptor - async transaction descriptor
>- * ---dma generic offload fields---
>+ * struct dma_descriptor - generic dma offload descriptor
>  * @cookie: tracking cookie for this transaction, set to -EBUSY if
>  *	this tx is sitting on a dependency list
>  * @ack: the descriptor can not be reused until the client 
>acknowledges
>@@ -280,12 +279,8 @@ typedef void 
>(*dma_async_tx_callback)(void *dma_async_param);
>  * @tx_submit: set the prepared descriptor(s) to be executed 
>by the engine
>  * @callback: routine to call after this operation is complete
>  * @callback_param: general parameter to pass to the callback routine
>- * ---async_tx api specific fields---
>- * @next: at completion submit this descriptor
>- * @parent: pointer to the next level up in the dependency chain
>- * @lock: protect the parent and next pointers
>  */
>-struct dma_async_tx_descriptor {
>+struct dma_descriptor {
> 	dma_cookie_t cookie;
> 	int ack;
> 	dma_addr_t phys;
>@@ -294,6 +289,17 @@ struct dma_async_tx_descriptor {
> 	dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx);
> 	dma_async_tx_callback callback;
> 	void *callback_param;
>+};
>+
>+/**
>+ * struct dma_async_tx_descriptor - async transaction descriptor
>+ * @dma: generic dma descriptor
>+ * @next: at completion submit this descriptor
>+ * @parent: pointer to the next level up in the dependency chain
>+ * @lock: protect the parent and next pointers
>+ */
>+struct dma_async_tx_descriptor {
>+	struct dma_descriptor dma;
> 	struct dma_async_tx_descriptor *next;
> 	struct dma_async_tx_descriptor *parent;
> 	spinlock_t lock;
>@@ -301,13 +307,13 @@ struct dma_async_tx_descriptor {
> 
> /**
>  * struct dma_slave_descriptor - extended DMA descriptor for slave DMA
>- * @async_tx: async transaction descriptor
>- * @client_node: for use by the client, for example when operating on
>- *	scatterlists.
>+ * @dma: generic dma descriptor
>+ * @next: for use by the client, for example to keep track of
>+ *	submitted descriptors when dealing with scatterlists.
>  */
> struct dma_slave_descriptor {
>-	struct dma_async_tx_descriptor txd;
>-	struct list_head client_node;
>+	struct dma_descriptor dma;
>+	struct dma_slave_descriptor *next;
> };
> 
> /**
>

I'll jump in here briefly - I'm okay with the direction this is going,
but I want to be protective of ioatdma performance.  As used in struct
ioat_desc_sw, the cookie and ack elements end up very close to the end
of a cache line and I'd like them to not get pushed out across the
boundry.  I don't think this proposal changes the layout, I'm just
bringing up my concern.

Selfishly,
sln
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/