<br><tt><font size=2>Roland Dreier <rdreier@cisco.com> wrote on 01/27/2008
04:33:33 PM:<br>
> got it... I was tricking myself that the check for alignment at the<br>
> start of the function was sufficient, but it's not once we start using<br>
> a bigger value of shift.<br>
> <br>
> I think the patch below should be a fix for the problem, although
I've<br>
> only compile tested it. The idea is to stop increasing shift
once it<br>
> reaches a bit position where the first buffer and the iova differ.<br>
> What do you think? If this works for you, I will merge it for
2.6.25.<br>
</font></tt>
<br><tt><font size=2>I've tested your fix with a two-element buffer list
and various combinations of virtual and physical alignments. It works
as intended, allowing a larger page size (and corresponding expansion of
the physical region) if and only if the virtual and physical addresses
are sufficiently compatible.</font></tt>
<br>
<br><tt><font size=2>I think you can simplify the code somewhat if you
incorporate the (virtual ^ physical) alignment characterization into the
mask. Here's an alternative patch. The initial alignment check
gets subsumed into the later mask alignment check.</font></tt>
<br>
<br><tt><font size=2>================================================================================</font></tt>
<div>
<br><tt><font size=2>diff --git a/drivers/infiniband/hw/mthca/mthca_provider.c
b/drivers/infiniband/hw/mthca/mthca_provider.c<br>
index 6bcde1c..4e04b4f 100644<br>
--- a/drivers/infiniband/hw/mthca/mthca_provider.c<br>
+++ b/drivers/infiniband/hw/mthca/mthca_provider.c<br>
@@ -929,11 +929,7 @@ static struct ib_mr *mthca_reg_phys_mr(struct ib_pd
*pd,<br>
int err;<br>
int i, j, n;<br>
<br>
- /* First check that we have enough
alignment */<br>
- if ((*iova_start & ~PAGE_MASK)
!= (buffer_list[0].addr & ~PAGE_MASK))<br>
- return
ERR_PTR(-EINVAL);<br>
-<br>
- mask = 0;<br>
+ mask = buffer_list[0].addr ^ *iova_start;<br>
total_size = 0;<br>
for (i = 0; i < num_phys_buf; ++i)
{<br>
if
(i != 0)<br>
@@ -948,16 +944,16 @@ static struct ib_mr *mthca_reg_phys_mr(struct ib_pd
*pd,<br>
return
ERR_PTR(-EINVAL);<br>
<br>
/* Find largest page shift we can
use to cover buffers */<br>
- for (shift = PAGE_SHIFT; shift <
31; ++shift)<br>
- if
(num_phys_buf > 1) {<br>
-
if ((1ULL << shift) & mask)<br>
-
break;<br>
- }
else {<br>
+ for (shift = PAGE_SHIFT; shift <
31; ++shift) {<br>
+ if
((1ULL << shift) & mask)<br>
+
break;</font></tt>
<br><tt><font size=2>+
if (num_phys_buf == 1) {<br>
if (1ULL << shift >=<br>
buffer_list[0].size +<br>
(buffer_list[0].addr & ((1ULL
<< shift) - 1)))<br>
break;<br>
}<br>
+ }<br>
<br>
buffer_list[0].size += buffer_list[0].addr
& ((1ULL << shift) - 1);<br>
buffer_list[0].addr &= ~0ull <<
shift;</font></tt>
<br><tt><font size=2>================================================================================</font></tt>
<br>
<br><tt><font size=2>I'm still annoyed by the (num_phys_buf == 1) special
case. I'm wondering if it's still needed. If you leave out
that if-statement entirely, you may end up using a page size that is larger
(maybe much larger) than necessary, but I think things will still work,
given that the virtual-to-physical alignment constraints are respected.
If you remove the special case, you can replace the whole loop with
an ffs() call.</font></tt>
<br>
<br><tt><font size=2>Anyway, your patch works fine. Use my suggestion
only if you like it.</font></tt>
<br>
<br><tt><font size=2>- Bryan</font></tt>
<br></div>