Re: [PATCH for-next v2 06/18] RDMA/rxe: Add rxe_add_frag() to rxe_mr.c

[Bob Pearson <rpearsonhpe@gmail.com>]

On 11/24/22 13:10, Jason Gunthorpe wrote:
> On Mon, Oct 31, 2022 at 03:27:55PM -0500, Bob Pearson wrote:
>> +int rxe_add_frag(struct sk_buff *skb, struct rxe_phys_buf *buf,
>> +		 int length, int offset)
>> +{
>> +	int nr_frags = skb_shinfo(skb)->nr_frags;
>> +	skb_frag_t *frag = &skb_shinfo(skb)->frags[nr_frags];
>> +
>> +	if (nr_frags >= MAX_SKB_FRAGS) {
>> +		pr_debug("%s: nr_frags (%d) >= MAX_SKB_FRAGS\n",
>> +			__func__, nr_frags);
>> +		return -EINVAL;
>> +	}
>> +
>> +	frag->bv_len = length;
>> +	frag->bv_offset = offset;
>> +	frag->bv_page = virt_to_page(buf->addr);
> 
> Assuming this is even OK to do, then please do the xarray conversion
> I sketched first:
> 
> https://lore.kernel.org/linux-rdma/Y3gvZr6%2FNCii9Avy@nvidia.com/

I've been looking at this. Seems incorrect for IB_MR_TYPE_DMA which
do not carry a page map but simply convert iova to kernel virtual addresses.
This breaks in the mr_copy routine and atomic ops in responder.
There is also a missing rxe_mr_iova_to_index() function. Looks simple enough
just the number of pages starting from 0.

I am curious what the benefit of the 'advanced' API for xarrays buys here. Is it just
preallocating all the memory before it gets used?

I am happy to go in this direction and if we do it should be ahead of the other
outstanding changes that touch MRs.

I will try to submit a patch to do that.

Bob

> 
> And this operation is basically a xa_for_each loop taking 'struct page
> *' off of the MR's xarray, slicing it, then stuffing into the
> skb. Don't call virt_to_page()
> 
> *However* I have no idea if it is even safe to stuff unstable pages
> into a skb. Are there other examples doing this? Eg zero copy tcp?
> 
> Jason