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

[Bob Pearson <rpearsonhpe@gmail.com>]

On 11/30/22 18:20, Jason Gunthorpe wrote:
> On Wed, Nov 30, 2022 at 06:16:53PM -0600, Bob Pearson wrote:
>> On 11/30/22 17:36, Jason Gunthorpe wrote:
>>> On Wed, Nov 30, 2022 at 02:53:22PM -0600, Bob Pearson wrote:
>>>> 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.
>>>
>>> There is always a struct page involved, even in the kernel case. You
>>> can do virt_to_page on kernel addresses
> 
>> Agreed but there isn't a page map set up for DMA mr's. You just get the whole kernel
>> address space. So the call to rxe_mr_copy_xarray() won't work. There isn't an
>> xarray to copy to/from. Much easier to just leave the DMA mr code in place since
>> it does what we want very simply. Also you have to treat the DMA mr separately for
>> atomic ops.
> 
> You mean the all physical memory MR type? It is true, but you still
> have to add the kmap and so on. It should be a similar function that
> assumes the IOVA is a physical address is a kernel mapped address and
> does virt_to_page/etc instead of the xarray loop.
> 
> Jason

I'm not looking at my patch you responded to but the one you posted to replace maps
by xarrays. The existing rxe driver assumes that if ibmr->type == IB_MR_TYPE_DMA that
the iova is just a kernel (virtual) address that is already mapped. Maybe this is
not correct but it has always worked this way. These are heavily used by storage stacks
(e.g. Lustre) which always use DMA mr's. Since we don't actually do any DMAs we don't
need to setup the iommu for these and just do memcpy's without dealing with pages.

Bob