diff mbox

[v2,00/12] IB: Replace safe uses for ib_get_dma_mr with pd->local_dma_lkey

Message ID 55C2912A.50709@sandisk.com (mailing list archive)
State Superseded
Headers show

Commit Message

Bart Van Assche Aug. 5, 2015, 10:41 p.m. UTC 
On 08/05/2015 02:45 PM, Bart Van Assche wrote:
> On 08/05/2015 12:51 PM, Jason Gunthorpe wrote:
>> On Tue, Aug 04, 2015 at 11:41:16PM -0700, David Dillow wrote:
>>> On Tue, 2015-08-04 at 12:09 -0600, Jason Gunthorpe wrote:
>>>> On Mon, Aug 03, 2015 at 11:33:51AM -0700, Bart Van Assche wrote:
>>>>>> Bart, do you know what hardware this workaround is for?
>>>>>
>>>>> I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA
>>>>> models and/or firmware versions do not support FMR mapping with a non-zero
>>>>> offset.
>>>>
>>>> Perhaps David can remember why he added this:
>>>>
>>>> commit 8f26c9ff9cd0317ad867bce972f69e0c6c2cbe3c
>>>
>>> It's originally from commit 559ce8f150d7d031c79c4d79173860f1bdfe3ce4,
>>> and the list's attempts at code archaeology failed us:
>>> http://article.gmane.org/gmane.linux.drivers.rdma/7149
>>
>> Okay.. So over time we went from a clear target specific bug described
>> 9 years ago in 559ce through chinese whispers to a general unspecific
>> fear of non-zero offset FMR?
>>
>> But nobody has described FMR failure in this way in the past 9 years
>> with any specificity?
>>
>> My random guesses would be broken mthca firmware at the start of the
>> FMR feature (long since fixed) or a wonky target that is now 10 years
>> obsolete..
>>
>> If it was an HCA bug, I strongly have to think it is fixed now. We use
>> FMR all over the place and SRP is the only area I've noticed this
>> restriction..
>>
>> If it is a target bug, then FRWR should trigger it as wel, so we are
>> already about to revert that workaround.
>>
>> I'm inclined to drop this entirely.. What do you think Bart?
>
> Even today I see memory corruption at the initiator side with FMR and
> not with FR if I leave out the alignment check. Since this only occurs
> with FMR and not with FR that excludes the target side as a possible
> cause. I will have a closer look at the srp_map_finish_fmr() function.
> Always passing 0 as the second argument of srp_map_desc() in
> srp_map_finish_fmr() is probably wrong. Before 2011 that second argument
> was the page offset of the first sg-list entry.

(replying to my own e-mail)

The patch below makes FMR registration work again for buffers that are 
not aligned on a page boundary.

Regarding the discussion in 2011 about FMR 
(http://thread.gmane.org/gmane.linux.drivers.rdma/7149): since in 2011 
nobody recalled the root cause of the issue with non-page aligned FMR my 
proposal is to drop the page alignment check and if any issues occur to 
introduce a blacklist for the SRP target devices that have trouble with 
this.

Bart.


[PATCH] IB/srp: Restore FMR offset

Since srp_map_finish_fmr() is always called with base_dma_addr aligned
on a page boundary this patch does not change any functionality. See
also patch "IB/srp: rework mapping engine to use multiple FMR entries"
(commit ID 8f26c9ff9cd0; January 2011).
---
  drivers/infiniband/ulp/srp/ib_srp.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)


@@ -1283,7 +1285,8 @@ static int srp_map_finish_fmr(struct srp_map_state 
*state,
  	*state->next_fmr++ = fmr;
  	state->nmdesc++;

-	srp_map_desc(state, 0, state->dma_len, fmr->fmr->rkey);
+	srp_map_desc(state, state->base_dma_addr & ~dev->mr_page_mask,
+		     state->dma_len, fmr->fmr->rkey);

  	return 0;
  }

Comments

Jason Gunthorpe Aug. 6, 2015, 12:10 a.m. UTC | #1 
On Wed, Aug 05, 2015 at 03:41:46PM -0700, Bart Van Assche wrote:

