Re: [PATCH v3 3/5] RDMA/core: add MR support for bvec-based RDMA operations

[Christoph Hellwig <hch@lst.de>]

> +/*
> + * Check if the device requires memory registration for RDMA READs.
> + * iWARP always requires MR for RDMA READ due to protocol limitations.
> + */
> +static inline bool rdma_rw_io_requires_mr(struct ib_device *dev, u32 port_num,

>  static inline bool rdma_rw_io_needs_mr(struct ib_device *dev, u32 port_num,
>  		enum dma_data_direction dir, int dma_nents)

I find the naming really confusing here.  I guess requires is that
the protocol (iWarp) doesn't work with it, needs means we need it for
the the number of entries.

And the new API requires the ULP to size the mapping request to never
hit the latter case?

Maybe just kill off the old rdma_rw_io_needs_mr and open code the
latter case in the only user?:w


>  	for (i = 0; i < ctx->nr_ops; i++) {
> -		struct rdma_rw_reg_ctx *reg = &ctx->reg[i];
> +		struct rdma_rw_reg_ctx *reg = &ctx->reg.ctx[i];

Jumping ahead here - why can't the sgtable be stored in ->reg
without renaming?  Is there case where need it, but the rest of
reg?   In 

> +	ctx->nr_ops = DIV_ROUND_UP(ctx->reg.sgt.nents, pages_per_mr);
> +	ctx->reg.ctx = kcalloc(ctx->nr_ops, sizeof(*ctx->reg.ctx), GFP_KERNEL);
> +	if (!ctx->reg.ctx) {
> +		ret = -ENOMEM;
> +		goto out_unmap_sgt;
> +	}
> +
> +	sg = ctx->reg.sgt.sgl;
> +	nents = ctx->reg.sgt.nents;
> +	for (i = 0; i < ctx->nr_ops; i++) {
> +		struct rdma_rw_reg_ctx *reg = &ctx->reg.ctx[i];
> +		u32 sge_cnt = min(nents, pages_per_mr);
> +
> +		ret = rdma_rw_init_one_mr(qp, port_num, reg, sg, sge_cnt, 0);

I guess you looked into that, but never replied, but this still
looks like it duplicates most of rdma_rw_init_mr_wrs.  Is there something
that prevents reusing that directly or with minor refactoring?

> +	memcpy(ctx->reg.ctx->mr->sig_attrs, sig_attrs, sizeof(struct ib_sig_attrs));

Overly long line.  But this also shows an issue that the details of the
rw context leak for the later added signature MR support :P