[ofa-general] Re: [PATCH] IB/mthca: Clear ICM pages before handing to FW

[Roland Dreier]

 > I was thinking about applications which create and destroy HCA
 > resources at a high rate which might be affected.

I would expect all the other costs of mapping pages into/out of the ICM
to be large enough that the cost of zeroing a page is not too big a
deal.  But if there is anyone who cares we can always make this
__GFP_ZERO flag conditional on FW version I guess.

 > Looks to me like using __GFP_ZERO is the cleanest approach. I like
 > less clear_highpage()  since it uses kmap_atomic() which could fail.

To the best of my knowledge, kmap_atomic() cannot fail -- and if you
think about it, what could make it fail?  The whole point of
kmap_atomic() is that there is a per-cpu pre-reserved slot to map the
memory at for highmem pages, so it has to always work.  As far as I can
see, __GFP_ZERO allocations use clear_highpage() internally anyway, so
it ends up being the same thing.

Since just adding __GFP_ZERO is so much simpler, I'll just commit the
patch below and send it to Linus in a day or two if it seems OK to you:

commit 801d1ad7b6bdb3418a462c6b4950aee56dbac940
Author: Eli Cohen <eli at mellanox.co.il>
Date:   Sun Jun 22 12:56:58 2008 -0700

    IB/mthca: Clear ICM pages before handing to FW
    
    Current memfree FW has a bug which in some cases, assumes that ICM
    pages passed to it are cleared.  This patch uses __GFP_ZERO to
    allocate all ICM pages passed to the FW.  Once firmware with a fix is
    released, we can make the workaround conditional on firmware version.
    
    This fixes the bug reported by Arthur Kepner <akepner at sgi.com> here:
    http://lists.openfabrics.org/pipermail/general/2008-May/050026.html
    
    Signed-off-by: Eli Cohen <eli at mellanox.co.il>
    
    [ Rewritten to be a one-liner using __GFP_ZERO instead of vmap()ing
      ICM memory and memset()ing it to 0. - Roland ]
    
    Signed-off-by: Roland Dreier <rolandd at cisco.com>

diff --git a/drivers/infiniband/hw/mthca/mthca_memfree.c b/drivers/infiniband/hw/mthca/mthca_memfree.c
index 9e77ba9..1f7d1a2 100644
--- a/drivers/infiniband/hw/mthca/mthca_memfree.c
+++ b/drivers/infiniband/hw/mthca/mthca_memfree.c
@@ -107,7 +107,11 @@ static int mthca_alloc_icm_pages(struct scatterlist *mem, int order, gfp_t gfp_m
 {
 	struct page *page;
 
-	page = alloc_pages(gfp_mask, order);
+	/*
+	 * Use __GFP_ZERO because buggy firmware assumes ICM pages are
+	 * cleared, and subtle failures are seen if they aren't.
+	 */
+	page = alloc_pages(gfp_mask | __GFP_ZERO, order);
 	if (!page)
 		return -ENOMEM;