Re: [PATCH v2 08/11] async_tx: add support for asynchronous GF multiplication

[Andre Noll <maan@systemlinux.org>]

[-- Attachment #1: Type: text/plain, Size: 2496 bytes --]

On Mon, May 18, 2009 at 06:00:07PM -0700, Dan Williams wrote:
> +/* 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

> +	/* 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.

> +
> +	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;
> +			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.

> +/**
> + * 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.

> +	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
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]