On 26 May 2016 at 22:04, Mike Snitzer <snitzer@xxxxxxxxxx> wrote: > Comments inlined. > > In general the most concerning bit is the need for memory allocation in > the IO path (see comment/question below near call to sg_alloc_table). > In DM targets we make heavy use of .ctr preallocated memory and/or > per-bio-data to avoid memory allocations in the IO path. Make sense. > > On Wed, May 25 2016 at 2:12am -0400, > Baolin Wang <baolin.wang@xxxxxxxxxx> wrote: > >> In now dm-crypt code, it is ineffective to map one segment (always one >> sector) of one bio with just only one scatterlist at one time for hardware >> crypto engine. Especially for some encryption mode (like ecb or xts mode) >> cooperating with the crypto engine, they just need one initial IV or null >> IV instead of different IV for each sector. In this situation We can consider >> to use multiple scatterlists to map the whole bio and send all scatterlists >> of one bio to crypto engine to encrypt or decrypt, which can improve the >> hardware engine's efficiency. >> >> With this optimization, On my test setup (beaglebone black board) using 64KB >> I/Os on an eMMC storage device I saw about 60% improvement in throughput for >> encrypted writes, and about 100% improvement for encrypted reads. But this >> is not fit for other modes which need different IV for each sector. >> >> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx> >> --- >> drivers/md/dm-crypt.c | 188 +++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 176 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c >> index 4f3cb35..1c86ea7 100644 >> --- a/drivers/md/dm-crypt.c >> +++ b/drivers/md/dm-crypt.c >> @@ -33,6 +33,7 @@ >> #include <linux/device-mapper.h> >> >> #define DM_MSG_PREFIX "crypt" >> +#define DM_MAX_SG_LIST 1024 >> >> /* >> * context holding the current state of a multi-part conversion >> @@ -46,6 +47,8 @@ struct convert_context { >> sector_t cc_sector; >> atomic_t cc_pending; >> struct skcipher_request *req; >> + struct sg_table sgt_in; >> + struct sg_table sgt_out; >> }; >> >> /* >> @@ -803,6 +806,108 @@ static struct crypt_iv_operations crypt_iv_tcw_ops = { >> .post = crypt_iv_tcw_post >> }; >> >> +/* >> + * Check how many sg entry numbers are needed when map one bio >> + * with scatterlists in advance. >> + */ >> +static unsigned int crypt_sg_entry(struct bio *bio_t) >> +{ >> + struct request_queue *q = bdev_get_queue(bio_t->bi_bdev); >> + int cluster = blk_queue_cluster(q); >> + struct bio_vec bvec, bvprv = { NULL }; >> + struct bvec_iter biter; >> + unsigned long nbytes = 0, sg_length = 0; >> + unsigned int sg_cnt = 0, first_bvec = 0; >> + >> + if (bio_t->bi_rw & REQ_DISCARD) { >> + if (bio_t->bi_vcnt) >> + return 1; >> + return 0; >> + } >> + >> + if (bio_t->bi_rw & REQ_WRITE_SAME) >> + return 1; >> + >> + bio_for_each_segment(bvec, bio_t, biter) { >> + nbytes = bvec.bv_len; >> + >> + if (!cluster) { >> + sg_cnt++; >> + continue; >> + } >> + >> + if (!first_bvec) { >> + first_bvec = 1; >> + goto new_segment; >> + } >> + >> + if (sg_length + nbytes > queue_max_segment_size(q)) >> + goto new_segment; >> + >> + if (!BIOVEC_PHYS_MERGEABLE(&bvprv, &bvec)) >> + goto new_segment; >> + >> + if (!BIOVEC_SEG_BOUNDARY(q, &bvprv, &bvec)) >> + goto new_segment; >> + >> + sg_length += nbytes; >> + continue; >> + >> +new_segment: >> + memcpy(&bvprv, &bvec, sizeof(struct bio_vec)); >> + sg_length = nbytes; >> + sg_cnt++; >> + } >> + >> + return sg_cnt; >> +} >> + >> +static int crypt_convert_alloc_table(struct crypt_config *cc, >> + struct convert_context *ctx) >> +{ >> + struct bio *bio_in = ctx->bio_in; >> + struct bio *bio_out = ctx->bio_out; >> + unsigned int mode = skcipher_is_bulk_mode(any_tfm(cc)); > > please use: bool bulk_mode = ... OK. > >> + unsigned int sg_in_max, sg_out_max; >> + int ret = 0; >> + >> + if (!mode) >> + goto out2; > > Please use more descriptive label names than out[1-3] OK. > >> + >> + /* >> + * Need to calculate how many sg entry need to be used >> + * for this bio. >> + */ >> + sg_in_max = crypt_sg_entry(bio_in) + 1; > > The return from crypt_sg_entry() is pretty awkward, given you just go on > to add 1; as is the bounds checking.. the magic value of 2 needs to be > be made clearer. I'll remove the crypt_sg_entry() function. > >> + if (sg_in_max > DM_MAX_SG_LIST || sg_in_max <= 2) >> + goto out2; >> + >> + ret = sg_alloc_table(&ctx->sgt_in, sg_in_max, GFP_KERNEL); > > Is it safe to be using GFP_KERNEL here? AFAIK this is in the IO mapping > path and we try to avoid memory allocations at all costs -- due to the > risk of deadlock when issuing IO to stacked block devices (dm-crypt > could be part of a much more elaborate IO stack). OK. I'll move the sg table allocation to be preallocated in the .ctr function. > >> + if (ret) >> + goto out2; >> + >> + if (bio_data_dir(bio_in) == READ) >> + goto out1; >> + >> + sg_out_max = crypt_sg_entry(bio_out) + 1; >> + if (sg_out_max > DM_MAX_SG_LIST || sg_out_max <= 2) >> + goto out3; >> + >> + ret = sg_alloc_table(&ctx->sgt_out, sg_out_max, GFP_KERNEL); >> + if (ret) >> + goto out3; >> + >> + return 0; >> + >> +out3: > > out_free_table? > >> + sg_free_table(&ctx->sgt_in); >> +out2: > > out_skip_alloc? > >> + ctx->sgt_in.orig_nents = 0; >> +out1: > > out_skip_write? > >> + ctx->sgt_out.orig_nents = 0; >> + return ret; >> +} >> + >> static void crypt_convert_init(struct crypt_config *cc, >> struct convert_context *ctx, >> struct bio *bio_out, struct bio *bio_in, >> @@ -843,7 +948,13 @@ static int crypt_convert_block(struct crypt_config *cc, >> { >> struct bio_vec bv_in = bio_iter_iovec(ctx->bio_in, ctx->iter_in); >> struct bio_vec bv_out = bio_iter_iovec(ctx->bio_out, ctx->iter_out); >> + unsigned int mode = skcipher_is_bulk_mode(any_tfm(cc)); > > again please use: bool bulk_mode = ... OK. > >> + struct bio *bio_in = ctx->bio_in; >> + struct bio *bio_out = ctx->bio_out; >> + unsigned int total_bytes = bio_in->bi_iter.bi_size; >> struct dm_crypt_request *dmreq; >> + struct scatterlist *sg_in; >> + struct scatterlist *sg_out; >> u8 *iv; >> int r; >> >> @@ -852,16 +963,6 @@ static int crypt_convert_block(struct crypt_config *cc, >> >> dmreq->iv_sector = ctx->cc_sector; >> dmreq->ctx = ctx; >> - sg_init_table(&dmreq->sg_in, 1); >> - sg_set_page(&dmreq->sg_in, bv_in.bv_page, 1 << SECTOR_SHIFT, >> - bv_in.bv_offset); >> - >> - sg_init_table(&dmreq->sg_out, 1); >> - sg_set_page(&dmreq->sg_out, bv_out.bv_page, 1 << SECTOR_SHIFT, >> - bv_out.bv_offset); >> - >> - bio_advance_iter(ctx->bio_in, &ctx->iter_in, 1 << SECTOR_SHIFT); >> - bio_advance_iter(ctx->bio_out, &ctx->iter_out, 1 << SECTOR_SHIFT); >> >> if (cc->iv_gen_ops) { >> r = cc->iv_gen_ops->generator(cc, iv, dmreq); >> @@ -869,8 +970,63 @@ static int crypt_convert_block(struct crypt_config *cc, >> return r; >> } >> >> - skcipher_request_set_crypt(req, &dmreq->sg_in, &dmreq->sg_out, >> - 1 << SECTOR_SHIFT, iv); >> + if (mode && ctx->sgt_in.orig_nents > 0) { >> + struct scatterlist *sg = NULL; >> + unsigned int total_sg_in, total_sg_out; >> + >> + total_sg_in = blk_bio_map_sg(bdev_get_queue(bio_in->bi_bdev), >> + bio_in, ctx->sgt_in.sgl, &sg); >> + if ((total_sg_in <= 0) || >> + (total_sg_in > ctx->sgt_in.orig_nents)) { >> + DMERR("%s in sg map error %d, sg table nents[%d]\n", >> + __func__, total_sg_in, ctx->sgt_in.orig_nents); >> + return -EINVAL; >> + } >> + >> + if (sg) >> + sg_mark_end(sg); >> + >> + ctx->iter_in.bi_size -= total_bytes; >> + sg_in = ctx->sgt_in.sgl; >> + sg_out = ctx->sgt_in.sgl; >> + >> + if (bio_data_dir(bio_in) == READ) >> + goto set_crypt; >> + >> + sg = NULL; >> + total_sg_out = blk_bio_map_sg(bdev_get_queue(bio_out->bi_bdev), >> + bio_out, ctx->sgt_out.sgl, &sg); >> + if ((total_sg_out <= 0) || >> + (total_sg_out > ctx->sgt_out.orig_nents)) { >> + DMERR("%s out sg map error %d, sg table nents[%d]\n", >> + __func__, total_sg_out, ctx->sgt_out.orig_nents); >> + return -EINVAL; >> + } >> + >> + if (sg) >> + sg_mark_end(sg); >> + >> + ctx->iter_out.bi_size -= total_bytes; >> + sg_out = ctx->sgt_out.sgl; >> + } else { >> + sg_init_table(&dmreq->sg_in, 1); >> + sg_set_page(&dmreq->sg_in, bv_in.bv_page, 1 << SECTOR_SHIFT, >> + bv_in.bv_offset); >> + >> + sg_init_table(&dmreq->sg_out, 1); >> + sg_set_page(&dmreq->sg_out, bv_out.bv_page, 1 << SECTOR_SHIFT, >> + bv_out.bv_offset); >> + >> + bio_advance_iter(ctx->bio_in, &ctx->iter_in, 1 << SECTOR_SHIFT); >> + bio_advance_iter(ctx->bio_out, &ctx->iter_out, 1 << SECTOR_SHIFT); >> + >> + sg_in = &dmreq->sg_in; >> + sg_out = &dmreq->sg_out; >> + total_bytes = 1 << SECTOR_SHIFT; >> + } >> + >> +set_crypt: >> + skcipher_request_set_crypt(req, sg_in, sg_out, total_bytes, iv); > > Given how long this code has gotten I'd prefer to see this factored out > to a new setup method. I'll refactor this long function. Thanks for your comments. > >> if (bio_data_dir(ctx->bio_in) == WRITE) >> r = crypto_skcipher_encrypt(req); >> @@ -1081,6 +1237,8 @@ static void crypt_dec_pending(struct dm_crypt_io *io) >> if (io->ctx.req) >> crypt_free_req(cc, io->ctx.req, base_bio); >> >> + sg_free_table(&io->ctx.sgt_in); >> + sg_free_table(&io->ctx.sgt_out); >> base_bio->bi_error = error; >> bio_endio(base_bio); >> } >> @@ -1312,6 +1470,9 @@ static void kcryptd_crypt_write_convert(struct dm_crypt_io *io) >> io->ctx.iter_out = clone->bi_iter; >> >> sector += bio_sectors(clone); >> + r = crypt_convert_alloc_table(cc, &io->ctx); >> + if (r < 0) >> + io->error = -EIO; >> >> crypt_inc_pending(io); >> r = crypt_convert(cc, &io->ctx); >> @@ -1343,6 +1504,9 @@ static void kcryptd_crypt_read_convert(struct dm_crypt_io *io) >> >> crypt_convert_init(cc, &io->ctx, io->base_bio, io->base_bio, >> io->sector); >> + r = crypt_convert_alloc_table(cc, &io->ctx); >> + if (r < 0) >> + io->error = -EIO; >> >> r = crypt_convert(cc, &io->ctx); >> if (r < 0) >> -- >> 1.7.9.5 >> >> -- >> dm-devel mailing list >> dm-devel@xxxxxxxxxx >> https://www.redhat.com/mailman/listinfo/dm-devel -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html