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

[Eli Cohen]

On Sun, Jun 22, 2008 at 12:57:26PM -0700, Roland Dreier wrote:
> 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.
Makes sense.

> 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.
Looking at the implementation of kmap_atomic() quoted bellow:

void *page_address(struct page *page)
{
        unsigned long flags;
        void *ret;
        struct page_address_slot *pas;

        if (!PageHighMem(page))
                return lowmem_page_address(page);

        pas = page_slot(page);
        ret = NULL;
        spin_lock_irqsave(&pas->lock, flags);
        if (!list_empty(&pas->lh)) {
                struct page_address_map *pam;

                list_for_each_entry(pam, &pas->lh, list) {
                        if (pam->page == page) {
                                ret = pam->virtual;
                                goto done;
                        }
                }
        }
done:
        spin_unlock_irqrestore(&pas->lock, flags);
        return ret;
}

static inline void *kmap_atomic(struct page *page, enum km_type idx)
{
        pagefault_disable();
        return page_address(page);
}

I can't see page_address doing anything actively to map the page. It
just searches for the page and returns it's mapping. But I'm probably
missing something.


>  As far as I can
> see, __GFP_ZERO allocations use clear_highpage() internally anyway, so
> it ends up being the same thing.
Yes, now I can see that.

> 
> 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;
>  

Of course. Go ahead and send it.
Thanks.