Asynchronous raid6 acceleration (part 1 of 3)
Subsystems (7)
v2 cover Asynchronous raid6 acceleration (part 1 of 3)
This series extends the async_tx api to include support for offloading raid6 operations to hardware. Part 2 covers updates to the iop-adma driver to enable raid6. Part 3 covers the changes to md/raid6 to use the async_tx api for its raid6 computations.
A merge of these 3 topic series is available via the raid6 branch of async_tx.git:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/async_tx.git raid6
There are several changes since v1 of the patch set[1]. The primary updates are:
1/ Address H. Peter Anvin's comments around the asynchronous raid6 recovery
implementation. It has been rewritten to more closely model the existing recovery implementation [2].
2/ Address Andre Noll's comment concerning the excessive number of arguments
passed to the async_* routines. Common parameters are now passed in a structure.
3/ Address Neil Brown's comment that we should solve the "kernel stack space
limits the size of an array" problem with pre-allocated buffers. stripe_head objects now carry a kmalloc'd 'scribble' buffer relieving the stack pressure of raid computations.
4/ Various updates to get the code to pass the new raid6 recovery self test
Please review.
Thanks,
Dan
[1] http://marc.info/?l=linux-raid&m=123740421725422&w=2
[2] http://marc.info/?l=linux-raid&m=124096283425451&w=2
---
Dan Williams (11):
async_tx: rename zero_sum to val
async_tx: kill ASYNC_TX_DEP_ACK flag
async_tx: structify submission arguments, add scribble
async_xor: permit callers to pass in a 'dma/page scribble' region md/raid5: add scribble region for buffer lists
async_tx: add sum check flags
async_tx: kill needless module_{init|exit}
async_tx: add support for asynchronous GF multiplication
async_tx: add support for asynchronous RAID6 recovery operations
dmatest: add pq support
async_tx: raid6 recovery self test
arch/arm/include/asm/hardware/iop3xx-adma.h | 5 arch/arm/mach-iop13xx/include/mach/adma.h | 12 - arch/arm/mach-iop13xx/setup.c | 10 - arch/arm/plat-iop/adma.c | 2 crypto/async_tx/Kconfig | 9 + crypto/async_tx/Makefile | 3 crypto/async_tx/async_memcpy.c | 34 +- crypto/async_tx/async_memset.c | 36 +- crypto/async_tx/async_pq.c | 399 +++++++++++++++++++++++++++ crypto/async_tx/async_raid6_recov.c | 292 ++++++++++++++++++++ crypto/async_tx/async_tx.c | 53 +--- crypto/async_tx/async_xor.c | 168 +++++------ crypto/async_tx/raid6test.c | 212 ++++++++++++++ drivers/dma/Kconfig | 2 drivers/dma/dmaengine.c | 8 - drivers/dma/dmatest.c | 26 ++ drivers/dma/iop-adma.c | 40 +-- drivers/md/Kconfig | 13 + drivers/md/raid5.c | 123 ++++++-- drivers/md/raid5.h | 10 + include/linux/async_tx.h | 103 +++++-- include/linux/dmaengine.h | 87 +++++- 22 files changed, 1347 insertions(+), 300 deletions(-) create mode 100644 crypto/async_tx/async_pq.c create mode 100644 crypto/async_tx/async_raid6_recov.c create mode 100644 crypto/async_tx/raid6test.c
PATCH v2 1/11 async_tx: rename zero_sum to val
'zero_sum' does not properly describe the operation of generating parity and checking that it validates against an existing buffer. Change the name of the operation to 'val' (for 'validate'). This is in anticipation of the p+q case where it is a requirement to identify the target parity buffers separately from the source buffers, because the target parity buffers will not have corresponding pq coefficients.
--- arch/arm/mach-iop13xx/setup.c | 8 ++++---- arch/arm/plat-iop/adma.c | 2 +- crypto/async_tx/async_xor.c | 16 ++++++++-------- drivers/dma/dmaengine.c | 4 ++-- drivers/dma/iop-adma.c | 38 +++++++++++++++++++------------------- drivers/md/raid5.c | 2 +- include/linux/async_tx.h | 2 +- include/linux/dmaengine.h | 8 ++++---- 8 files changed, 40 insertions(+), 40 deletions(-)
arch/arm/mach-iop13xx/setup.c +4 -4
@@ -478,7 +478,7 @@ void __init iop13xx_platform_init(void)dma_cap_set (DMA_MEMCPY , plat_data->cap_mask);dma_cap_set (DMA_XOR , plat_data->cap_mask);dma_cap_set (DMA_DUAL_XOR , plat_data->cap_mask); -dma_cap_set (DMA_ZERO_SUM , plat_data->cap_mask); +dma_cap_set (DMA_XOR_VAL , plat_data->cap_mask);dma_cap_set (DMA_MEMSET , plat_data->cap_mask);dma_cap_set (DMA_MEMCPY_CRC32C , plat_data->cap_mask);dma_cap_set (DMA_INTERRUPT , plat_data->cap_mask);
@@ -490,7 +490,7 @@ void __init iop13xx_platform_init(void)dma_cap_set (DMA_MEMCPY , plat_data->cap_mask);dma_cap_set (DMA_XOR , plat_data->cap_mask);dma_cap_set (DMA_DUAL_XOR , plat_data->cap_mask); -dma_cap_set (DMA_ZERO_SUM , plat_data->cap_mask); +dma_cap_set (DMA_XOR_VAL , plat_data->cap_mask);dma_cap_set (DMA_MEMSET , plat_data->cap_mask);dma_cap_set (DMA_MEMCPY_CRC32C , plat_data->cap_mask);dma_cap_set (DMA_INTERRUPT , plat_data->cap_mask);
@@ -502,13 +502,13 @@ void __init iop13xx_platform_init(void)dma_cap_set (DMA_MEMCPY , plat_data->cap_mask);dma_cap_set (DMA_XOR , plat_data->cap_mask);dma_cap_set (DMA_DUAL_XOR , plat_data->cap_mask); -dma_cap_set (DMA_ZERO_SUM , plat_data->cap_mask); +dma_cap_set (DMA_XOR_VAL , plat_data->cap_mask);dma_cap_set (DMA_MEMSET , plat_data->cap_mask);dma_cap_set (DMA_MEMCPY_CRC32C , plat_data->cap_mask);dma_cap_set (DMA_INTERRUPT , plat_data->cap_mask);dma_cap_set (DMA_PQ_XOR , plat_data->cap_mask);dma_cap_set (DMA_PQ_UPDATE , plat_data->cap_mask); -dma_cap_set (DMA_PQ_ZERO_SUM , plat_data->cap_mask); +dma_cap_set (DMA_PQ_VAL , plat_data->cap_mask);break ; } }
arch/arm/plat-iop/adma.c +1 -1
@@ -198,7 +198,7 @@ static int __init iop3xx_adma_cap_init(void)dma_cap_set (DMA_INTERRUPT , iop3xx_aau_data.cap_mask);#else dma_cap_set (DMA_XOR , iop3xx_aau_data.cap_mask); -dma_cap_set (DMA_ZERO_SUM , iop3xx_aau_data.cap_mask); +dma_cap_set (DMA_XOR_VAL , iop3xx_aau_data.cap_mask);dma_cap_set (DMA_MEMSET , iop3xx_aau_data.cap_mask);dma_cap_set (DMA_INTERRUPT , iop3xx_aau_data.cap_mask);#endif
crypto/async_tx/async_xor.c +8 -8
@@ -222,7 +222,7 @@ static int page_is_zero(struct page *p, unsigned int offset, size_t len) }/** -* async_xor_zero_sum - attempt a xor parity check with a dma engine. +* async_xor_val - attempt a xor parity check with a dma engine. * @dest: destination page used if the xor is performed synchronously * @src_list: array of source pages. The dest page must be listed as a source * at index zero. The contents of this array may be overwritten.
@@ -236,13 +236,13 @@ static int page_is_zero(struct page *p, unsigned int offset, size_t len)* @cb_param: parameter to pass to the callback routine */ struct dma_async_tx_descriptor * -async_xor_zero_sum (struct page *dest,struct page **src_list, +async_xor_val (struct page *dest,struct page **src_list,unsigned int offset,int src_cnt,size_t len,u32 *result,enum async_tx_flags flags,struct dma_async_tx_descriptor *depend_tx,dma_async_tx_callback cb_fn,void *cb_param) { -struct dma_chan *chan =async_tx_find_channel (depend_tx,DMA_ZERO_SUM , +struct dma_chan *chan =async_tx_find_channel (depend_tx,DMA_XOR_VAL , &dest,1 , src_list, src_cnt, len);struct dma_device *device = chan ? chan->device :NULL ;
@@ -261,15 +261,15 @@ async_xor_zero_sum(struct page *dest, struct page **src_list, dma_src[i] =dma_map_page (device->dev, src_list[i], offset, len,DMA_TO_DEVICE ); - tx = device->device_prep_dma_zero_sum (chan, dma_src, src_cnt, - len, result, - dma_prep_flags); + tx = device->device_prep_dma_xor_val (chan, dma_src, src_cnt, + len, result, + dma_prep_flags);if (unlikely (!tx)) {async_tx_quiesce (&depend_tx);while (!tx) {dma_async_issue_pending (chan); - tx = device->device_prep_dma_zero_sum (chan, + tx = device->device_prep_dma_xor_val (chan, dma_src, src_cnt, len, result, dma_prep_flags); }
@@ -296,7 +296,7 @@ async_xor_zero_sum(struct page *dest, struct page **src_list,return tx; } -EXPORT_SYMBOL_GPL (async_xor_zero_sum); +EXPORT_SYMBOL_GPL (async_xor_val);static int __init async_xor_init (void ) {
drivers/dma/dmaengine.c +2 -2
@@ -644,8 +644,8 @@ int dma_async_device_register(struct dma_device *device) !device->device_prep_dma_memcpy);BUG_ON (dma_has_cap (DMA_XOR , device->cap_mask) && !device->device_prep_dma_xor); -BUG_ON (dma_has_cap (DMA_ZERO_SUM , device->cap_mask) && - !device->device_prep_dma_zero_sum); +BUG_ON (dma_has_cap (DMA_XOR_VAL , device->cap_mask) && + !device->device_prep_dma_xor_val);BUG_ON (dma_has_cap (DMA_MEMSET , device->cap_mask) && !device->device_prep_dma_memset); BUG_ON(dma_has_cap(DMA_INTERRUPT, device->cap_mask) &&
drivers/dma/iop-adma.c +19 -19
@@ -660,9 +660,9 @@ iop_adma_prep_dma_xor(struct dma_chan *chan, dma_addr_t dma_dest, }static struct dma_async_tx_descriptor * -iop_adma_prep_dma_zero_sum (struct dma_chan *chan,dma_addr_t *dma_src, -unsigned int src_cnt,size_t len,u32 *result, -unsigned long flags) +iop_adma_prep_dma_xor_val (struct dma_chan *chan,dma_addr_t *dma_src, +unsigned int src_cnt,size_t len,u32 *result, +unsigned long flags) {struct iop_adma_chan *iop_chan =to_iop_adma_chan (chan);struct iop_adma_desc_slot *sw_desc, *grp_start;
@@ -906,7 +906,7 @@ out:#define IOP_ADMA_NUM_SRC_TEST 4/* must be <= 15 */ static int __devinit -iop_adma_xor_zero_sum_self_test (struct iop_adma_device *device) +iop_adma_xor_val_self_test (struct iop_adma_device *device) {int i, src_idx;struct page *dest;
@@ -1002,7 +1002,7 @@ iop_adma_xor_zero_sum_self_test(struct iop_adma_device *device) PAGE_SIZE, DMA_TO_DEVICE);/* skip zero sum if the capability is not present */ -if (!dma_has_cap (DMA_ZERO_SUM , dma_chan->device->cap_mask)) +if (!dma_has_cap (DMA_XOR_VAL , dma_chan->device->cap_mask)) goto free_resources;/* zero sum the sources with the destintation page */
@@ -1016,10 +1016,10 @@ iop_adma_xor_zero_sum_self_test(struct iop_adma_device *device) dma_srcs[i] =dma_map_page (dma_chan->device->dev, zero_sum_srcs[i],0 ,PAGE_SIZE ,DMA_TO_DEVICE ); - tx =iop_adma_prep_dma_zero_sum (dma_chan, dma_srcs, -IOP_ADMA_NUM_SRC_TEST +1 ,PAGE_SIZE , - &zero_sum_result, -DMA_PREP_INTERRUPT |DMA_CTRL_ACK ); + tx =iop_adma_prep_dma_xor_val (dma_chan, dma_srcs, +IOP_ADMA_NUM_SRC_TEST +1 ,PAGE_SIZE , + &zero_sum_result, +DMA_PREP_INTERRUPT |DMA_CTRL_ACK ); cookie =iop_adma_tx_submit (tx);iop_adma_issue_pending (dma_chan);
@@ -1072,10 +1072,10 @@ iop_adma_xor_zero_sum_self_test(struct iop_adma_device *device) dma_srcs[i] =dma_map_page (dma_chan->device->dev, zero_sum_srcs[i],0 ,PAGE_SIZE ,DMA_TO_DEVICE ); - tx =iop_adma_prep_dma_zero_sum (dma_chan, dma_srcs, -IOP_ADMA_NUM_SRC_TEST +1 ,PAGE_SIZE , - &zero_sum_result, -DMA_PREP_INTERRUPT |DMA_CTRL_ACK ); + tx =iop_adma_prep_dma_xor_val (dma_chan, dma_srcs, +IOP_ADMA_NUM_SRC_TEST +1 ,PAGE_SIZE , + &zero_sum_result, +DMA_PREP_INTERRUPT |DMA_CTRL_ACK ); cookie =iop_adma_tx_submit (tx);iop_adma_issue_pending (dma_chan);
@@ -1192,9 +1192,9 @@ static int __devinit iop_adma_probe(struct platform_device *pdev) dma_dev->max_xor =iop_adma_get_max_xor (); dma_dev->device_prep_dma_xor = iop_adma_prep_dma_xor; } -if (dma_has_cap (DMA_ZERO_SUM , dma_dev->cap_mask)) - dma_dev->device_prep_dma_zero_sum = - iop_adma_prep_dma_zero_sum; +if (dma_has_cap (DMA_XOR_VAL , dma_dev->cap_mask)) + dma_dev->device_prep_dma_xor_val = + iop_adma_prep_dma_xor_val;if (dma_has_cap (DMA_INTERRUPT , dma_dev->cap_mask)) dma_dev->device_prep_dma_interrupt = iop_adma_prep_dma_interrupt;
@@ -1249,7 +1249,7 @@ static int __devinit iop_adma_probe(struct platform_device *pdev)if (dma_has_cap (DMA_XOR , dma_dev->cap_mask) ||dma_has_cap (DMA_MEMSET , dma_dev->cap_mask)) { - ret =iop_adma_xor_zero_sum_self_test (adev); + ret =iop_adma_xor_val_self_test (adev);dev_dbg (&pdev->dev,"xor self test returned %d\n" , ret);if (ret) goto err_free_iop_chan;
@@ -1259,10 +1259,10 @@ static int __devinit iop_adma_probe(struct platform_device *pdev)"( %s%s%s%s%s%s%s%s%s%s)\n" ,dma_has_cap (DMA_PQ_XOR , dma_dev->cap_mask) ?"pq_xor " :"" ,dma_has_cap (DMA_PQ_UPDATE , dma_dev->cap_mask) ?"pq_update " :"" , -dma_has_cap (DMA_PQ_ZERO_SUM , dma_dev->cap_mask) ?"pq_zero_sum " :"" , +dma_has_cap (DMA_PQ_VAL , dma_dev->cap_mask) ?"pq_val " :"" ,dma_has_cap (DMA_XOR , dma_dev->cap_mask) ?"xor " :"" ,dma_has_cap (DMA_DUAL_XOR , dma_dev->cap_mask) ?"dual_xor " :"" , -dma_has_cap (DMA_ZERO_SUM , dma_dev->cap_mask) ?"xor_zero_sum " :"" , +dma_has_cap (DMA_XOR_VAL , dma_dev->cap_mask) ?"xor_val " :"" ,dma_has_cap (DMA_MEMSET , dma_dev->cap_mask) ?"fill " :"" ,dma_has_cap (DMA_MEMCPY_CRC32C , dma_dev->cap_mask) ?"cpy+crc " :"" , dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask) ? "cpy " : "",
drivers/md/raid5.c +1 -1
@@ -854,7 +854,7 @@ static void ops_run_check(struct stripe_head *sh) xor_srcs[count++] = dev->page; } - tx =async_xor_zero_sum (xor_dest, xor_srcs,0 , count,STRIPE_SIZE , + tx =async_xor_val (xor_dest, xor_srcs,0 , count,STRIPE_SIZE , &sh->ops.zero_sum_result,0 ,NULL ,NULL ,NULL );atomic_inc (&sh->count);
include/linux/async_tx.h +1 -1
@@ -117,7 +117,7 @@ async_xor(struct page *dest, struct page **src_list, unsigned int offset, dma_async_tx_callback cb_fn, void *cb_fn_param);struct dma_async_tx_descriptor * -async_xor_zero_sum(struct page *dest, struct page **src_list, +async_xor_val(struct page *dest, struct page **src_list, unsigned int offset, int src_cnt, size_t len, u32 *result, enum async_tx_flags flags, struct dma_async_tx_descriptor *depend_tx,
include/linux/dmaengine.h +4 -4
@@ -55,8 +55,8 @@ enum dma_transaction_type {DMA_PQ_XOR ,DMA_DUAL_XOR ,DMA_PQ_UPDATE , -DMA_ZERO_SUM , -DMA_PQ_ZERO_SUM , +DMA_XOR_VAL , +DMA_PQ_VAL ,DMA_MEMSET ,DMA_MEMCPY_CRC32C , DMA_INTERRUPT,
@@ -214,7 +214,7 @@ struct dma_async_tx_descriptor {* @device_free_chan_resources: release DMA channel's resources * @device_prep_dma_memcpy: prepares a memcpy operation * @device_prep_dma_xor: prepares a xor operation -* @device_prep_dma_zero_sum: prepares a zero_sum operation +* @device_prep_dma_xor_val: prepares a xor validation operation * @device_prep_dma_memset: prepares a memset operation * @device_prep_dma_interrupt: prepares an end of chain interrupt operation * @device_prep_slave_sg: prepares a slave dma operation
@@ -243,7 +243,7 @@ struct dma_device {struct dma_async_tx_descriptor *(*device_prep_dma_xor)(struct dma_chan *chan,dma_addr_t dest,dma_addr_t *src,unsigned int src_cnt,size_t len,unsigned long flags); -struct dma_async_tx_descriptor *(*device_prep_dma_zero_sum)( +struct dma_async_tx_descriptor *(*device_prep_dma_xor_val)(struct dma_chan *chan,dma_addr_t *src,unsigned int src_cnt,size_t len,u32 *result,unsigned long flags); struct dma_async_tx_descriptor *(*device_prep_dma_memset)(
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sosnowski, Maciej
Acked-byRE: [PATCH v2 01/11] async_tx: rename zero_sum to val
Quoted 17 lines · Dan Williams
'
zero_sum' does not properly describe the operation of generating parity and checking that it validates against an existing buffer. Change the name of the operation to 'val' (for 'validate'). This is in anticipation of the p+q case where it is a requirement to identify the target parity buffers separately from the source buffers, because the target parity buffers will not have corresponding pq coefficients.Signed-off-by Dan Williams <[email protected]>---
arch/arm/mach-iop13xx/setup.c | 8 ++++----arch/arm/plat-iop/adma.c| 2 +-crypto/async_tx/async_xor.c| 16 ++++++++--------drivers/dma/dmaengine.c| 4 ++--drivers/dma/iop-adma.c| 38 +++++++++++++++++++-------------------drivers/md/raid5.c| 2 +-include/linux/async_tx.h| 2 +-include/linux/dmaengine.h| 8 ++++----
Quoted 1 line
8 files changed, 40 insertions(+), 40 deletions(-)
with following comments:
drivers/dma/iop-adma.c iop_adma_xor_zero_sum_self_test(struct · line 1079
Quoted 13 lines
drivers/dma/iop-adma.c +4
-4@@ -1072,10 +1072,10 @@ iop_adma_xor_zero_sum_self_test(struct iop_adma_device *device) dma_srcs[i] = dma_map_page(dma_chan->device->dev, zero_sum_srcs[i], 0, PAGE_SIZE, DMA_TO_DEVICE); - tx = iop_adma_prep_dma_zero_sum(dma_chan, dma_srcs, - IOP_ADMA_NUM_SRC_TEST + 1, PAGE_SIZE, - &zero_sum_result, - DMA_PREP_INTERRUPT | DMA_CTRL_ACK); + tx = iop_adma_prep_dma_xor_val(dma_chan, dma_srcs, + IOP_ADMA_NUM_SRC_TEST + 1, PAGE_SIZE, + &zero_sum_result, + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
What about zero_sum_srcs and zero_sum_result? Shouldn't they be renamed too?
drivers/md/raid5.c static void ops_run_check(struct stripe_head *sh) · line 858
Quoted 9 lines
drivers/md/raid5.c +1
-1@@ -854,7 +854,7 @@ static void ops_run_check(struct stripe_head *sh) xor_srcs[count++] = dev->page; } - tx = async_xor_zero_sum(xor_dest, xor_srcs, 0, count, STRIPE_SIZE, + tx = async_xor_val(xor_dest, xor_srcs, 0, count, STRIPE_SIZE, &sh->ops.zero_sum_result, 0, NULL, NULL, NULL);
And the same here...
Regards,
Maciej--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/11] async_tx: rename zero_sum to val
2009/5/29 Sosnowski, Maciej <[email protected]>:
Quoted 24 lines
Quoted 17 lines · Dan Williams
'
zero_sum' does not properly describe the operation of generating parity and checking that it validates against an existing buffer. Change the name of the operation to 'val' (for 'validate'). This is in anticipation of the p+q case where it is a requirement to identify the target parity buffers separately from the source buffers, because the target parity buffers will not have corresponding pq coefficients.Signed-off-by Dan Williams <[email protected]>---
arch/arm/mach-iop13xx/setup.c | 8 ++++----arch/arm/plat-iop/adma.c| 2 +-crypto/async_tx/async_xor.c| 16 ++++++++--------drivers/dma/dmaengine.c| 4 ++--drivers/dma/iop-adma.c| 38 +++++++++++++++++++-------------------drivers/md/raid5.c| 2 +-include/linux/async_tx.h| 2 +-include/linux/dmaengine.h| 8 ++++----Quoted 1 line
8 files changed, 40 insertions(+), 40 deletions(-)Acked-by Maciej Sosnowski <[email protected]>with following comments:
Quoted 15 lines
drivers/dma/iop-adma.c iop_adma_xor_zero_sum_self_test(struct · line 1079
Quoted 13 lines
drivers/dma/iop-adma.c +4
-4@@ -1072,10 +1072,10 @@ iop_adma_xor_zero_sum_self_test(struct iop_adma_device *device) dma_srcs[i] = dma_map_page(dma_chan->device->dev, zero_sum_srcs[i], 0, PAGE_SIZE, DMA_TO_DEVICE); - tx = iop_adma_prep_dma_zero_sum(dma_chan, dma_srcs, - IOP_ADMA_NUM_SRC_TEST + 1, PAGE_SIZE, - &zero_sum_result, - DMA_PREP_INTERRUPT | DMA_CTRL_ACK); + tx = iop_adma_prep_dma_xor_val(dma_chan, dma_srcs, + IOP_ADMA_NUM_SRC_TEST + 1, PAGE_SIZE, + &zero_sum_result, + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);What about
zero_sum_srcsandzero_sum_result? Shouldn't they be renamed too?
No, the hardware specification calls the operation "zero sum", so it is fine that the kernel calls it one thing but the driver translates it into something else.
Thanks,
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 2/11 async_tx: kill ASYNC_TX_DEP_ACK flag
In reply to Dan WilliamsIn support of inter-channel chaining async_tx utilizes an ack flag to gate whether a dependent operation can be chained to another. While the flag is not set the chain can be considered open for appending. Setting the ack flag closes the chain and flags the descriptor for garbage collection. The ASYNC_TX_DEP_ACK flag essentially means "close the chain after adding this dependency". Since each operation can only have one child the api now implicitly sets the ack flag at dependency submission time. This removes an unnecessary management burden from clients of the api.
[ Impact: clean up and enforce one dependency per operation ]
--- crypto/async_tx/async_memcpy.c | 2 +- crypto/async_tx/async_memset.c | 2 +- crypto/async_tx/async_tx.c | 4 ++-- crypto/async_tx/async_xor.c | 6 ++---- drivers/md/raid5.c | 25 +++++++++++-------------- include/linux/async_tx.h | 4 +--- 6 files changed, 18 insertions(+), 25 deletions(-)
crypto/async_tx/async_memcpy.c +1 -1
@@ -35,7 +35,7 @@* @src: src page * @offset: offset in pages to start transaction * @len: length in bytes -* @flags: ASYNC_TX_ACK, ASYNC_TX_DEP_ACK, +* @flags: ASYNC_TX_ACK * @depend_tx: memcpy depends on the result of this transaction * @cb_fn: function to call when the memcpy completes * @cb_param: parameter to pass to the callback routine
crypto/async_tx/async_memset.c +1 -1
@@ -35,7 +35,7 @@* @val: fill value * @offset: offset in pages to start transaction * @len: length in bytes -* @flags: ASYNC_TX_ACK, ASYNC_TX_DEP_ACK +* @flags: ASYNC_TX_ACK * @depend_tx: memset depends on the result of this transaction * @cb_fn: function to call when the memcpy completes * @cb_param: parameter to pass to the callback routine
crypto/async_tx/async_tx.c +2 -2
@@ -223,7 +223,7 @@ async_tx_submit(struct dma_chan *chan, struct dma_async_tx_descriptor *tx,if (flags &ASYNC_TX_ACK )async_tx_ack (tx); -if (depend_tx && (flags &ASYNC_TX_DEP_ACK )) +if (depend_tx)async_tx_ack (depend_tx); }EXPORT_SYMBOL_GPL (async_tx_submit);
@@ -231,7 +231,7 @@ EXPORT_SYMBOL_GPL(async_tx_submit);/** * async_trigger_callback - schedules the callback function to be run after * any dependent operations have been completed. -* @flags: ASYNC_TX_ACK, ASYNC_TX_DEP_ACK +* @flags: ASYNC_TX_ACK * @depend_tx: 'callback' requires the completion of this transaction * @cb_fn: function to call after depend_tx completes * @cb_param: parameter to pass to the callback routine
crypto/async_tx/async_xor.c +2 -4
@@ -105,7 +105,6 @@ do_async_xor(struct dma_chan *chan, struct page *dest, struct page **src_list, _cb_param); depend_tx = tx; - flags |=ASYNC_TX_DEP_ACK ;if (src_cnt > xor_src_cnt) {/* drop completed sources */
@@ -168,8 +167,7 @@ do_sync_xor(struct page *dest, struct page **src_list, unsigned int offset,* @offset: offset in pages to start transaction * @src_cnt: number of source pages * @len: length in bytes -* @flags: ASYNC_TX_XOR_ZERO_DST, ASYNC_TX_XOR_DROP_DEST, -* ASYNC_TX_ACK, ASYNC_TX_DEP_ACK +* @flags: ASYNC_TX_XOR_ZERO_DST, ASYNC_TX_XOR_DROP_DEST, ASYNC_TX_ACK * @depend_tx: xor depends on the result of this transaction. * @cb_fn: function to call when the xor completes * @cb_param: parameter to pass to the callback routine
@@ -230,7 +228,7 @@ static int page_is_zero(struct page *p, unsigned int offset, size_t len)* @src_cnt: number of source pages * @len: length in bytes * @result: 0 if sum == 0 else non-zero -* @flags: ASYNC_TX_ACK, ASYNC_TX_DEP_ACK +* @flags: ASYNC_TX_ACK * @depend_tx: xor depends on the result of this transaction. * @cb_fn: function to call when the xor completes * @cb_param: parameter to pass to the callback routine
drivers/md/raid5.c +11 -14
@@ -525,14 +525,12 @@ async_copy_data(int frombio, struct bio *bio, struct page *page, bio_page =bio_iovec_idx (bio, i)->bv_page;if (frombio) tx =async_memcpy (page, bio_page, page_offset, - b_offset, clen, -ASYNC_TX_DEP_ACK , - tx,NULL ,NULL ); + b_offset, clen,0 , + tx,NULL ,NULL );else tx =async_memcpy (bio_page, page, b_offset, - page_offset, clen, -ASYNC_TX_DEP_ACK , - tx,NULL ,NULL ); + page_offset, clen,0 , + tx,NULL ,NULL ); }if (clen < len)/* hit end of page */ break ;
@@ -615,8 +613,7 @@ static void ops_run_biofill(struct stripe_head *sh) }atomic_inc (&sh->count); -async_trigger_callback (ASYNC_TX_DEP_ACK |ASYNC_TX_ACK , tx, - ops_complete_biofill, sh); +async_trigger_callback (ASYNC_TX_ACK , tx, ops_complete_biofill, sh); }static void ops_complete_compute5 (void *stripe_head_ref)
@@ -701,8 +698,8 @@ ops_run_prexor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx) } tx =async_xor (xor_dest, xor_srcs,0 , count,STRIPE_SIZE , -ASYNC_TX_DEP_ACK |ASYNC_TX_XOR_DROP_DST , tx, - ops_complete_prexor, sh); +ASYNC_TX_XOR_DROP_DST , tx, + ops_complete_prexor, sh);return tx; }
@@ -809,7 +806,7 @@ ops_run_postxor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)* set ASYNC_TX_XOR_DROP_DST and ASYNC_TX_XOR_ZERO_DST * for the synchronous xor case */ - flags =ASYNC_TX_DEP_ACK |ASYNC_TX_ACK | + flags =ASYNC_TX_ACK | (prexor ?ASYNC_TX_XOR_DROP_DST :ASYNC_TX_XOR_ZERO_DST );atomic_inc (&sh->count);
@@ -858,7 +855,7 @@ static void ops_run_check(struct stripe_head *sh) &sh->ops.zero_sum_result, 0, NULL, NULL, NULL);atomic_inc (&sh->count); - tx =async_trigger_callback (ASYNC_TX_DEP_ACK |ASYNC_TX_ACK , tx, + tx =async_trigger_callback (ASYNC_TX_ACK , tx, ops_complete_check, sh); }
@@ -2687,8 +2684,8 @@ static void handle_stripe_expansion(raid5_conf_t *conf, struct stripe_head *sh,/* place all the copies on one channel */ tx =async_memcpy (sh2->dev[dd_idx].page, - sh->dev[i].page,0 ,0 ,STRIPE_SIZE , -ASYNC_TX_DEP_ACK , tx,NULL ,NULL ); + sh->dev[i].page,0 ,0 ,STRIPE_SIZE , +0 , tx,NULL ,NULL );set_bit (R5_Expanded, &sh2->dev[dd_idx].flags);set_bit (R5_UPTODATE , &sh2->dev[dd_idx].flags);
include/linux/async_tx.h +1 -3
@@ -58,13 +58,11 @@ struct dma_chan_ref {* array. * @ASYNC_TX_ACK: immediately ack the descriptor, precludes setting up a * dependency chain -* @ASYNC_TX_DEP_ACK: ack the dependency descriptor. Useful for chaining. */ enum async_tx_flags {ASYNC_TX_XOR_ZERO_DST = (1 <<0 ),ASYNC_TX_XOR_DROP_DST = (1 <<1 ), -ASYNC_TX_ACK = (1 <<3 ), -ASYNC_TX_DEP_ACK = (1 <<4 ), +ASYNC_TX_ACK = (1 <<2 ), };#ifdef CONFIG_DMA_ENGINE
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sosnowski, Maciej
Acked-byRE: [PATCH v2 02/11] async_tx: kill ASYNC_TX_DEP_ACK flag
Quoted 13 lines · Dan Williams
In support of inter-channel chaining
async_txutilizes an ack flag to gate whether a dependent operation can be chained to another. While the flag is not set the chain can be considered open for appending. Setting the ack flag closes the chain and flags the descriptor for garbage collection. TheASYNC_TX_DEP_ACKflag essentially means "close the chain after adding this dependency". Since each operation can only have one child the api now implicitly sets the ack flag at dependency submission time. This removes an unnecessary management burden from clients of the api.[ Impact: clean up and enforce one dependency per operation ]
Signed-off-by Dan Williams <[email protected]>
Quoted 8 lines
---
crypto/async_tx/async_memcpy.c | 2 +-crypto/async_tx/async_memset.c| 2 +-crypto/async_tx/async_tx.c| 4 ++--crypto/async_tx/async_xor.c| 6 ++----drivers/md/raid5.c| 25 +++++++++++--------------include/linux/async_tx.h| 4 +---6 files changed, 18 insertions(+), 25 deletions(-)
Async-tx-api.txt documentation should be also updated (it still describes ASYNC_TX_DEP_ACK usage).
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/11] async_tx: kill ASYNC_TX_DEP_ACK flag
2009/5/29 Sosnowski, Maciej <[email protected]>:
Quoted 23 lines
Quoted 13 lines · Dan Williams
In support of inter-channel chaining
async_txutilizes an ack flag to gate whether a dependent operation can be chained to another. While the flag is not set the chain can be considered open for appending. Setting the ack flag closes the chain and flags the descriptor for garbage collection. TheASYNC_TX_DEP_ACKflag essentially means "close the chain after adding this dependency". Since each operation can only have one child the api now implicitly sets the ack flag at dependency submission time. This removes an unnecessary management burden from clients of the api.[ Impact: clean up and enforce one dependency per operation ]
Signed-off-by Dan Williams <[email protected]>Quoted 8 lines
---
crypto/async_tx/async_memcpy.c | 2 +-crypto/async_tx/async_memset.c| 2 +-crypto/async_tx/async_tx.c| 4 ++--crypto/async_tx/async_xor.c| 6 ++----drivers/md/raid5.c| 25 +++++++++++--------------include/linux/async_tx.h| 4 +---6 files changed, 18 insertions(+), 25 deletions(-)
Quoted 3 lines
Async-tx-api.txt documentation should be also updated (it still describes
ASYNC_TX_DEP_ACKusage).
Done, thanks... and I also need to update the document for the async_submit_ctl structure.
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 3/11 async_tx: structify submission arguments, add scribble
In reply to Dan WilliamsPrepare the api for the arrival of a new parameter, 'scribble'. This will allow callers to identify scratchpad memory for dma address or page address conversions. As this adds yet another parameter, take this opportunity to convert the common submission parameters (flags, dependency, callback, and callback argument) into an object that is passed by reference.
[ Impact: moves api pass-by-value parameters to a pass-by-reference struct ]
--- crypto/async_tx/async_memcpy.c | 21 ++++----- crypto/async_tx/async_memset.c | 23 ++++------ crypto/async_tx/async_tx.c | 34 +++++++-------- crypto/async_tx/async_xor.c | 93 +++++++++++++++++----------------------- drivers/md/raid5.c | 59 +++++++++++++++---------- include/linux/async_tx.h | 84 +++++++++++++++++++++++------------- 6 files changed, 161 insertions(+), 153 deletions(-)
crypto/async_tx/async_memcpy.c +9 -12
@@ -35,26 +35,23 @@* @src: src page * @offset: offset in pages to start transaction * @len: length in bytes -* @flags: ASYNC_TX_ACK -* @depend_tx: memcpy depends on the result of this transaction -* @cb_fn: function to call when the memcpy completes -* @cb_param: parameter to pass to the callback routine +* @submit: submission / completion modifiers */ struct dma_async_tx_descriptor *async_memcpy (struct page *dest,struct page *src,unsigned int dest_offset, -unsigned int src_offset,size_t len,enum async_tx_flags flags, -struct dma_async_tx_descriptor *depend_tx, -dma_async_tx_callback cb_fn,void *cb_param) +unsigned int src_offset,size_t len, +struct async_submit_ctl *submit) { -struct dma_chan *chan =async_tx_find_channel (depend_tx,DMA_MEMCPY , +struct dma_chan *chan =async_tx_find_channel (submit,DMA_MEMCPY , &dest,1 , &src,1 , len);struct dma_device *device = chan ? chan->device :NULL ;struct dma_async_tx_descriptor *tx =NULL ;if (device) {dma_addr_t dma_dest, dma_src; -unsigned long dma_prep_flags = cb_fn ?DMA_PREP_INTERRUPT :0 ; +unsigned long dma_prep_flags; + dma_prep_flags = submit->cb_fn ?DMA_PREP_INTERRUPT :0 ; dma_dest =dma_map_page (device->dev, dest, dest_offset, len,DMA_FROM_DEVICE );
@@ -67,13 +64,13 @@ async_memcpy(struct page *dest, struct page *src, unsigned int dest_offset,if (tx) {pr_debug ("%s: (async) len: %zu\n" , __func__, len); -async_tx_submit (chan, tx, flags, depend_tx, cb_fn, cb_param); +async_tx_submit (chan, tx, submit); }else {void *dest_buf, *src_buf;pr_debug ("%s: (sync) len: %zu\n" , __func__, len);/* wait for any prerequisite operations */ -async_tx_quiesce (&depend_tx); +async_tx_quiesce (&submit->depend_tx); dest_buf =kmap_atomic (dest,KM_USER0 ) + dest_offset; src_buf =kmap_atomic (src,KM_USER1 ) + src_offset;
@@ -83,7 +80,7 @@ async_memcpy(struct page *dest, struct page *src, unsigned int dest_offset,kunmap_atomic (dest_buf,KM_USER0 );kunmap_atomic (src_buf,KM_USER1 ); -async_tx_sync_epilog (cb_fn, cb_param); +async_tx_sync_epilog (submit); }return tx;
crypto/async_tx/async_memset.c +9 -14
@@ -35,26 +35,21 @@* @val: fill value * @offset: offset in pages to start transaction * @len: length in bytes -* @flags: ASYNC_TX_ACK -* @depend_tx: memset depends on the result of this transaction -* @cb_fn: function to call when the memcpy completes -* @cb_param: parameter to pass to the callback routine */ struct dma_async_tx_descriptor * -async_memset (struct page *dest,int val,unsigned int offset, -size_t len,enum async_tx_flags flags, -struct dma_async_tx_descriptor *depend_tx, -dma_async_tx_callback cb_fn,void *cb_param) +async_memset (struct page *dest,int val,unsigned int offset,size_t len, +struct async_submit_ctl *submit) { -struct dma_chan *chan =async_tx_find_channel (depend_tx,DMA_MEMSET , +struct dma_chan *chan =async_tx_find_channel (submit,DMA_MEMSET , &dest,1 ,NULL ,0 , len);struct dma_device *device = chan ? chan->device :NULL ;struct dma_async_tx_descriptor *tx =NULL ;if (device) {dma_addr_t dma_dest; -unsigned long dma_prep_flags = cb_fn ?DMA_PREP_INTERRUPT :0 ; +unsigned long dma_prep_flags; + dma_prep_flags = submit->cb_fn ?DMA_PREP_INTERRUPT :0 ; dma_dest =dma_map_page (device->dev, dest, offset, len,DMA_FROM_DEVICE );
@@ -64,19 +59,19 @@ async_memset(struct page *dest, int val, unsigned int offset,if (tx) {pr_debug ("%s: (async) len: %zu\n" , __func__, len); -async_tx_submit (chan, tx, flags, depend_tx, cb_fn, cb_param); +async_tx_submit (chan, tx, submit); }else {/* run the memset synchronously */ void *dest_buf;pr_debug ("%s: (sync) len: %zu\n" , __func__, len); - dest_buf = (void *) (((char *)page_address (dest)) + offset); + dest_buf =page_address (dest) + offset;/* wait for any prerequisite operations */ -async_tx_quiesce (&depend_tx); +async_tx_quiesce (&submit->depend_tx);memset (dest_buf, val, len); -async_tx_sync_epilog (cb_fn, cb_param); +async_tx_sync_epilog (submit); }return tx;
crypto/async_tx/async_tx.c +16 -18
@@ -45,13 +45,15 @@ static void __exit async_tx_exit(void)/** * __async_tx_find_channel - find a channel to carry out the operation or let * the transaction execute synchronously -* @depend_tx: transaction dependency +* @submit: transaction dependency and submission modifiers * @tx_type: transaction type */ struct dma_chan * -__async_tx_find_channel (struct dma_async_tx_descriptor *depend_tx, -enum dma_transaction_type tx_type) +__async_tx_find_channel (struct async_submit_ctl *submit, +enum dma_transaction_type tx_type) { +struct dma_async_tx_descriptor *depend_tx = submit->depend_tx; +/* see if we can keep the chain on one channel */ if (depend_tx &&dma_has_cap (tx_type, depend_tx->chan->device->cap_mask))
@@ -160,11 +162,12 @@ enum submit_disposition {void async_tx_submit (struct dma_chan *chan,struct dma_async_tx_descriptor *tx, -enum async_tx_flags flags,struct dma_async_tx_descriptor *depend_tx, -dma_async_tx_callback cb_fn,void *cb_param) +struct async_submit_ctl *submit) { - tx->callback = cb_fn; - tx->callback_param = cb_param; +struct dma_async_tx_descriptor *depend_tx = submit->depend_tx; + + tx->callback = submit->cb_fn; + tx->callback_param = submit->cb_param;if (depend_tx) {enum submit_disposition s;
@@ -220,7 +223,7 @@ async_tx_submit(struct dma_chan *chan, struct dma_async_tx_descriptor *tx, tx->tx_submit (tx); } -if (flags &ASYNC_TX_ACK ) +if (submit->flags &ASYNC_TX_ACK )async_tx_ack (tx);if (depend_tx)
@@ -231,19 +234,14 @@ EXPORT_SYMBOL_GPL(async_tx_submit);/** * async_trigger_callback - schedules the callback function to be run after * any dependent operations have been completed. -* @flags: ASYNC_TX_ACK -* @depend_tx: 'callback' requires the completion of this transaction -* @cb_fn: function to call after depend_tx completes -* @cb_param: parameter to pass to the callback routine */ struct dma_async_tx_descriptor * -async_trigger_callback (enum async_tx_flags flags, -struct dma_async_tx_descriptor *depend_tx, -dma_async_tx_callback cb_fn,void *cb_param) +async_trigger_callback (struct async_submit_ctl *submit) {struct dma_chan *chan;struct dma_device *device;struct dma_async_tx_descriptor *tx; +struct dma_async_tx_descriptor *depend_tx = submit->depend_tx;if (depend_tx) { chan = depend_tx->chan;
@@ -262,14 +260,14 @@ async_trigger_callback(enum async_tx_flags flags,if (tx) {pr_debug ("%s: (async)\n" , __func__); -async_tx_submit (chan, tx, flags, depend_tx, cb_fn, cb_param); +async_tx_submit (chan, tx, submit); }else {pr_debug ("%s: (sync)\n" , __func__);/* wait for any prerequisite operations */ -async_tx_quiesce (&depend_tx); +async_tx_quiesce (&submit->depend_tx); -async_tx_sync_epilog (cb_fn, cb_param); +async_tx_sync_epilog (submit); }return tx;
crypto/async_tx/async_xor.c +39 -54
219 lines not on this page · raw message
drivers/md/raid5.c +34 -25
158 lines not on this page · raw message
include/linux/async_tx.h +54 -30
123 lines not on this page · raw message
Re: [PATCH v2 03/11] async_tx: structify submission arguments, add scribble
Quoted 12 lines · Dan Williams
crypto/async_tx/async_memcpy.c +0
-4@@ -35,26 +35,23 @@* @src: src page * @offset: offset in pages to start transaction * @len: length in bytes -* @flags: ASYNC_TX_ACK -* @depend_tx: memcpy depends on the result of this transaction -* @cb_fn: function to call when the memcpy completes -* @cb_param: parameter to pass to the callback routine
crypto/async_tx/async_memcpy.c line 44
Quoted 10 lines
crypto/async_tx/async_memcpy.c +3
-3+* @submit: submission / completion modifiers */ struct dma_async_tx_descriptor *async_memcpy (struct page *dest,struct page *src,unsigned int dest_offset, -unsigned int src_offset,size_t len,enum async_tx_flags flags, -struct dma_async_tx_descriptor *depend_tx, -dma_async_tx_callback cb_fn,void *cb_param) +unsigned int src_offset,size_t len, +struct async_submit_ctl *submit) {
The third parameter is called "dest_offset", but the comment refers to "@offset".
crypto/async_tx/async_memcpy.c line 51
Quoted 7 lines
crypto/async_tx/async_memcpy.c +7
-0+struct async_submit_ctl { +enum async_tx_flags flags; +struct dma_async_tx_descriptor *depend_tx; +dma_async_tx_callback cb_fn; +void *cb_param; +void *scribble; +};
Can't scribble be of type addr_conv_t *?
Apart from these two minor issues the patch looks really nice. Thanks for taking the time to combine the common submission parameters to the new structure. This improves readability of the code and makes adaptation to future needs easier.
Andre
Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
Re: [PATCH v2 03/11] async_tx: structify submission arguments, add scribble
Quoted 19 lines · Andre Noll
Quoted 17 lines · Dan Williams
crypto/async_tx/async_memcpy.c +1
-4@@ -35,26 +35,23 @@* @src: src page * @offset: offset in pages to start transaction * @len: length in bytes -* @flags: ASYNC_TX_ACK -* @depend_tx: memcpy depends on the result of this transaction -* @cb_fn: function to call when the memcpy completes -* @cb_param: parameter to pass to the callback routine +* @submit: submission / completion modifiers */ struct
dma_async_tx_descriptor*
async_memcpy(struct page *dest, struct page *src, unsigned intdest_offset,
- unsigned intsrc_offset,size_tlen, enumasync_tx_flagsflags,
Quoted 8 lines
Quoted 5 lines
- struct
dma_async_tx_descriptor*depend_tx,
-dma_async_tx_callbackcb_fn, void*cb_param)
+ unsigned intsrc_offset,size_tlen,
+ structasync_submit_ctl*submit) {The third parameter is called "
dest_offset", but the comment refers to "@offset".
...and I was missing a comment for src_offset. fixed.
Quoted 10 lines
crypto/async_tx/async_memcpy.c
Quoted 8 lines
crypto/async_tx/async_memcpy.c +7
-0+struct async_submit_ctl { + enum async_tx_flags flags; + struct dma_async_tx_descriptor *depend_tx; + dma_async_tx_callback cb_fn; + void *cb_param; + void *scribble; +};Can't scribble be of type
addr_conv_t*?
It could, but that would require casting it when it is used. I really only added the addr_conv_t type to get a compiler warning if someone inadvertently swaps the cb_param and scribble arguments to init_async_submit(). There is no benefit for type safety once we go to use it, and this will allow me to drop that unnecessary cast from void * that you identified in [PATCH 04/11].
Quoted 6 lines
Apart from these two minor issues the patch looks really nice. Thanks for taking the time to combine the common submission parameters to the new structure. This improves readability of the code and makes adaptation to future needs easier.
Signed-off-by Andre Noll <[email protected]>
Thanks!
--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
RE: [PATCH v2 03/11] async_tx: structify submission arguments, add scribble
Quoted 18 lines · Dan Williams
Prepare the api for the arrival of a new parameter, 'scribble'. This will allow callers to identify scratchpad memory for dma address or page address conversions. As this adds yet another parameter, take this opportunity to convert the common submission parameters (flags, dependency, callback, and callback argument) into an object that is passed by reference.
[ Impact: moves api pass-by-value parameters to a pass-by-reference struct ]
Signed-off-by Dan Williams <[email protected]>---
crypto/async_tx/async_memcpy.c | 21 ++++-----crypto/async_tx/async_memset.c| 23 ++++------crypto/async_tx/async_tx.c| 34 +++++++--------crypto/async_tx/async_xor.c| 93 +++++++++++++++++-----------------------drivers/md/raid5.c| 59 +++++++++++++++----------include/linux/async_tx.h| 84 +++++++++++++++++++++++-------------6 files changed, 161 insertions(+), 153 deletions(-)
(...)
Quoted 8 lines
drivers/md/raid5.c +0
-3@@ -811,13 +818,11 @@ ops_run_postxor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx) atomic_inc(&sh->count); - if (unlikely(count == 1)) { - flags &= ~(ASYNC_TX_XOR_DROP_DST | ASYNC_TX_XOR_ZERO_DST); - tx = async_memcpy(xor_dest, xor_srcs[0], 0, 0, STRIPE_SIZE,
drivers/md/raid5.c ops_run_postxor(struct stripe_head *sh, struct · line 827
Quoted 10 lines
drivers/md/raid5.c +5
-4- flags, tx, ops_complete_postxor, sh); - } else - tx = async_xor(xor_dest, xor_srcs, 0, count, STRIPE_SIZE, - flags, tx, ops_complete_postxor, sh); + init_async_submit(&submit, flags, tx, ops_complete_postxor, sh, NULL); + if (unlikely(count == 1)) + tx = async_memcpy(xor_dest, xor_srcs[0], 0, 0, STRIPE_SIZE, &submit); + else + tx = async_xor(xor_dest, xor_srcs, 0, count, STRIPE_SIZE, &submit); }
What about ASYNC_TX_XOR_DROP_DST and ASYNC_TX_XOR_ZERO_DST flags clearing before async_memcpy?
Maciej--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/11] async_tx: structify submission arguments, add scribble
2009/5/29 Sosnowski, Maciej <[email protected]>:
Quoted 27 lines
Quoted 18 lines · Dan Williams
Prepare the api for the arrival of a new parameter, 'scribble'. This will allow callers to identify scratchpad memory for dma address or page address conversions. As this adds yet another parameter, take this opportunity to convert the common submission parameters (flags, dependency, callback, and callback argument) into an object that is passed by reference.
[ Impact: moves api pass-by-value parameters to a pass-by-reference struct ]
Signed-off-by Dan Williams <[email protected]>---
crypto/async_tx/async_memcpy.c | 21 ++++-----crypto/async_tx/async_memset.c| 23 ++++------crypto/async_tx/async_tx.c| 34 +++++++--------crypto/async_tx/async_xor.c| 93 +++++++++++++++++-----------------------drivers/md/raid5.c| 59 +++++++++++++++----------include/linux/async_tx.h| 84 +++++++++++++++++++++++-------------6 files changed, 161 insertions(+), 153 deletions(-)(...)
Quoted 5 lines
drivers/md/raid5.c +0
-0@@ -811,13 +818,11 @@ ops_run_postxor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx) atomic_inc(&sh->count);
Quoted 16 lines
drivers/md/raid5.c ops_run_postxor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx) · line 826
Quoted 13 lines
drivers/md/raid5.c +5
-7- if (unlikely(count == 1)) { - flags &= ~(ASYNC_TX_XOR_DROP_DST | ASYNC_TX_XOR_ZERO_DST); - tx = async_memcpy(xor_dest, xor_srcs[0], 0, 0, STRIPE_SIZE, - flags, tx, ops_complete_postxor, sh); - } else - tx = async_xor(xor_dest, xor_srcs, 0, count, STRIPE_SIZE, - flags, tx, ops_complete_postxor, sh); + init_async_submit(&submit, flags, tx, ops_complete_postxor, sh, NULL); + if (unlikely(count == 1)) + tx = async_memcpy(xor_dest, xor_srcs[0], 0, 0, STRIPE_SIZE, &submit); + else + tx = async_xor(xor_dest, xor_srcs, 0, count, STRIPE_SIZE, &submit); }What about
ASYNC_TX_XOR_DROP_DSTandASYNC_TX_XOR_ZERO_DSTflags clearing beforeasync_memcpy?
Not necessary. The routines ignore the flags that are not relevant. However, the relevant flags for each routine used to be documented in the kerneldoc description, but those lines got removed when the submit parameter was added. I'll go back and add notes to each routine about which flags are honored and fix up compliance with the kerneldoc format.
Thanks,
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 4/11 async_xor: permit callers to pass in a 'dma/page scribble' region
In reply to Dan Williamsasync_xor() needs space to perform dma and page address conversions. In most cases the code can simply reuse the struct page * array because the size of the native pointer matches the size of a dma/page address. In order to support archs where sizeof(dma_addr_t) is larger than sizeof(struct page *), or to preserve the input parameters, we utilize a memory region passed in by the caller.
Since the code is now prepared to handle the case where it cannot perform address conversions on the stack, we no longer need the !HIGHMEM64G dependency in drivers/dma/Kconfig.
[ Impact: don't clobber input buffers for address conversions ]
--- crypto/async_tx/async_xor.c | 59 ++++++++++++++++++++----------------------- drivers/dma/Kconfig | 2 + 2 files changed, 29 insertions(+), 32 deletions(-)
crypto/async_tx/async_xor.c +28 -31
@@ -33,11 +33,10 @@/* do_async_xor - dma map the pages and perform the xor with an engine */ static __async_inline struct dma_async_tx_descriptor *do_async_xor (struct dma_chan *chan,struct page *dest,struct page **src_list, -unsigned int offset,int src_cnt,size_t len, +unsigned int offset,int src_cnt,size_t len,dma_addr_t *dma_src,struct async_submit_ctl *submit) {struct dma_device *dma = chan->device; -dma_addr_t *dma_src = (dma_addr_t *) src_list;struct dma_async_tx_descriptor *tx =NULL ;int src_off =0 ;int i;
@@ -125,9 +124,14 @@ do_sync_xor(struct page *dest, struct page **src_list, unsigned int offset,int xor_src_cnt;int src_off =0 ;void *dest_buf; -void **srcs = (void **) src_list; +void **srcs; -/* reuse the 'src_list' array to convert to buffer pointers */ +if (submit->scribble) + srcs = (void **) submit->scribble; +else + srcs = (void **) src_list; + +/* convert to buffer pointers */ for (i =0 ; i < src_cnt; i++) srcs[i] =page_address (src_list[i]) + offset;
@@ -171,17 +175,26 @@ async_xor(struct page *dest, struct page **src_list, unsigned int offset,struct dma_chan *chan =async_tx_find_channel (submit,DMA_XOR , &dest,1 , src_list, src_cnt, len); +dma_addr_t *dma_src =NULL ; +BUG_ON (src_cnt <=1 ); -if (chan) { +if (submit->scribble) + dma_src = submit->scribble; +else if (sizeof (dma_addr_t) <=sizeof (struct page *)) + dma_src = (dma_addr_t *) src_list; + +if (dma_src && chan) {/* run the xor asynchronously */ pr_debug ("%s (async): len: %zu\n" , __func__, len);return do_async_xor (chan, dest, src_list, offset, src_cnt, len, - submit); + dma_src, submit); }else {/* run the xor synchronously */ pr_debug ("%s (sync): len: %zu\n" , __func__, len); +WARN_ONCE (chan,"%s: no space for dma address conversion\n" , + __func__);/* in the sync case the dest is an implied source * (assumes the dest is the first source)
@@ -229,11 +242,16 @@ async_xor_val(struct page *dest, struct page **src_list, unsigned int offset, src_cnt, len);struct dma_device *device = chan ? chan->device :NULL ;struct dma_async_tx_descriptor *tx =NULL ; +dma_addr_t *dma_src =NULL ;BUG_ON (src_cnt <=1 ); -if (device && src_cnt <= device->max_xor) { -dma_addr_t *dma_src = (dma_addr_t *) src_list; +if (submit->scribble) + dma_src = submit->scribble; +else if (sizeof (dma_addr_t) <=sizeof (struct page *)) + dma_src = (dma_addr_t *) src_list; + +if (dma_src && device && src_cnt <= device->max_xor) {unsigned long dma_prep_flags;int i;
@@ -263,6 +281,8 @@ async_xor_val(struct page *dest, struct page **src_list, unsigned int offset,enum async_tx_flags flags_orig = submit->flags;pr_debug ("%s: (sync) len: %zu\n" , __func__, len); +WARN_ONCE (device && src_cnt <= device->max_xor, +"%s: no space for dma address conversion\n" , __func__); submit->flags |=ASYNC_TX_XOR_DROP_DST ; submit->flags &= ~ASYNC_TX_ACK ;
@@ -281,29 +301,6 @@ async_xor_val(struct page *dest, struct page **src_list, unsigned int offset, }EXPORT_SYMBOL_GPL (async_xor_val); -static int __init async_xor_init (void ) -{ -#ifdef CONFIG_DMA_ENGINE -/* To conserve stack space the input src_list (array of page pointers) -* is reused to hold the array of dma addresses passed to the driver. -* This conversion is only possible when dma_addr_t is less than the -* the size of a pointer. HIGHMEM64G is known to violate this -* assumption. -*/ -BUILD_BUG_ON (sizeof (dma_addr_t) >sizeof (struct page *)); -#endif - -return 0 ; -} - -static void __exit async_xor_exit(void) -{ -do { }while (0 ); -} - -module_init (async_xor_init); -module_exit (async_xor_exit); -MODULE_AUTHOR ("Intel Corporation" );MODULE_DESCRIPTION ("asynchronous xor/xor-zero-sum api" );MODULE_LICENSE ("GPL" );
drivers/dma/Kconfig +1 -1
@@ -4,7 +4,7 @@ menuconfig DMADEVICES bool "DMA Engine support" - depends on !HIGHMEM64G && HAS_DMA + depends on HAS_DMA help DMA engines can do asynchronous data transfers without involving the host CPU. Currently, this framework can be
Re: [PATCH v2 04/11] async_xor: permit callers to pass in a 'dma/page scribble' region
Quoted 18 lines · Dan Williams
crypto/async_tx/async_xor.c +1
-2@@ -33,11 +33,10 @@/* do_async_xor - dma map the pages and perform the xor with an engine */ static __async_inline struct dma_async_tx_descriptor *do_async_xor (struct dma_chan *chan,struct page *dest,struct page **src_list, -unsigned int offset,int src_cnt,size_t len, +unsigned int offset,int src_cnt,size_t len,dma_addr_t *dma_src,struct async_submit_ctl *submit) {struct dma_device *dma = chan->device; -dma_addr_t *dma_src = (dma_addr_t *) src_list;struct dma_async_tx_descriptor *tx =NULL ;int src_off =0 ;int i;
crypto/async_tx/async_xor.c do_sync_xor(struct page *dest, struct page **src_list, unsigned int offset, · line 130
Quoted 9 lines
crypto/async_tx/async_xor.c +3
-2@@ -125,9 +124,14 @@ do_sync_xor(struct page *dest, struct page **src_list, unsigned int offset,int xor_src_cnt;int src_off =0 ;void *dest_buf; -void **srcs = (void **) src_list; +void **srcs; -/* reuse the 'src_list' array to convert to buffer pointers */ +if (submit->scribble) + srcs = (void **) submit->scribble;
Unnecessary cast as submit->scribble is void *.
Quoted 12 lines
crypto/async_tx/async_xor.c +5
-1@@ -171,17 +175,26 @@ async_xor(struct page *dest, struct page **src_list, unsigned int offset,struct dma_chan *chan =async_tx_find_channel (submit,DMA_XOR , &dest,1 , src_list, src_cnt, len); +dma_addr_t *dma_src =NULL ; +BUG_ON (src_cnt <=1 ); -if (chan) { +if (submit->scribble) + dma_src = submit->scribble; +else if (sizeof (dma_addr_t) <=sizeof (struct page *))
crypto/async_tx/async_xor.c async_xor(struct page *dest, struct page **src_list, unsigned int offset, · line 193
Quoted 10 lines
crypto/async_tx/async_xor.c +4
-1+ dma_src = (dma_addr_t *) src_list; + +if (dma_src && chan) {/* run the xor asynchronously */ pr_debug ("%s (async): len: %zu\n" , __func__, len);return do_async_xor (chan, dest, src_list, offset, src_cnt, len, - submit); + dma_src, submit); }else {
Don't we need to fall back to sync xor if src_cnt exceeds what the device can handle, i.e. if it is larger than chan->device->max_xor? async_xor_val() further down has a check for this condition.
Thanks
Andre
Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
Re: [PATCH v2 04/11] async_xor: permit callers to pass in a 'dma/page scribble' region
Quoted 10 lines · Andre Noll
Quoted 8 lines · Dan Williams
crypto/async_tx/async_xor.c +1
-1@@ -125,9 +124,14 @@ do_sync_xor(struct page *dest, struct page **src_list, unsigned int offset, int xor_src_cnt; int src_off = 0; void *dest_buf; - void **srcs = (void **) src_list; + void **srcs;
Quoted 5 lines
crypto/async_tx/async_xor.c do_sync_xor(struct page *dest, struct page **src_list, unsigned int offset, · line 130
Quoted 3 lines
crypto/async_tx/async_xor.c +2
-1- /* reuse the 'src_list' array to convert to buffer pointers */ + if (submit->scribble) + srcs = (void **) submit->scribble;Unnecessary cast as
submit->scribbleis void *.
fixed.
Quoted 19 lines
Quoted 19 lines
crypto/async_tx/async_xor.c +8
-1@@ -171,17 +175,26 @@ async_xor(struct page *dest, struct page **src_list, unsigned int offset, struct dma_chan *chan = async_tx_find_channel(submit, DMA_XOR, &dest, 1, src_list, src_cnt, len); + dma_addr_t *dma_src = NULL; + BUG_ON(src_cnt <= 1); - if (chan) { + if (submit->scribble) + dma_src = submit->scribble; + else if (sizeof(dma_addr_t) <= sizeof(struct page *)) + dma_src = (dma_addr_t *) src_list; + + if (dma_src && chan) { /* run the xor asynchronously */ pr_debug("%s (async): len: %zu\n", __func__, len);
Quoted 9 lines
crypto/async_tx/async_xor.c async_xor(struct page *dest, struct page **src_list, unsigned int offset, · line 193
Quoted 4 lines
crypto/async_tx/async_xor.c +1
-1return do_async_xor(chan, dest, src_list, offset, src_cnt, len, - submit); + dma_src, submit); } else {Don't we need to fall back to sync xor if
src_cntexceeds what the device can handle, i.e. if it is larger thanchan->device->max_xor?async_xor_val()further down has a check for this condition.
No, we don't need this check for async_xor(). The asynchronous path of async_xor_val() has the constraint that it must be able to validate an xor block without writing to that block i.e. it is a read-only operation. The synchronous path recalculates the xor with the destination as a source and then does a memcmp() to validate that the new result is zero.
To support more than ->max_xor sources for async_xor_val() we would need an engine that supported hardware continuation of validate operations (i.e. an engine with an internal buffer for the intermediate xor result) otherwise we will need to store an intermediate xor result in system memory which is no better than the synchronous path.
For async_xor() we are always allowed to write the destination, so we can reuse it as a source to continue the calculation of the xor result.
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/11] async_xor: permit callers to pass in a 'dma/page scribble' region
Quoted 19 lines · Dan Williams
Quoted 4 lines
Don't we need to fall back to sync xor if
src_cntexceeds what the device can handle, i.e. if it is larger thanchan->device->max_xor?async_xor_val()further down has a check for this condition.No, we don't need this check for
async_xor(). The asynchronous path ofasync_xor_val()has the constraint that it must be able to validate an xor block without writing to that block i.e. it is a read-only operation. The synchronous path recalculates the xor with the destination as a source and then does amemcmp()to validate that the new result is zero.To support more than ->max_xor sources for
async_xor_val()we would need an engine that supported hardware continuation of validate operations (i.e. an engine with an internal buffer for the intermediate xor result) otherwise we will need to store an intermediate xor result in system memory which is no better than the synchronous path.
Quoted 3 lines
For
async_xor()we are always allowed to write the destination, so we can reuse it as a source to continue the calculation of the xor result.
I see. Thanks for this detailed explanation!
BTW: So far I've only glanced at patches 7-11 of your series. I will review these patches this evening and tomorrow, so I'll send another batch of comments/questions/sign-offs by Friday.
Robinson^WAndre
Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
Re: [PATCH v2 04/11] async_xor: permit callers to pass in a 'dma/page scribble' region
Quoted 3 lines · Andre Noll
BTW: So far I've only glanced at patches 7-11 of your series. I will review these patches this evening and tomorrow, so I'll send another batch of comments/questions/sign-offs by Friday.
I've just sent a couple of review comments for the remaining patches. Since I found only minor issues, feel free to add my sign-off to all patches of the series.
Thanks
Andre
Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
Re: [PATCH v2 04/11] async_xor: permit callers to pass in a 'dma/page scribble' region
Quoted 9 lines · Andre Noll
Quoted 3 lines · Andre Noll
BTW: So far I've only glanced at patches 7-11 of your series. I will review these patches this evening and tomorrow, so I'll send another batch of comments/questions/sign-offs by Friday.
I've just sent a couple of review comments for the remaining patches. Since I found only minor issues, feel free to add my sign-off to all patches of the series.
Thanks it is very much appreciated.
Strictly speaking your "-by" line in this scenario should be either Acked-by or Reviewed-by. Signed-off-by is reserved for people who had the patch in their possession on its way to mainline. Any heartburn if I use Reviewed-by?
Re: [PATCH v2 04/11] async_xor: permit callers to pass in a 'dma/page scribble' region
Quoted 4 lines · Dan Williams
Strictly speaking your "-by" line in this scenario should be either Acked-by or Reviewed-by. Signed-off-by is reserved for people who had the patch in their possession on its way to mainline. Any heartburn if I use Reviewed-by?
No heartburns, just use Reviewed-by.
Thanks
Andre
Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sosnowski, Maciej
Acked-byRE: [PATCH v2 04/11] async_xor: permit callers to pass in a 'dma/page scribble' region
Quoted 11 lines · Dan Williams
async_xor()needs space to perform dma and page address conversions. In most cases the code can simply reuse the struct page * array because the size of the native pointer matches the size of a dma/page address. In order to support archs wheresizeof(dma_addr_t)is larger thansizeof(struct page *), or to preserve the input parameters, we utilize a memory region passed in by the caller.Since the code is now prepared to handle the case where it cannot perform address conversions on the stack, we no longer need the !HIGHMEM64G dependency in
drivers/dma/Kconfig.
Quoted 7 lines
[ Impact: don't clobber input buffers for address conversions ]
Signed-off-by Dan Williams <[email protected]>---
crypto/async_tx/async_xor.c | 59 ++++++++++++++++++++-----------------------drivers/dma/Kconfig| 2 +2 files changed, 29 insertions(+), 32 deletions(-)
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 5/11 md/raid5: add scribble region for buffer lists
In reply to Dan WilliamsHang some memory off of each stripe_head which can be used for storing the buffer lists used in parity calculations. Include space for dma address conversions and pass that to async_tx via the async_submit_ctl.scribble pointer.
[ Impact: move memory pressure from stack to heap ]
--- drivers/md/raid5.c | 61 ++++++++++++++++++++++++++++++++++++++++++---------- drivers/md/raid5.h | 5 ++++ 2 files changed, 54 insertions(+), 12 deletions(-)
drivers/md/raid5.c +49 -12
@@ -275,6 +275,9 @@ static void shrink_buffers(struct stripe_head *sh, int num)struct page *p;int i; +kfree (sh->scribble); + sh->scribble =NULL ; +for (i=0 ; i<num ; i++) { p = sh->dev[i].page;if (!p)
@@ -284,10 +287,26 @@ static void shrink_buffers(struct stripe_head *sh, int num) } } +static size_t scribble_len (int num) +{ +size_t len; + +/* return enough space for an array of page pointers and dma +* addresses for the ddf raid6 layout +*/ + len =sizeof (struct page *) * (num+2 ) +sizeof (addr_conv_t) * (num+2 ); + +return len; +} +static int grow_buffers (struct stripe_head *sh,int num) {int i; + sh->scribble =kmalloc (scribble_len (num),GFP_KERNEL ); +if (!sh->scribble) +return 1 ; +for (i=0 ; i<num; i++) {struct page *page;
@@ -641,11 +660,16 @@ static void ops_complete_compute5(void *stripe_head_ref)release_stripe (sh); } +/* return a pointer to the address conversion region of the scribble buffer */ +static addr_conv_t *sh_to_addr_conv (struct stripe_head *sh) +{ +return sh->scribble +sizeof (struct page *) * (sh->disks +2 ); +} +static struct dma_async_tx_descriptor *ops_run_compute5 (struct stripe_head *sh) { -/* kernel stack size limits the total number of disks */ int disks = sh->disks; -struct page *xor_srcs[disks]; +struct page **xor_srcs = sh->scribble;int target = sh->ops.target;struct r5dev *tgt = &sh->dev[target];struct page *xor_dest = tgt->page;
@@ -665,7 +689,7 @@ static struct dma_async_tx_descriptor *ops_run_compute5(struct stripe_head *sh)atomic_inc (&sh->count);init_async_submit (&submit,ASYNC_TX_XOR_ZERO_DST ,NULL , - ops_complete_compute5, sh,NULL ); + ops_complete_compute5, sh,sh_to_addr_conv (sh));if (unlikely (count ==1 )) tx =async_memcpy (xor_dest, xor_srcs[0 ],0 ,0 ,STRIPE_SIZE , &submit);else
@@ -685,9 +709,8 @@ static void ops_complete_prexor(void *stripe_head_ref)static struct dma_async_tx_descriptor *ops_run_prexor (struct stripe_head *sh,struct dma_async_tx_descriptor *tx) { -/* kernel stack size limits the total number of disks */ int disks = sh->disks; -struct page *xor_srcs[disks]; +struct page **xor_srcs = sh->scribble;int count =0 , pd_idx = sh->pd_idx, i;struct async_submit_ctl submit;
@@ -705,7 +728,7 @@ ops_run_prexor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx) }init_async_submit (&submit,ASYNC_TX_XOR_DROP_DST , tx, - ops_complete_prexor, sh,NULL ); + ops_complete_prexor, sh,sh_to_addr_conv (sh)); tx =async_xor (xor_dest, xor_srcs,0 , count,STRIPE_SIZE , &submit);return tx;
@@ -776,9 +799,8 @@ static void ops_complete_postxor(void *stripe_head_ref)static void ops_run_postxor (struct stripe_head *sh,struct dma_async_tx_descriptor *tx) { -/* kernel stack size limits the total number of disks */ int disks = sh->disks; -struct page *xor_srcs[disks]; +struct page **xor_srcs = sh->scribble;struct async_submit_ctl submit;int count =0 , pd_idx = sh->pd_idx, i;struct page *xor_dest;
@@ -818,7 +840,8 @@ ops_run_postxor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)atomic_inc (&sh->count); -init_async_submit (&submit, flags, tx, ops_complete_postxor, sh,NULL ); +init_async_submit (&submit, flags, tx, ops_complete_postxor, sh, +sh_to_addr_conv (sh));if (unlikely (count ==1 )) tx =async_memcpy (xor_dest, xor_srcs[0 ],0 ,0 ,STRIPE_SIZE , &submit);else
@@ -839,9 +862,8 @@ static void ops_complete_check(void *stripe_head_ref)static void ops_run_check (struct stripe_head *sh) { -/* kernel stack size limits the total number of disks */ int disks = sh->disks; -struct page *xor_srcs[disks]; +struct page **xor_srcs = sh->scribble;struct dma_async_tx_descriptor *tx;struct async_submit_ctl submit;
@@ -857,7 +879,7 @@ static void ops_run_check(struct stripe_head *sh) xor_srcs[count++] = dev->page; } -init_async_submit (&submit,0 ,NULL ,NULL ,NULL ,NULL ); +init_async_submit (&submit,0 ,NULL ,NULL ,NULL ,sh_to_addr_conv (sh)); tx =async_xor_val (xor_dest, xor_srcs,0 , count,STRIPE_SIZE , &sh->ops.zero_sum_result, &submit);
@@ -871,6 +893,7 @@ static void raid5_run_ops(struct stripe_head *sh, unsigned long ops_request)int overlap_clear =0 , i, disks = sh->disks;struct dma_async_tx_descriptor *tx =NULL ; +mutex_lock (&sh->scribble_lock);if (test_bit (STRIPE_OP_BIOFILL , &ops_request)) {ops_run_biofill (sh); overlap_clear++;
@@ -903,6 +926,7 @@ static void raid5_run_ops(struct stripe_head *sh, unsigned long ops_request)if (test_and_clear_bit (R5_Overlap, &dev->flags))wake_up (&sh->raid_conf->wait_for_overlap); } +mutex_unlock (&sh->scribble_lock); }static int grow_one_stripe (raid5_conf_t *conf)
@@ -914,6 +938,7 @@ static int grow_one_stripe(raid5_conf_t *conf)memset (sh,0 ,sizeof (*sh) + (conf->raid_disks-1 )*sizeof (struct r5dev )); sh->raid_conf = conf;spin_lock_init (&sh->lock); +mutex_init (&sh->scribble_lock);if (grow_buffers (sh, conf->raid_disks)) {shrink_buffers (sh, conf->raid_disks);
@@ -1007,6 +1032,7 @@ static int resize_stripes(raid5_conf_t *conf, int newsize) nsh->raid_conf = conf;spin_lock_init (&nsh->lock); +mutex_init (&nsh->scribble_lock);list_add (&nsh->lru, &newstripes); }
@@ -1038,6 +1064,7 @@ static int resize_stripes(raid5_conf_t *conf, int newsize) nsh->dev[i].page = osh->dev[i].page;for ( ; i<newsize; i++) nsh->dev[i].page =NULL ; + nsh->scribble = osh->scribble;kmem_cache_free (conf->slab_cache, osh); }kmem_cache_destroy (conf->slab_cache);
@@ -1058,8 +1085,18 @@ static int resize_stripes(raid5_conf_t *conf, int newsize)/* Step 4, return new stripes to service */ while (!list_empty (&newstripes)) { +void *scribble; + nsh = list_entry(newstripes.next, struct stripe_head, lru);list_del_init (&nsh->lru); + + scribble =kmalloc (scribble_len (newsize),GFP_NOIO ); +if (scribble) { +kfree (nsh->scribble); + nsh->scribble = scribble; + }else + err = -ENOMEM ; +for (i=conf->raid_disks; i < newsize; i++)if (nsh->dev[i].page ==NULL ) {struct page *p =alloc_page (GFP_NOIO );
drivers/md/raid5.h +5 -0
@@ -211,6 +211,11 @@ struct stripe_head {int disks;/* disks in stripe */ enum check_states check_state;enum reconstruct_states reconstruct_state; +void *scribble;/* space for constructing buffer +* lists and performing address +* conversions +*/ +struct mutex scribble_lock;/* no concurrent scribbling */ /* stripe_operations * @target - STRIPE_OP_COMPUTE_BLK target */
Re: [PATCH v2 05/11] md/raid5: add scribble region for buffer lists
drivers/md/raid5.c static void shrink_buffers(struct stripe_head *sh, int num) · line 300
Quoted 11 lines · Dan Williams
drivers/md/raid5.c +11
-0+static size_t scribble_len (int num) +{ +size_t len; + +/* return enough space for an array of page pointers and dma +* addresses for the ddf raid6 layout +*/ + len =sizeof (struct page *) * (num+2 ) +sizeof (addr_conv_t) * (num+2 ); + +return len; +}
The comment is a bit misleading as the function only returns the _amount_ of space needed. It should probably also explain the meaning of the "+2".
drivers/md/raid5.c static void ops_complete_compute5(void *stripe_head_ref) · line 667
Quoted 5 lines
drivers/md/raid5.c +5
-0+/* return a pointer to the address conversion region of the scribble buffer */ +static addr_conv_t *sh_to_addr_conv (struct stripe_head *sh) +{ +return sh->scribble +sizeof (struct page *) * (sh->disks +2 ); +}
Maybe it's safer to return NULL if sh->scribble is NULL.
Thanks
Andre
Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
Re: [PATCH v2 05/11] md/raid5: add scribble region for buffer lists
Quoted 17 lines · Andre Noll
drivers/md/raid5.c static void shrink_buffers(struct stripe_head *sh, int num) · line 300
Quoted 11 lines · Dan Williams
drivers/md/raid5.c +11
-0+static size_t scribble_len (int num) +{ + size_t len; + + /* return enough space for an array of page pointers and dma + * addresses for the ddf raid6 layout + */ + len = sizeof(struct page *) * (num+2) + sizeof(addr_conv_t) * (num+2); + + return len; +}The comment is a bit misleading as the function only returns the _amount_ of space needed. It should probably also explain the meaning of the "+2".
Ok I updated this to:
/**
* scribble_len - return the required size of the scribble region
* @num - total number of disks in the array
*
* The size must be enough to contain:
* 1/ a struct page pointer for each device in the array +2
* 2/ room to convert each entry in (1) to its corresponding dma
* (dma_map_page()) or page (page_address()) address.
*
* Note: the +2 is for the destination buffers of the ddf/raid6 case where we
* calculate over all devices (not just the data blocks), using zeros in place
* of the P and Q blocks.
*/
Quoted 8 lines
drivers/md/raid5.c static void ops_complete_compute5(void *stripe_head_ref) · line 667
Quoted 6 lines
drivers/md/raid5.c +5
-0+/* return a pointer to the address conversion region of the scribble buffer */ +static addr_conv_t *sh_to_addr_conv (struct stripe_head *sh) +{ + return sh->scribble + sizeof(struct page *) * (sh->disks + 2); +}Maybe it's safer to return NULL if
sh->scribbleis NULL.
...yes, and a big fat warning, because it should never be NULL.
Thanks for the review,
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/11] md/raid5: add scribble region for buffer lists
In reply to Dan WilliamsQuoted 6 lines · On Monday May 18, [email protected] wrote:
Hang some memory off of each
stripe_headwhich can be used for storing the buffer lists used in parity calculations. Include space for dma address conversions and pass that toasync_txvia the async_submit_ctl.scribble pointer.[ Impact: move memory pressure from stack to heap ]
I've finally had a look at this and I cannot say that I like it.
We don't really need one scribble-buffer per stripe_head. And in fact, that isn't even enough because you find you need a mutex to avoid multiple-use.
We really want one scribble-buffer per thread, or per CPU, or something like that.
You could possibly handle it a bit like ->spare_page, though we cope with that being NULL some times, and you might not be able to do that with scribble-buffer.
How do the async-raid6 patches cope with possible multiple users of ->spare_page now that the computations are async and so possible in parallel?
Maybe a little mempool would be best?.... though given that in most cases, the stack solution is really quite adequate it would be good to make sure the replacement isn't too heavy-weight....
I'm not sure what would be best, but I really don't like the current proposal.
NeilBrown
Quoted 212 lines
Signed-off-by Dan Williams <[email protected]>--- drivers/md/raid5.c | 61 ++++++++++++++++++++++++++++++++++++++++++---------- drivers/md/raid5.h | 5 ++++ 2 files changed, 54 insertions(+), 12 deletions(-)drivers/md/raid5.c +49
-12@@ -275,6 +275,9 @@ static void shrink_buffers(struct stripe_head *sh, int num)struct page *p;int i; +kfree (sh->scribble); + sh->scribble =NULL ; +for (i=0 ; i<num ; i++) { p = sh->dev[i].page;if (!p)@@ -284,10 +287,26 @@ static void shrink_buffers(struct stripe_head *sh, int num) } } +static size_t scribble_len (int num) +{ +size_t len; + +/* return enough space for an array of page pointers and dma +* addresses for the ddf raid6 layout +*/ + len =sizeof (struct page *) * (num+2 ) +sizeof (addr_conv_t) * (num+2 ); + +return len; +} +static int grow_buffers (struct stripe_head *sh,int num) {int i; + sh->scribble =kmalloc (scribble_len (num),GFP_KERNEL ); +if (!sh->scribble) +return 1 ; +for (i=0 ; i<num; i++) {struct page *page;@@ -641,11 +660,16 @@ static void ops_complete_compute5(void *stripe_head_ref)release_stripe (sh); } +/* return a pointer to the address conversion region of the scribble buffer */ +static addr_conv_t *sh_to_addr_conv (struct stripe_head *sh) +{ +return sh->scribble +sizeof (struct page *) * (sh->disks +2 ); +} +static struct dma_async_tx_descriptor *ops_run_compute5 (struct stripe_head *sh) { -/* kernel stack size limits the total number of disks */ int disks = sh->disks; -struct page *xor_srcs[disks]; +struct page **xor_srcs = sh->scribble;int target = sh->ops.target;struct r5dev *tgt = &sh->dev[target];struct page *xor_dest = tgt->page;@@ -665,7 +689,7 @@ static struct dma_async_tx_descriptor *ops_run_compute5(struct stripe_head *sh)atomic_inc (&sh->count);init_async_submit (&submit,ASYNC_TX_XOR_ZERO_DST ,NULL , - ops_complete_compute5, sh,NULL ); + ops_complete_compute5, sh,sh_to_addr_conv (sh));if (unlikely (count ==1 )) tx =async_memcpy (xor_dest, xor_srcs[0 ],0 ,0 ,STRIPE_SIZE , &submit);else @@ -685,9 +709,8 @@ static void ops_complete_prexor(void *stripe_head_ref)static struct dma_async_tx_descriptor *ops_run_prexor (struct stripe_head *sh,struct dma_async_tx_descriptor *tx) { -/* kernel stack size limits the total number of disks */ int disks = sh->disks; -struct page *xor_srcs[disks]; +struct page **xor_srcs = sh->scribble;int count =0 , pd_idx = sh->pd_idx, i;struct async_submit_ctl submit;@@ -705,7 +728,7 @@ ops_run_prexor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx) }init_async_submit (&submit,ASYNC_TX_XOR_DROP_DST , tx, - ops_complete_prexor, sh,NULL ); + ops_complete_prexor, sh,sh_to_addr_conv (sh)); tx =async_xor (xor_dest, xor_srcs,0 , count,STRIPE_SIZE , &submit);return tx;@@ -776,9 +799,8 @@ static void ops_complete_postxor(void *stripe_head_ref)static void ops_run_postxor (struct stripe_head *sh,struct dma_async_tx_descriptor *tx) { -/* kernel stack size limits the total number of disks */ int disks = sh->disks; -struct page *xor_srcs[disks]; +struct page **xor_srcs = sh->scribble;struct async_submit_ctl submit;int count =0 , pd_idx = sh->pd_idx, i;struct page *xor_dest;@@ -818,7 +840,8 @@ ops_run_postxor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)atomic_inc (&sh->count); -init_async_submit (&submit, flags, tx, ops_complete_postxor, sh,NULL ); +init_async_submit (&submit, flags, tx, ops_complete_postxor, sh, +sh_to_addr_conv (sh));if (unlikely (count ==1 )) tx =async_memcpy (xor_dest, xor_srcs[0 ],0 ,0 ,STRIPE_SIZE , &submit);else @@ -839,9 +862,8 @@ static void ops_complete_check(void *stripe_head_ref)static void ops_run_check (struct stripe_head *sh) { -/* kernel stack size limits the total number of disks */ int disks = sh->disks; -struct page *xor_srcs[disks]; +struct page **xor_srcs = sh->scribble;struct dma_async_tx_descriptor *tx;struct async_submit_ctl submit;@@ -857,7 +879,7 @@ static void ops_run_check(struct stripe_head *sh) xor_srcs[count++] = dev->page; } -init_async_submit (&submit,0 ,NULL ,NULL ,NULL ,NULL ); +init_async_submit (&submit,0 ,NULL ,NULL ,NULL ,sh_to_addr_conv (sh)); tx =async_xor_val (xor_dest, xor_srcs,0 , count,STRIPE_SIZE , &sh->ops.zero_sum_result, &submit);@@ -871,6 +893,7 @@ static void raid5_run_ops(struct stripe_head *sh, unsigned long ops_request)int overlap_clear =0 , i, disks = sh->disks;struct dma_async_tx_descriptor *tx =NULL ; +mutex_lock (&sh->scribble_lock);if (test_bit (STRIPE_OP_BIOFILL , &ops_request)) {ops_run_biofill (sh); overlap_clear++;@@ -903,6 +926,7 @@ static void raid5_run_ops(struct stripe_head *sh, unsigned long ops_request)if (test_and_clear_bit (R5_Overlap, &dev->flags))wake_up (&sh->raid_conf->wait_for_overlap); } +mutex_unlock (&sh->scribble_lock); }static int grow_one_stripe (raid5_conf_t *conf)@@ -914,6 +938,7 @@ static int grow_one_stripe(raid5_conf_t *conf)memset (sh,0 ,sizeof (*sh) + (conf->raid_disks-1 )*sizeof (struct r5dev )); sh->raid_conf = conf;spin_lock_init (&sh->lock); +mutex_init (&sh->scribble_lock);if (grow_buffers (sh, conf->raid_disks)) {shrink_buffers (sh, conf->raid_disks);@@ -1007,6 +1032,7 @@ static int resize_stripes(raid5_conf_t *conf, int newsize) nsh->raid_conf = conf;spin_lock_init (&nsh->lock); +mutex_init (&nsh->scribble_lock);list_add (&nsh->lru, &newstripes); }@@ -1038,6 +1064,7 @@ static int resize_stripes(raid5_conf_t *conf, int newsize) nsh->dev[i].page = osh->dev[i].page;for ( ; i<newsize; i++) nsh->dev[i].page =NULL ; + nsh->scribble = osh->scribble;kmem_cache_free (conf->slab_cache, osh); }kmem_cache_destroy (conf->slab_cache);@@ -1058,8 +1085,18 @@ static int resize_stripes(raid5_conf_t *conf, int newsize)/* Step 4, return new stripes to service */ while (!list_empty (&newstripes)) { +void *scribble; + nsh = list_entry(newstripes.next, struct stripe_head, lru);list_del_init (&nsh->lru); + + scribble =kmalloc (scribble_len (newsize),GFP_NOIO ); +if (scribble) { +kfree (nsh->scribble); + nsh->scribble = scribble; + }else + err = -ENOMEM ; +for (i=conf->raid_disks; i < newsize; i++)if (nsh->dev[i].page ==NULL ) {struct page *p =alloc_page (GFP_NOIO );drivers/md/raid5.h +5
-0@@ -211,6 +211,11 @@ struct stripe_head {int disks;/* disks in stripe */ enum check_states check_state;enum reconstruct_states reconstruct_state; +void *scribble;/* space for constructing buffer +* lists and performing address +* conversions +*/ +struct mutex scribble_lock;/* no concurrent scribbling */ /* stripe_operations * @target - STRIPE_OP_COMPUTE_BLK target */
Re: [PATCH v2 05/11] md/raid5: add scribble region for buffer lists
Quoted 6 lines · Neil Brown
Quoted 5 lines · On Monday May 18, [email protected] wrote:
Hang some memory off of each
stripe_headwhich can be used for storing the buffer lists used in parity calculations. Include space for dma address conversions and pass that toasync_txvia the async_submit_ctl.scribble pointer.
Quoted 7 lines
Quoted 1 line
[ Impact: move memory pressure from stack to heap ]
I've finally had a look at this and I cannot say that I like it.
We don't really need one scribble-buffer per
stripe_head. And in fact, that isn't even enough because you find you need a mutex to avoid multiple-use.
The mutex is probably not necessary, just need to audit the stripe_operations state machine to make sure threads don't overlap in raid_run_ops()... but the point is mute when we move to per-cpu resources.
Quoted 2 lines
We really want one scribble-buffer per thread, or per CPU, or something like that.
One of the design goals was to prevent the occurrence of the softlockup watchdog events which seem to trigger on large raid6 resyncs. A per-cpu scheme would still require preempt_disable() while the calculation is active, so perhaps we just need a call to cond_resched() in raid5d to appease the scheduler.
Quoted 6 lines
You could possibly handle it a bit like ->spare_page, though we cope with that being NULL some times, and you might not be able to do that with scribble-buffer.
How do the async-raid6 patches cope with possible multiple users of ->spare_page now that the computations are async and so possible in parallel?
Currently the code just takes a spare page lock.
Quoted 8 lines
Maybe a little mempool would be best?.... though given that in most cases, the stack solution is really quite adequate it would be good to make sure the replacement isn't too heavy-weight....
I'm not sure what would be best, but I really don't like the current proposal.
I'll take a look at a per-cpu implementation.
Thanks,
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/11] md/raid5: add scribble region for buffer lists
Quoted 5 lines · Dan Williams
One of the design goals was to prevent the occurrence of the softlockup watchdog events which seem to trigger on large raid6 resyncs. A per-cpu scheme would still require
preempt_disable()while the calculation is active, so perhaps we just need a call tocond_resched()in raid5d to appease the scheduler.
FWIW we added this to the patches shipped with Lustre:
drivers/md/raid5.c +2 -0
@@ -2987,6 +2987,8 @@ static void raid5d (mddev_t *mddev)handle_stripe (sh, conf->spare_page);release_stripe (sh); +cond_resched (); +spin_lock_irq (&conf->device_lock); }PRINTK ("%d stripes handled\n" , handled);
I thought most of these issues were gone in more recent kernels, but we haven't tested RAID on anything other than RHEL 4+5 extensively (Lustre doesn't support sufficiently new kernels yet.)
Cheers,
Jody
PATCH v2 6/11 async_tx: add sum check flags
In reply to Dan WilliamsReplace the flat zero_sum_result with a collection of flags to contain the P (xor) zero-sum result, and the soon to be utilized Q (raid6 reed solomon syndrome) zero-sum result. Use the SUM_CHECK_ namespace instead of DMA_ since these flags will be used on non-dma-zero-sum enabled platforms.
--- arch/arm/include/asm/hardware/iop3xx-adma.h | 5 +++-- arch/arm/mach-iop13xx/include/mach/adma.h | 12 +++++++----- crypto/async_tx/async_xor.c | 4 ++-- drivers/md/raid5.c | 2 +- drivers/md/raid5.h | 5 +++-- include/linux/async_tx.h | 2 +- include/linux/dmaengine.h | 21 ++++++++++++++++++++- 7 files changed, 37 insertions(+), 14 deletions(-)
arch/arm/include/asm/hardware/iop3xx-adma.h +3 -2
@@ -756,13 +756,14 @@ static inline void iop_desc_set_block_fill_val(struct iop_adma_desc_slot *desc, hw_desc->src[0 ] = val; } -static inline int iop_desc_get_zero_result (struct iop_adma_desc_slot *desc) +static inline enum sum_check_flags +iop_desc_get_zero_result (struct iop_adma_desc_slot *desc) {struct iop3xx_desc_aau *hw_desc = desc->hw_desc;struct iop3xx_aau_desc_ctrl desc_ctrl = hw_desc->desc_ctrl_field;iop_paranoia (!(desc_ctrl.tx_complete && desc_ctrl.zero_result_en)); -return desc_ctrl.zero_result_err; +return desc_ctrl.zero_result_err <<SUM_CHECK_P ; }static inline void iop_chan_append (struct iop_adma_chan *chan)
arch/arm/mach-iop13xx/include/mach/adma.h +7 -5
@@ -428,18 +428,20 @@ static inline void iop_desc_set_block_fill_val(struct iop_adma_desc_slot *desc, hw_desc->block_fill_data = val; } -static inline int iop_desc_get_zero_result (struct iop_adma_desc_slot *desc) +static inline enum sum_check_flags +iop_desc_get_zero_result (struct iop_adma_desc_slot *desc) {struct iop13xx_adma_desc_hw *hw_desc = desc->hw_desc;struct iop13xx_adma_desc_ctrl desc_ctrl = hw_desc->desc_ctrl_field;struct iop13xx_adma_byte_count byte_count = hw_desc->byte_count_field; +enum sum_check_flags flags;BUG_ON (!(byte_count.tx_complete && desc_ctrl.zero_result)); -if (desc_ctrl.pq_xfer_en) -return byte_count.zero_result_err_q; -else -return byte_count.zero_result_err; + flags = byte_count.zero_result_err_q <<SUM_CHECK_Q ; + flags |= byte_count.zero_result_err <<SUM_CHECK_P ; + +return flags; }static inline void iop_chan_append (struct iop_adma_chan *chan)
crypto/async_tx/async_xor.c +2 -2
@@ -234,7 +234,7 @@ static int page_is_zero(struct page *p, unsigned int offset, size_t len)*/ struct dma_async_tx_descriptor *async_xor_val (struct page *dest,struct page **src_list,unsigned int offset, -int src_cnt,size_t len,u32 *result, +int src_cnt,size_t len,enum sum_check_flags *result,struct async_submit_ctl *submit) { struct dma_chan *chan = async_tx_find_channel(submit, DMA_XOR_VAL,
@@ -291,7 +291,7 @@ async_xor_val(struct page *dest, struct page **src_list, unsigned int offset,async_tx_quiesce (&tx); - *result =page_is_zero (dest, offset, len) ?0 :1 ; + *result = !page_is_zero (dest, offset, len) <<SUM_CHECK_P ;async_tx_sync_epilog (submit); submit->flags = flags_orig;
drivers/md/raid5.c +1 -1
@@ -2569,7 +2569,7 @@ static void handle_parity_checks5(raid5_conf_t *conf, struct stripe_head *sh,* we are done. Otherwise update the mismatch count and repair * parity if !MD_RECOVERY_CHECK */ -if (sh->ops.zero_sum_result ==0 ) +if ((sh->ops.zero_sum_result &SUM_CHECK_P_RESULT ) ==0 )/* parity is correct (on disc, * not in buffer any more) */
drivers/md/raid5.h +3 -2
@@ -2,6 +2,7 @@#define _RAID5_H#include <linux/raid/xor.h> +#include <linux/dmaengine.h> /* *
@@ -220,8 +221,8 @@ struct stripe_head {* @target - STRIPE_OP_COMPUTE_BLK target */ struct stripe_operations { -int target; -u32 zero_sum_result; +int target; +enum sum_check_flags zero_sum_result; } ops;struct r5dev {struct bio req;
include/linux/async_tx.h +1 -1
@@ -148,7 +148,7 @@ async_xor(struct page *dest, struct page **src_list, unsigned int offset,struct dma_async_tx_descriptor *async_xor_val (struct page *dest,struct page **src_list,unsigned int offset, -int src_cnt,size_t len,u32 *result, +int src_cnt,size_t len,enum sum_check_flags *result,struct async_submit_ctl *submit); struct dma_async_tx_descriptor *
include/linux/dmaengine.h +20 -1
@@ -87,6 +87,25 @@ enum dma_ctrl_flags { };/** +* enum sum_check_bits - bit position of pq_check_flags +*/ +enum sum_check_bits { +SUM_CHECK_P =0 , +SUM_CHECK_Q =1 , +}; + +/** +* enum pq_check_flags - result of async_{xor,pq}_zero_sum operations +* @SUM_CHECK_P_RESULT - 1 if xor zero sum error, 0 otherwise +* @SUM_CHECK_Q_RESULT - 1 if reed-solomon zero sum error, 0 otherwise +*/ +enum sum_check_flags { +SUM_CHECK_P_RESULT = (1 <<SUM_CHECK_P ), +SUM_CHECK_Q_RESULT = (1 <<SUM_CHECK_Q ), +}; + + +/** * dma_cap_mask_t - capabilities bitmap modeled after cpumask_t. * See linux/cpumask.h */
@@ -245,7 +264,7 @@ struct dma_device { unsigned int src_cnt, size_t len, unsigned long flags);struct dma_async_tx_descriptor *(*device_prep_dma_xor_val)(struct dma_chan *chan,dma_addr_t *src,unsigned int src_cnt, -size_t len,u32 *result,unsigned long flags); +size_t len,enum sum_check_flags *result,unsigned long flags);struct dma_async_tx_descriptor *(*device_prep_dma_memset)(struct dma_chan *chan,dma_addr_t dest,int value,size_t len,unsigned long flags);
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sosnowski, Maciej
Acked-byRE: [PATCH v2 06/11] async_tx: add sum check flags
Quoted 15 lines · Dan Williams
Replace the flat
zero_sum_resultwith a collection of flags to contain the P (xor) zero-sum result, and the soon to be utilized Q (raid6 reed solomon syndrome) zero-sum result. Use theSUM_CHECK_namespace instead ofDMA_since these flags will be used on non-dma-zero-sum enabled platforms.Signed-off-by Dan Williams <[email protected]>---
arch/arm/include/asm/hardware/iop3xx-adma.h | 5 +++--arch/arm/mach-iop13xx/include/mach/adma.h| 12 +++++++-----crypto/async_tx/async_xor.c| 4 ++--drivers/md/raid5.c| 2 +-drivers/md/raid5.h| 5 +++--include/linux/async_tx.h| 2 +-include/linux/dmaengine.h| 21 ++++++++++++++++++++-
Quoted 1 line
7 files changed, 37 insertions(+), 14 deletions(-)
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 7/11 async_tx: kill needless module_{init|exit}
In reply to Dan WilliamsIf module_init and module_exit are nops then neither need to be defined.
[ Impact: pure cleanup ]
--- crypto/async_tx/async_memcpy.c | 13 ------------- crypto/async_tx/async_memset.c | 13 ------------- crypto/async_tx/async_tx.c | 17 +++-------------- 3 files changed, 3 insertions(+), 40 deletions(-)
crypto/async_tx/async_memcpy.c +0 -13
@@ -87,19 +87,6 @@ async_memcpy(struct page *dest, struct page *src, unsigned int dest_offset, }EXPORT_SYMBOL_GPL (async_memcpy); -static int __init async_memcpy_init (void ) -{ -return 0 ; -} - -static void __exit async_memcpy_exit(void) -{ -do { }while (0 ); -} - -module_init (async_memcpy_init); -module_exit (async_memcpy_exit); -MODULE_AUTHOR ("Intel Corporation" );MODULE_DESCRIPTION ("asynchronous memcpy api" );MODULE_LICENSE ("GPL" );
crypto/async_tx/async_memset.c +0 -13
@@ -78,19 +78,6 @@ async_memset(struct page *dest, int val, unsigned int offset, size_t len, }EXPORT_SYMBOL_GPL (async_memset); -static int __init async_memset_init (void ) -{ -return 0 ; -} - -static void __exit async_memset_exit(void) -{ -do { }while (0 ); -} - -module_init (async_memset_init); -module_exit (async_memset_exit); -MODULE_AUTHOR ("Intel Corporation" );MODULE_DESCRIPTION ("asynchronous memset api" );MODULE_LICENSE ("GPL" );
crypto/async_tx/async_tx.c +3 -14
@@ -42,6 +42,9 @@ static void __exit async_tx_exit(void)async_dmaengine_put (); } +module_init (async_tx_init); +module_exit (async_tx_exit); +/** * __async_tx_find_channel - find a channel to carry out the operation or let * the transaction execute synchronously
@@ -61,17 +64,6 @@ __async_tx_find_channel(struct async_submit_ctl *submit, return async_dma_find_channel(tx_type); }EXPORT_SYMBOL_GPL (__async_tx_find_channel); -#else -static int __init async_tx_init (void ) -{ -printk (KERN_INFO "async_tx: api initialized (sync-only)\n" ); -return 0 ; -} - -static void __exit async_tx_exit(void) -{ -do { }while (0 ); -}#endif
@@ -293,9 +285,6 @@ void async_tx_quiesce(struct dma_async_tx_descriptor **tx) }EXPORT_SYMBOL_GPL (async_tx_quiesce); -module_init (async_tx_init); -module_exit (async_tx_exit); -MODULE_AUTHOR ("Intel Corporation" );MODULE_DESCRIPTION ("Asynchronous Bulk Memory Transactions API" );MODULE_LICENSE ("GPL" );
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sosnowski, Maciej
Acked-byRE: [PATCH v2 07/11] async_tx: kill needless module_{init|exit}
Quoted 10 lines · Dan Williams
If
module_initandmodule_exitare nops then neither need to be defined.[ Impact: pure cleanup ]
Signed-off-by Dan Williams <[email protected]>---
crypto/async_tx/async_memcpy.c | 13 -------------crypto/async_tx/async_memset.c| 13 -------------crypto/async_tx/async_tx.c| 17 +++--------------3 files changed, 3 insertions(+), 40 deletions(-)
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 8/11 async_tx: add support for asynchronous GF multiplication
In reply to Dan Williams[ Based on an original patch by Yuri Tikhonov ]
This adds support for doing asynchronous GF multiplication by adding two additional functions to the async_tx API:
async_gen_syndrome() does simultaneous XOR and Galois field
multiplication of sources.
async_syndrome_val() validates the given source buffers against known P
and Q values.
When a request is made to run async_pq against more than the hardware maximum number of supported sources we need to reuse the previous generated P and Q values as sources into the next operation. Care must be taken to remove Q from P' and P from Q'. For example to perform a 5 source pq op with hardware that only supports 4 sources at a time the following approach is taken:
p, q = PQ(src0, src1, src2, src3, COEF({01}, {02}, {04}, {08})) p', q' = PQ(p, q, q, src4, COEF({00}, {01}, {00}, {10}))
p' = p + q + q + src4 = p + src4
q' = {00}*p + {01}*q + {00}*q + {10}*src4 = q + {10}*src4
Note: 4 is the minimum acceptable maxpq otherwise we punt to synchronous-software path.
The DMA_PREP_CONTINUE flag indicates to the driver to reuse p and q as sources (in the above manner) and fill the remaining slots up to maxpq with the new sources/coefficients.
Note: Some devices have native support for P+Q continuation and can skip this extra work. Devices with this capability can advertise it with dma_set_maxpq. It is up to each driver how the DMA_PREP_CONTINUE flag is honored.
--- arch/arm/mach-iop13xx/setup.c | 2 crypto/async_tx/Kconfig | 4 crypto/async_tx/Makefile | 1 crypto/async_tx/async_pq.c | 399 +++++++++++++++++++++++++++++++++++++++++ crypto/async_tx/async_xor.c | 2 drivers/dma/dmaengine.c | 4 drivers/dma/iop-adma.c | 2 include/linux/async_tx.h | 9 + include/linux/dmaengine.h | 58 +++++- 9 files changed, 472 insertions(+), 9 deletions(-) create mode 100644 crypto/async_tx/async_pq.c
arch/arm/mach-iop13xx/setup.c +1 -1
@@ -506,7 +506,7 @@ void __init iop13xx_platform_init(void)dma_cap_set (DMA_MEMSET , plat_data->cap_mask);dma_cap_set (DMA_MEMCPY_CRC32C , plat_data->cap_mask);dma_cap_set (DMA_INTERRUPT , plat_data->cap_mask); -dma_cap_set (DMA_PQ_XOR , plat_data->cap_mask); +dma_cap_set (DMA_PQ , plat_data->cap_mask);dma_cap_set (DMA_PQ_UPDATE , plat_data->cap_mask);dma_cap_set (DMA_PQ_VAL , plat_data->cap_mask);break ;
crypto/async_tx/Kconfig +4 -0
@@ -14,3 +14,7 @@ config ASYNC_MEMSET tristate select ASYNC_CORE +config ASYNC_PQ + tristate + select ASYNC_CORE +
crypto/async_tx/Makefile +1 -0
@@ -2,3 +2,4 @@ obj-$(CONFIG_ASYNC_CORE) += async_tx.o obj-$(CONFIG_ASYNC_MEMCPY) += async_memcpy.o obj-$(CONFIG_ASYNC_MEMSET) += async_memset.o obj-$(CONFIG_ASYNC_XOR) += async_xor.o +obj-$(CONFIG_ASYNC_PQ) += async_pq.o
crypto/async_tx/async_pq.c new file +399 -0
399 lines not on this page · raw message
crypto/async_tx/async_xor.c +1 -1
@@ -62,7 +62,7 @@ do_async_xor(struct dma_chan *chan, struct page *dest, struct page **src_list,while (src_cnt) { submit->flags = flags_orig; dma_flags =0 ; - xor_src_cnt =min (src_cnt, dma->max_xor); + xor_src_cnt =min (src_cnt, (int )dma->max_xor);/* if we are submitting additional xors, leave the chain open, * clear the callback parameters, and leave the destination * buffer mapped
drivers/dma/dmaengine.c +4 -0
@@ -646,6 +646,10 @@ int dma_async_device_register(struct dma_device *device) !device->device_prep_dma_xor);BUG_ON (dma_has_cap (DMA_XOR_VAL , device->cap_mask) && !device->device_prep_dma_xor_val); +BUG_ON (dma_has_cap (DMA_PQ , device->cap_mask) && + !device->device_prep_dma_pq); +BUG_ON (dma_has_cap (DMA_PQ_VAL , device->cap_mask) && + !device->device_prep_dma_pq_val);BUG_ON (dma_has_cap (DMA_MEMSET , device->cap_mask) && !device->device_prep_dma_memset); BUG_ON(dma_has_cap(DMA_INTERRUPT, device->cap_mask) &&
drivers/dma/iop-adma.c +1 -1
@@ -1257,7 +1257,7 @@ static int __devinit iop_adma_probe(struct platform_device *pdev) dev_printk(KERN_INFO, &pdev->dev, "Intel(R) IOP: " "( %s%s%s%s%s%s%s%s%s%s)\n", - dma_has_cap(DMA_PQ_XOR, dma_dev->cap_mask) ? "pq_xor " : "", + dma_has_cap(DMA_PQ, dma_dev->cap_mask) ? "pq " : "", dma_has_cap(DMA_PQ_UPDATE, dma_dev->cap_mask) ? "pq_update " : "", dma_has_cap(DMA_PQ_VAL, dma_dev->cap_mask) ? "pq_val " : "", dma_has_cap(DMA_XOR, dma_dev->cap_mask) ? "xor " : "",
include/linux/async_tx.h +9 -0
@@ -162,5 +162,14 @@ async_memset(struct page *dest, int val, unsigned int offset,struct dma_async_tx_descriptor *async_trigger_callback (struct async_submit_ctl *submit); +struct dma_async_tx_descriptor * +async_gen_syndrome (struct page **blocks,unsigned int offset,int src_cnt, +size_t len,struct async_submit_ctl *submit); + +struct dma_async_tx_descriptor * +async_syndrome_val (struct page **blocks,unsigned int offset,int src_cnt, +size_t len,enum sum_check_flags *pqres, +struct async_submit_ctl *submit); +void async_tx_quiesce (struct dma_async_tx_descriptor **tx);#endif /* _ASYNC_TX_H_ */
include/linux/dmaengine.h +52 -6
110 lines not on this page · raw message
Re: [PATCH v2 08/11] async_tx: add support for asynchronous GF multiplication
crypto/async_tx/async_pq.c line 53
Quoted 8 lines · Dan Williams
crypto/async_tx/async_pq.c new file +8
-0+/* the struct page *blocks[] parameter passed to async_gen_syndrome() +* and async_syndrome_val() contains the 'P' destination address at +* blocks[disks-2] and the 'Q' destination address at blocks[disks-1] +* +* note: these are macros as they are used a lvalues +*/ +#define P (b, d) (b[d-2]) +#define Q (b, d) (b[d-1])
s/a lvalues/as lvalues
crypto/async_tx/async_pq.c line 101
Quoted 12 lines
crypto/async_tx/async_pq.c new file +12
-0+/* convert source addresses being careful to collapse 'empty' +* sources and update the coefficients accordingly +*/ +for (i =0 , idx =0 ; i < src_cnt; i++) { +if (is_raid6_zero_block (blocks[i])) +continue ; + dma_src[idx] =dma_map_page (dma->dev, blocks[i], offset, len, +DMA_TO_DEVICE ); + coefs[idx] = scfs[i]; + idx++; + } + src_cnt = idx;
If P is disabled, we could further collapse this loop by also skipping sources where the coefficient is zero. Not sure if this would be a win though.
Quoted 10 lines
crypto/async_tx/async_pq.c new file +10
-0+ +while (src_cnt >0 ) { + submit->flags = flags_orig; + pq_src_cnt =min (src_cnt,dma_maxpq (dma, dma_flags)); +/* if we are submitting additional pqs, leave the chain open, +* clear the callback parameters, and leave the destination +* buffers mapped +*/ +if (src_cnt > pq_src_cnt) { + submit->flags &= ~ASYNC_TX_ACK ;
crypto/async_tx/async_pq.c line 121
Quoted 10 lines
crypto/async_tx/async_pq.c new file +10
-0+ dma_flags |=DMA_COMPL_SKIP_DEST_UNMAP ; + submit->cb_fn =NULL ; + submit->cb_param =NULL ; + }else { + dma_flags &= ~DMA_COMPL_SKIP_DEST_UNMAP ; + submit->cb_fn = cb_fn_orig; + submit->cb_param = cb_param_orig; + } +if (submit->cb_fn) + dma_flags |=DMA_PREP_INTERRUPT ;
The last if() can go to the else branch.
crypto/async_tx/async_pq.c line 163
Quoted 12 lines
crypto/async_tx/async_pq.c new file +12
-0+/** +* do_sync_gen_syndrome - synchronously calculate a raid6 syndrome +*/ +static void +do_sync_gen_syndrome (struct page **blocks,unsigned int offset,int disks, +size_t len,struct async_submit_ctl *submit) +{ +void **srcs; +int i; + +if (submit->scribble) + srcs = (void **) submit->scribble;
Unnecessary cast.
crypto/async_tx/async_pq.c line 170
Quoted 7 lines
crypto/async_tx/async_pq.c new file +7
-0+else + srcs = (void **) blocks; + +for (i =0 ; i < disks; i++) { +if (is_raid6_zero_block (blocks[i])) { +BUG_ON (i > disks -3 );/* P or Q can't be zero */ + srcs[i] = (void *) blocks[i];
Another Unnecessary cast.
Otherwise, this patch looks also very nice.
Thanks
Andre
Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
Re: [PATCH v2 08/11] async_tx: add support for asynchronous GF multiplication
Quoted 3 lines · Andre Noll
If P is disabled, we could further collapse this loop by also skipping sources where the coefficient is zero. Not sure if this would be a win though.
It's ok to leave this as an optimization opportunity for the caller.
Quoted 4 lines
Otherwise, this patch looks also very nice.
Thanks
Andre
Thanks, and the other cleanups were addressed as well.
A message that is not in the archive (sent off-list or not yet received): [email protected]
RE: [PATCH v2 08/11] async_tx: add support for asynchronous GF multiplication
Quoted 51 lines · Dan Williams
[ Based on an original patch by Yuri Tikhonov ]
This adds support for doing asynchronous GF multiplication by adding two additional functions to the
async_txAPI:async_gen_syndrome() does simultaneous XOR and Galois field multiplication of sources.
async_syndrome_val() validates the given source buffers against known P and Q values.
When a request is made to run
async_pqagainst more than the hardware maximum number of supported sources we need to reuse the previous generated P and Q values as sources into the next operation. Care must be taken to remove Q from P' and P from Q'. For example to perform a 5 source pq op with hardware that only supports 4 sources at a time the following approach is taken:p, q =
PQ(src0, src1, src2, src3, COEF({01}, {02}, {04}, {08}))p', q' =PQ(p, q, q, src4, COEF({00}, {01}, {00}, {10}))p' = p + q + q + src4 = p + src4
q' = {00}*p + {01}*q + {00}*q + {10}*src4 = q + {10}*src4Note: 4 is the minimum acceptable maxpq otherwise we punt to synchronous-software path.
The
DMA_PREP_CONTINUEflag indicates to the driver to reuse p and q as sources (in the above manner) and fill the remaining slots up to maxpq with the new sources/coefficients.Note: Some devices have native support for P+Q continuation and can skip this extra work. Devices with this capability can advertise it with
dma_set_maxpq. It is up to each driver how theDMA_PREP_CONTINUEflag is honored.Signed-off-by Yuri Tikhonov <[email protected]>Signed-off-by Ilya Yanok <[email protected]>Signed-off-by Dan Williams <[email protected]>---
arch/arm/mach-iop13xx/setup.c | 2
crypto/async_tx/Kconfig | 4
crypto/async_tx/Makefile | 1
crypto/async_tx/async_pq.c | 399 +++++++++++++++++++++++++++++++++++++++++crypto/async_tx/async_xor.c| 2
drivers/dma/dmaengine.c | 4
drivers/dma/iop-adma.c | 2
include/linux/async_tx.h | 9 +
include/linux/dmaengine.h | 58 +++++-9 files changed, 472 insertions(+), 9 deletions(-)create mode 100644
crypto/async_tx/async_pq.c
(...)
Quoted 7 lines
+ /* Since we have clobbered the
src_listwe are committed
+ * to doing this asynchronously. Drivers force forward
+ * progress in case they can not provide a descriptor
+ */
+ for (;;) {
+ tx = dma->device_prep_dma_pq(chan,dma_dest,
+ &dma_src[src_off],
Quoted 8 lines
+ pq_src_cnt,
+ &coefs[src_off], len,
+ dma_flags);
+ if (likely(tx))
+ break;
+async_tx_quiesce(&submit->depend_tx);
+dma_async_issue_pending(chan);
+ }
How about adding a timeout to the loop in case we do not get a descriptor at all for some reason?
Quoted 11 lines
+ for (;;) {
+ tx = device->device_prep_dma_pq_val(chan, pq,dma_src,
+ disks - 2,
+raid6_gfexp,
+ len, pqres,
+dma_flags);
+ if (likely(tx))
+ break;
+async_tx_quiesce(&submit->depend_tx);
+dma_async_issue_pending(chan);
+ }
Same as above...
Thanks,
Maciej--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/11] async_tx: add support for asynchronous GF multiplication
2009/5/29 Sosnowski, Maciej <[email protected]>:
Quoted 17 lines
Quoted 7 lines · Dan Williams
+ /* Since we have clobbered the
src_listwe are committed
+ * to doing this asynchronously. Drivers force forward
+ * progress in case they can not provide a descriptor
+ */
+ for (;;) {
+ tx = dma->device_prep_dma_pq(chan,dma_dest,
+ &dma_src[src_off],Quoted 8 lines
+ pq_src_cnt,
+ &coefs[src_off], len,
+ dma_flags);
+ if (likely(tx))
+ break;
+async_tx_quiesce(&submit->depend_tx);
+dma_async_issue_pending(chan);
+ }
Quoted 2 lines
How about adding a timeout to the loop in case we do not get a descriptor at all for some reason?
There is an embedded timeout in async_tx_quiesce(). However, now that we have the ->scribble pointer a future patch could make it a requirement of the api. With that in place we would always be able to fall back to the synchronous at any point because the input parameters would be preserved.
Thanks,
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 9/11 async_tx: add support for asynchronous RAID6 recovery operations
In reply to Dan Williams async_raid6_2data_recov() recovers two data disk failures
async_raid6_datap_recov() recovers a data disk and the P disk
These routines are a port of the synchronous versions found in drivers/md/raid6recov.c. The primary difference is breaking out the xor operations into separate calls to async_xor. Two helper routines are introduced to perform scalar multiplication where needed. async_sum_product() multiplies two sources by scalar coefficients and then sums (xor) the result. async_mult() simply multiplies a single source by a scalar.
[ Impact: asynchronous raid6 recovery routines for 2data and datap cases ]
--- crypto/async_tx/Kconfig | 5 + crypto/async_tx/Makefile | 1 crypto/async_tx/async_raid6_recov.c | 292 +++++++++++++++++++++++++++++++++++ include/linux/async_tx.h | 8 + 4 files changed, 306 insertions(+), 0 deletions(-) create mode 100644 crypto/async_tx/async_raid6_recov.c
crypto/async_tx/Kconfig +5 -0
@@ -18,3 +18,8 @@ config ASYNC_PQ tristate select ASYNC_CORE +config ASYNC_RAID6_RECOV + tristate + select ASYNC_CORE + select ASYNC_PQ +
crypto/async_tx/Makefile +1 -0
@@ -3,3 +3,4 @@ obj-$(CONFIG_ASYNC_MEMCPY) += async_memcpy.o obj-$(CONFIG_ASYNC_MEMSET) += async_memset.o obj-$(CONFIG_ASYNC_XOR) += async_xor.o obj-$(CONFIG_ASYNC_PQ) += async_pq.o +obj-$(CONFIG_ASYNC_RAID6_RECOV) += async_raid6_recov.o
crypto/async_tx/async_raid6_recov.c new file +292 -0
292 lines not on this page · raw message
include/linux/async_tx.h +8 -0
@@ -171,5 +171,13 @@ async_syndrome_val(struct page **blocks, unsigned int offset, int src_cnt, size_t len, enum sum_check_flags *pqres, struct async_submit_ctl *submit); +struct dma_async_tx_descriptor * +async_raid6_2data_recov (int src_num,size_t bytes,int faila,int failb, +struct page **ptrs,struct async_submit_ctl *submit); + +struct dma_async_tx_descriptor * +async_raid6_datap_recov (int src_num,size_t bytes,int faila, +struct page **ptrs,struct async_submit_ctl *submit); +void async_tx_quiesce (struct dma_async_tx_descriptor **tx);#endif /* _ASYNC_TX_H_ */
Re: [PATCH v2 09/11] async_tx: add support for asynchronous RAID6 recovery operations
Quoted 13 lines · Dan Williams
crypto/async_tx/async_raid6_recov.c new file +13
-0+/** +* async_raid6_2data_recov - asynchronously calculate two missing data blocks +* @disks: number of disks in the RAID-6 array +* @bytes: block size +* @faila: first failed drive index +* @failb: second failed drive index +* @blocks: array of source pointers where the last two entries are p and q +* @submit: submission/completion modifiers +*/ +struct dma_async_tx_descriptor * +async_raid6_2data_recov (int disks,size_t bytes,int faila,int failb, +struct page **blocks,struct async_submit_ctl *submit) +{
[...]
crypto/async_tx/async_raid6_recov.c line 155
Quoted 4 lines
crypto/async_tx/async_raid6_recov.c new file +4
-0+/* Dx = A*(P+Pxy) + B*(Q+Qxy) */ + srcs[0 ] = dp; + srcs[1 ] = dq; + coef[0 ] = raid6_gfexi[failb-faila];
Here it's essential that faila < failb. This should either be clearly documented and checked for, or (better) the function should swap faila and failb if they are in the wrong order.
Quoted 11 lines
crypto/async_tx/async_raid6_recov.c new file +11
-0+ p = blocks[disks-2 ]; + q = blocks[disks-1 ]; + +/* Compute syndrome with zero for the missing data page +Use the dead data page as temporary storage for delta q */ + dq = blocks[faila]; + blocks[faila] = (void *)raid6_empty_zero_page; + blocks[disks-1 ] = dq; + + submit->flags &= ~ASYNC_TX_ACK ; + submit->cb_fn =NULL ;
crypto/async_tx/async_raid6_recov.c line 268
Quoted 10 lines
crypto/async_tx/async_raid6_recov.c new file +10
-0+ submit->cb_param =NULL ; + tx =async_gen_syndrome (blocks,0 , disks, bytes, submit); + submit->depend_tx = tx; + +/* Restore pointer table */ + blocks[faila] = dq; + blocks[disks-1 ] = q; + +/* Now, pick the proper data tables */ + coef = raid6_gfinv[raid6_gfexp[faila]];
Pick data tables? The formula for recovering the data block is
(q + dq) / g^faila
(with g being the generator). What this line actually does is computing g^{-faila}.
Regards
Andre
Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
Re: [PATCH v2 09/11] async_tx: add support for asynchronous RAID6 recovery operations
Quoted 17 lines · Andre Noll
Quoted 13 lines · Dan Williams
crypto/async_tx/async_raid6_recov.c new file +13
-0+/** +* async_raid6_2data_recov - asynchronously calculate two missing data blocks +* @disks: number of disks in the RAID-6 array +* @bytes: block size +* @faila: first failed drive index +* @failb: second failed drive index +* @blocks: array of source pointers where the last two entries are p and q +* @submit: submission/completion modifiers +*/ +struct dma_async_tx_descriptor * +async_raid6_2data_recov (int disks,size_t bytes,int faila,int failb, + struct page **blocks, struct async_submit_ctl *submit) +{[...]
Quoted 8 lines
Quoted 4 lines
+ /* Dx = A*(P+Pxy) + B*(Q+Qxy) */
+ srcs[0] = dp;
+ srcs[1] = dq;
+ coef[0] = raid6_gfexi[failb-faila];Here it's essential that faila < failb. This should either be clearly documented and checked for, or (better) the function should swap faila and failb if they are in the wrong order.
Yes, it would be safer if we did not trust the caller to get this right.
Quoted 25 lines
Quoted 12 lines
+ p = blocks[disks-2];
+ q = blocks[disks-1];crypto/async_tx/async_raid6_recov.c new file +9
-0+ + /* Compute syndrome with zero for the missing data page + Use the dead data page as temporary storage for delta q */ + dq = blocks[faila]; + blocks[faila] = (void *)raid6_empty_zero_page; + blocks[disks-1] = dq; + + submit->flags &= ~ASYNC_TX_ACK; + submit->cb_fn = NULL;crypto/async_tx/async_raid6_recov.c line 268
Quoted 10 lines
crypto/async_tx/async_raid6_recov.c new file +10
-0+ submit->cb_param = NULL; + tx = async_gen_syndrome(blocks, 0, disks, bytes, submit); + submit->depend_tx = tx; + + /* Restore pointer table */ + blocks[faila] = dq; + blocks[disks-1] = q; + + /* Now, pick the proper data tables */ + coef = raid6_gfinv[raid6_gfexp[faila]];Pick data tables? The formula for recovering the data block is
Quoted 4 lines
(q + dq) / g^faila
(with g being the generator). What this line actually does is computing g^{-faila}.
Yeah, that is a leftover comment from the original implementation, I'll change it to be more informative.
Thanks,
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sosnowski, Maciej
Acked-byRE: [PATCH v2 09/11] async_tx: add support for asynchronous RAID6 recovery operations
Quoted 18 lines · Dan Williams
async_raid6_2data_recov() recovers two data disk failures
async_raid6_datap_recov() recovers a data disk and the P disk
These routines are a port of the synchronous versions found in
drivers/md/raid6recov.c. The primary difference is breaking out the xor operations into separate calls toasync_xor. Two helper routines are introduced to perform scalar multiplication where needed.async_sum_product()multiplies two sources by scalar coefficients and then sums (xor) the result. async_mult() simply multiplies a single source by a scalar.[ Impact: asynchronous raid6 recovery routines for 2data and datap cases ]
Cc Yuri Tikhonov <[email protected]>Cc Ilya Yanok <[email protected]>Cc H. Peter Anvin <[email protected]>Signed-off-by Dan Williams <[email protected]>
Quoted 7 lines
---
crypto/async_tx/Kconfig | 5 +crypto/async_tx/Makefile| 1crypto/async_tx/async_raid6_recov.c| 292 +++++++++++++++++++++++++++++++++++include/linux/async_tx.h| 8 +4 files changed, 306 insertions(+), 0 deletions(-)create mode 100644
crypto/async_tx/async_raid6_recov.c
With a minor comment:
Quoted 19 lines
crypto/async_tx/async_raid6_recov.c new file +18
-0+static struct dma_async_tx_descriptor * +async_mult (struct page *dest,struct page *src,u8 coef,size_t len, + struct async_submit_ctl *submit) +{ + struct dma_chan *chan = async_tx_find_channel(submit, DMA_PQ, + &dest, 1, srcs, 2, len); + struct dma_device *dma = chan ? chan->device : NULL; + const u8 *qmul; /* Q multiplier table */ + u8 *d, *s; + + if (dma) { + dma_addr_t dma_dest[2]; + dma_addr_t dma_src[1]; + struct device *dev = dma->dev; + struct dma_async_tx_descriptor *tx; + enum dma_ctrl_flags dma_flags = DMA_PREP_PQ_DISABLE_P; + + dma_dest[1] = dma_map_page(dev, dest, 0, len,
DMA_BIDIRECTIONAL);
Quoted 8 lines
+ dma_src[0] =
dma_map_page(dev, src, 0, len, DMA_TO_DEVICE);
+ tx = dma->device_prep_dma_pq(chan,dma_dest,dma_src, 1, &coef,
+ len,dma_flags);
+ if (tx) {
+async_tx_submit(chan, tx, submit);
+ return tx;
+ }
+ }
How about adding "run the operation synchronously" comment at this point, just like it is in async_sum_product?
Maciej--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 10/11 dmatest: add pq support
In reply to Dan WilliamsTest raid6 p+q operations with a simple "always multiply by 1" q calculation to fit into dmatest's current destination verification scheme.
--- drivers/dma/dmatest.c | 26 ++++++++++++++++++++++++++ 1 files changed, 26 insertions(+), 0 deletions(-)
drivers/dma/dmatest.c +26 -0
@@ -43,6 +43,11 @@ module_param(xor_sources, uint, S_IRUGO);MODULE_PARM_DESC (xor_sources,"Number of xor source buffers (default: 3)" ); +static unsigned int pq_sources =3 ; +module_param (pq_sources, uint,S_IRUGO ); +MODULE_PARM_DESC (pq_sources, +"Number of p+q source buffers (default: 3)" ); +/* * Initialization patterns. All bytes in the source buffer has bit 7 * set, all bytes in the destination buffer has bit 7 cleared.
@@ -227,6 +232,7 @@ static int dmatest_func(void *data)dma_cookie_t cookie;enum dma_status status;enum dma_ctrl_flags flags; +u8 pq_coefs[pq_sources];int ret;int src_cnt;int dst_cnt;
@@ -243,6 +249,11 @@ static int dmatest_func(void *data)else if (thread->type ==DMA_XOR ) { src_cnt = xor_sources |1 ;/* force odd to ensure dst = src */ dst_cnt =1 ; + }else if (thread->type ==DMA_PQ ) { + src_cnt = pq_sources |1 ;/* force odd to ensure dst = src */ + dst_cnt =2 ; +for (i =0 ; i < pq_sources; i++) + pq_coefs[i] =1 ; }else goto err_srcs;
@@ -310,6 +321,15 @@ static int dmatest_func(void *data) dma_dsts[0 ] + dst_off, dma_srcs, xor_sources, len, flags); + else if (thread->type == DMA_PQ) { +dma_addr_t dma_pq[dst_cnt]; + +for (i =0 ; i < dst_cnt; i++) + dma_pq[i] = dma_dsts[i] + dst_off; + tx = dev->device_prep_dma_pq (chan, dma_pq, dma_srcs, + pq_sources, pq_coefs, + len, flags); + }if (!tx) {for (i =0 ; i < src_cnt; i++)
@@ -446,6 +466,8 @@ static int dmatest_add_threads(struct dmatest_chan *dtc, enum dma_transaction_ty op ="copy" ; else if (type == DMA_XOR) op ="xor" ; + else if (type == DMA_PQ) + op ="pq" ; else return -EINVAL;
@@ -501,6 +523,10 @@ static int dmatest_add_channel(struct dma_chan *chan) cnt =dmatest_add_threads (dtc,DMA_XOR ); thread_count += cnt >0 ?:0 ; } +if (dma_has_cap (DMA_PQ , dma_dev->cap_mask)) { + cnt =dmatest_add_threads (dtc,DMA_PQ ); + thread_count += cnt >0 ?:0 ; + }pr_info ("dmatest: Started %u threads using %s\n" , thread_count,dma_chan_name (chan));
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sosnowski, Maciej
Acked-byRE: [PATCH v2 10/11] dmatest: add pq support
Quoted 8 lines · Dan Williams
Test raid6 p+q operations with a simple "always multiply by 1" q calculation to fit into dmatest's current destination verification scheme.
Signed-off-by Dan Williams <[email protected]>---
drivers/dma/dmatest.c | 26 ++++++++++++++++++++++++++1 files changed, 26 insertions(+), 0 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 11/11 async_tx: raid6 recovery self test
In reply to Dan WilliamsPort drivers/md/raid6test/test.c to use the async raid6 recovery routines. This is meant as a unit test for raid6 acceleration drivers.
--- crypto/async_tx/Makefile | 1 crypto/async_tx/raid6test.c | 212 +++++++++++++++++++++++++++++++++++++++++++ drivers/md/Kconfig | 13 +++ 3 files changed, 226 insertions(+), 0 deletions(-) create mode 100644 crypto/async_tx/raid6test.c
crypto/async_tx/Makefile +1 -0
@@ -4,3 +4,4 @@ obj-$(CONFIG_ASYNC_MEMSET) += async_memset.o obj-$(CONFIG_ASYNC_XOR) += async_xor.o obj-$(CONFIG_ASYNC_PQ) += async_pq.o obj-$(CONFIG_ASYNC_RAID6_RECOV) += async_raid6_recov.o +obj-$(CONFIG_ASYNC_RAID6_TEST) += raid6test.o
crypto/async_tx/raid6test.c new file +212 -0
212 lines not on this page · raw message
drivers/md/Kconfig +13 -0
@@ -155,6 +155,19 @@ config MD_RAID456 config MD_RAID6_PQ tristate +config ASYNC_RAID6_TEST + tristate "Self test for hardware accelerated raid6 recovery" + depends on MD_RAID6_PQ + select ASYNC_RAID6_RECOV + ---help--- + This is a one-shot self test that permutes through the + recovery of all the possible two disk failure scenarios for a + N-disk array. Recovery is performed with the asynchronous + raid6 recovery routines, and will optionally use an offload + engine if one is available. + + If unsure, say N. + config MD_MULTIPATH tristate "Multipath I/O support" depends on BLK_DEV_MD
Re: [PATCH v2 11/11] async_tx: raid6 recovery self test
crypto/async_tx/raid6test.c line 46
Quoted 5 lines · Dan Williams
crypto/async_tx/raid6test.c new file +5
-0+static void makedata (void ) +{ +int i, j; + +for (i =0 ; i <NDISKS ; i++) {
i < NDISKS - 2 would be sufficient.
crypto/async_tx/raid6test.c line 67
Quoted 11 lines
crypto/async_tx/raid6test.c new file +11
-0+static char disk_type (int d) +{ +switch (d) { +case NDISKS -2 : +return 'P' ; +case NDISKS -1 : +return 'Q' ; +default : +return 'D' ; + } +}
I like this function very much because "if (disk_type(faila) == 'Q')" is so much more readable than "if (faila == num_disks - 1)". It's a pity that we only have it in this test module ;)
crypto/async_tx/raid6test.c line 70
Quoted 2 lines
crypto/async_tx/raid6test.c new file +2
-0+/* Recover two failed blocks. */ +static void raid6_dual_recov (int disks,size_t bytes,int faila,int failb,struct page **ptrs)
As disks is always NDISKS, the disks parameter could be removed.
crypto/async_tx/raid6test.c line 83
Quoted 13 lines
crypto/async_tx/raid6test.c new file +13
-0+{ +struct async_submit_ctl submit; +addr_conv_t addr_conv[NDISKS ]; +struct completion cmp; +struct dma_async_tx_descriptor *tx =NULL ; +enum sum_check_flags result = ~0 ; +bool dataq = false; + +if (faila > failb) +swap (faila, failb); + +init_async_submit (&submit,0 ,NULL ,NULL ,NULL , addr_conv); +if (failb == disks-1 ) {
if (disk_type(failb) == 'Q'). Similar for the other tests in this function.
crypto/async_tx/raid6test.c line 179
Quoted 4 lines
crypto/async_tx/raid6test.c new file +4
-0+/* Generate assumed good syndrome */ +init_completion (&cmp); +init_async_submit (&submit,ASYNC_TX_ACK ,NULL , callback, &cmp, addr_conv); +async_gen_syndrome (dataptrs,0 ,NDISKS ,PAGE_SIZE , &submit);
How hard would it be to also test the fallback code for the synchronous paths? AFAICS this test module wouldn't notice errors in the fallback logic.
drivers/md/Kconfig config MD_RAID456 · line 165
Quoted 8 lines
drivers/md/Kconfig +8
-0+config ASYNC_RAID6_TEST + tristate "Self test for hardware accelerated raid6 recovery" + depends on MD_RAID6_PQ + select ASYNC_RAID6_RECOV + ---help--- + This is a one-shot self test that permutes through the + recovery of all the possible two disk failure scenarios for a + N-disk array. Recovery is performed with the asynchronous
Currently N is 16. So s/N-disk/raid6 perhaps?
Regards
Andre
Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
Re: [PATCH v2 11/11] async_tx: raid6 recovery self test
Quoted 16 lines · Andre Noll
crypto/async_tx/raid6test.c line 67
Quoted 11 lines · Dan Williams
crypto/async_tx/raid6test.c new file +11
-0+static char disk_type (int d) +{ + switch (d) { + case NDISKS-2: + return 'P'; + case NDISKS-1: + return 'Q'; + default: + return 'D'; + } +}I like this function very much because "if (
disk_type(faila)== 'Q')" is so much more readable than "if (faila ==num_disks- 1)". It's a pity that we only have it in this test module ;)
I do not see too many places outside this file where it would be helpful... but an incremental patch for this cleanup would be welcome.
In general this code tries to duplicate the look ofdrivers/md/raid6test/test.c, so any cleanups would be applicable in both locations.
Quoted 3 lines
How hard would it be to also test the fallback code for the synchronous paths? AFAICS this test module wouldn't notice errors in the fallback logic.
It does. Simply omit loading an offload driver and the api will by necessity fallback to the synchronous path.
Thanks,
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sosnowski, Maciej
Acked-byRE: [PATCH v2 11/11] async_tx: raid6 recovery self test
Quoted 10 lines · Dan Williams
Port
drivers/md/raid6test/test.cto use the async raid6 recovery routines. This is meant as a unit test for raid6 acceleration drivers.Signed-off-by Dan Williams <[email protected]>---
crypto/async_tx/Makefile | 1
crypto/async_tx/raid6test.c | 212 +++++++++++++++++++++++++++++++++++++++++++drivers/md/Kconfig| 13 +++3 files changed, 226 insertions(+), 0 deletions(-)create mode 100644
crypto/async_tx/raid6test.c
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html