> Regarding the discussion in 2011 about FMR
> (http://thread.gmane.org/gmane.linux.drivers.rdma/7149): since in 2011
> nobody recalled the root cause of the issue with non-page aligned FMR my
> proposal is to drop the page alignment check and if any issues occur to
> introduce a blacklist for the SRP target devices that have trouble with
> this.

Nice find, that sounds like a solid plan.

That looks like it leaves only the srp_map_data flow and the error case
flow in srp_map_sg as remaining problems for the rkey patch? Any
thoughts on adressing those?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Aug. 6, 2015, 12:19 a.m. UTC | #2 
On 08/05/2015 05:10 PM, Jason Gunthorpe wrote:
> On Wed, Aug 05, 2015 at 03:41:46PM -0700, Bart Van Assche wrote:
>> Regarding the discussion in 2011 about FMR
>> (http://thread.gmane.org/gmane.linux.drivers.rdma/7149): since in 2011
>> nobody recalled the root cause of the issue with non-page aligned FMR my
>> proposal is to drop the page alignment check and if any issues occur to
>> introduce a blacklist for the SRP target devices that have trouble with
>> this.
>
> Nice find, that sounds like a solid plan.
>
> That looks like it leaves only the srp_map_data flow and the error case
> flow in srp_map_sg as remaining problems for the rkey patch? Any
> thoughts on adressing those?

Hello Jason,

A few experimental and untested patches are available for review at 
https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Aug. 6, 2015, 4:36 a.m. UTC | #3 
On Wed, Aug 05, 2015 at 05:19:10PM -0700, Bart Van Assche wrote:

> A few experimental and untested patches are available for review at
> https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.

I'll leave this is in your hands then..

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Aug. 6, 2015, 3:09 p.m. UTC | #4 
On 08/05/2015 09:36 PM, Jason Gunthorpe wrote:
> On Wed, Aug 05, 2015 at 05:19:10PM -0700, Bart Van Assche wrote:
>> A few experimental and untested patches are available for review at
>> https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.
>
> I'll leave this is in your hands then..

Is there perhaps a tree available somewhere with the latest version of 
your patch series ?

Thanks,

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Aug. 11, 2015, 12:05 a.m. UTC | #5 
On 08/05/2015 09:36 PM, Jason Gunthorpe wrote:
> On Wed, Aug 05, 2015 at 05:19:10PM -0700, Bart Van Assche wrote:
>
>> A few experimental and untested patches are available for review at
>> https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.
>
> I'll leave this is in your hands then..

In case anyone would like to see what I am working on, I will post the 
patches I am currently testing in reply to this e-mail. The names of 
these patches are as follows:

0001-IB-srp-Re-enable-FMR-for-non-page-aligned-buffers.patch
0002-IB-srp-Use-multiple-registrations-for-large-memory-r.patch
0003-IB-srp-Add-memory-descriptor-array-pointer-range-che.patch
0004-IB-srp-Remove-the-memory-registration-backtracking-c.patch
0005-IB-srp-Remove-use_mr-argument-from-srp_map_sg_entry.patch
0006-IB-srp-Introduce-srp_device.use_fmr.patch
0007-IB-srp-Register-the-indirect-data-buffer-descriptor.patch
0008-IB-srp-Create-an-insecure-all-physical-rkey-only-if-.patch

These patches are also available at 
https://github.com/bvanassche/linux/tree/srp-initiator-for-next.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Aug. 11, 2015, 5:40 a.m. UTC | #6 
On Mon, Aug 10, 2015 at 05:05:53PM -0700, Bart Van Assche wrote:
> On 08/05/2015 09:36 PM, Jason Gunthorpe wrote:
> >On Wed, Aug 05, 2015 at 05:19:10PM -0700, Bart Van Assche wrote:
> >
> >>A few experimental and untested patches are available for review at
> >>https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.
> >
> >I'll leave this is in your hands then..
> 
> In case anyone would like to see what I am working on, I will post the
> patches I am currently testing in reply to this e-mail. The names of these
> patches are as follows:

Nice, looks like it covers all the cases..

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index 48201b3..cac444e 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1272,6 +1272,8 @@  static void srp_map_desc(struct srp_map_state 
*state, dma_addr_t dma_addr,
  static int srp_map_finish_fmr(struct srp_map_state *state,
  			      struct srp_rdma_ch *ch)
  {
+	struct srp_target_port *target = ch->target;
+	struct srp_device *dev = target->srp_host->srp_dev;
  	struct ib_pool_fmr *fmr;
  	u64 io_addr = 0;