> On Feb 11, 2020, at 11:36 AM, Robin Murphy <robin.murphy@xxxxxxx> wrote:
>
> On 11/02/2020 4:03 pm, Chuck Lever wrote:
>>> On Feb 11, 2020, at 10:32 AM, Robin Murphy <robin.murphy@xxxxxxx> wrote:
>>>
>>> On 11/02/2020 3:24 pm, Chuck Lever wrote:
>>>>> On Feb 11, 2020, at 10:12 AM, Robin Murphy <robin.murphy@xxxxxxx> wrote:
>>>>>
>>>>> On 11/02/2020 1:48 pm, Chuck Lever wrote:
>>>>>> Andre-
>>>>>> Thank you for the detailed report!
>>>>>> Tom-
>>>>>> There is a rich set of trace points available in the RPC/RDMA implementation in 5.4/5.5, fwiw.
>>>>>> Please keep me in the loop, let me know if there is anything I can do to help.
>>>>>
>>>>> One aspect that may be worth checking is whether there's anywhere that assumes a successful return value from dma_map_sg() is always the same as the number of entries passed in - that's the most obvious way the iommu-dma code differs (legitimately) from the previous amd-iommu implementation.
>>>> net/sunrpc/xprtrdma/frwr_ops.c: frwr_map()
>>>> 317 mr->mr_nents =
>>>> 318 ib_dma_map_sg(ia->ri_id->device, mr->mr_sg, i, mr->mr_dir);
>>>> 319 if (!mr->mr_nents)
>>>> 320 goto out_dmamap_err;
>>>> Should that rather be "if (mr->mr_nents != i)" ?
>>>
>>> No, that much is OK - the point is that dma_map_sg() may pack the DMA addresses such that sg_dma_len(sg) > sg->length - however, subsequently passing that mr->nents to dma_unmap_sg() in frwr_mr_recycle() (rather than the original value of i) looks at a glance like an example of how things may start to get out-of-whack.
>> Robin, your explanation makes sense to me. I can post a fix for this imbalance later today for Andre to try.
>
> FWIW here's a quick hack which *should* suppress the concatenation behaviour - if it makes Andre's system any happier then that would indeed point towards dma_map_sg() handling being the culprit.
Even so, 1f541895dae9 ("xprtrdma: Don't defer MR recovery if ro_map fails")
looks like it introduced this problem.
> Robin.
>
> ----->8-----
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index a2e96a5fd9a7..a6b71bad518e 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -779,7 +779,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents,
> * - but doesn't fall at a segment boundary
> * - and wouldn't make the resulting output segment too long
> */
> - if (cur_len && !s_iova_off && (dma_addr & seg_mask) &&
> + if (0 && cur_len && !s_iova_off && (dma_addr & seg_mask) &&
> (max_len - cur_len >= s_length)) {
> /* ...then concatenate it with the previous one */
> cur_len += s_length;
> @@ -799,6 +799,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents,
> if (s_length + s_iova_off < s_iova_len)
> cur_len = 0;
> }
> + WARN_ON(count < nents);
> return count;
> }
--
Chuck Lever
