[ofa-general] [PATCH 2/3] RDMA/cxgb3: fix page shift calculation in build_phys_page_list()

[Bryan S Rosenburg]

Roland Dreier <rdreier at cisco.com> wrote on 01/30/2008 11:47:55 AM:
> 
>  > 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.
> 
> Makes sense... let me post a patch for discussion.

For what it's worth, I removed the single-buffer special case and replaced 
the whole second loop with an __ffs() call.  As expected, it uses a very 
large page size to cover even a small single buffer if the alignment 
constraints allow it, and it works.  Here's what I've been playing with:

================================================================================
diff --git a/drivers/infiniband/hw/mthca/mthca_provider.c 
b/drivers/infiniband/hw/mthca/mthca_provider.c
index 6bcde1c..b00ab71 100644
--- a/drivers/infiniband/hw/mthca/mthca_provider.c
+++ b/drivers/infiniband/hw/mthca/mthca_provider.c
@@ -929,11 +929,7 @@ static struct ib_mr *mthca_reg_phys_mr(struct ib_pd   
 *pd,
        int err;
        int i, j, n;
 
-       /* First check that we have enough alignment */
-       if ((*iova_start & ~PAGE_MASK) != (buffer_list[0].addr & 
~PAGE_MASK))
-               return ERR_PTR(-EINVAL);
-
-       mask = 0;
+       mask = buffer_list[0].addr ^ *iova_start;
        total_size = 0;
        for (i = 0; i < num_phys_buf; ++i) {
                if (i != 0)
@@ -948,16 +944,7 @@ static struct ib_mr *mthca_reg_phys_mr(struct ib_pd   
 *pd,
                return ERR_PTR(-EINVAL);
 
        /* Find largest page shift we can use to cover buffers */
-       for (shift = PAGE_SHIFT; shift < 31; ++shift)
-               if (num_phys_buf > 1) {
-                       if ((1ULL << shift) & mask)
-                               break;
-               } else {
-                       if (1ULL << shift >=
-                           buffer_list[0].size +
-                           (buffer_list[0].addr & ((1ULL << shift) - 1)))
-                               break;
-               }
+       shift = __ffs((unsigned long) ((mask & ((1ull<<31)-1)) | 
(1ull<<31)));
 
        buffer_list[0].size += buffer_list[0].addr & ((1ULL << shift) - 
1);
        buffer_list[0].addr &= ~0ull << shift;
================================================================================
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/general/attachments/20080130/4bce7634/attachment.html>