Re: [PATCH/RFC 2.6.21 2/5] ehca: ehca_uverbs.c: "proper" use of mmap

[Christoph Hellwig <hch@infradead.org>]

On Thu, Jan 11, 2007 at 08:08:15PM +0100, Hoang-Nam Nguyen wrote:
> +static void mm_open(struct vm_area_struct *vma)

This should be name ehca_vma_open, dito for mm_close/ehca_vma_close and
vm_ops/ehca_vm_ops.

> +	u32 *count = (u32*)vma->vm_private_data;

No need for the cast here (both in the open and close routine)

> +	for (ofs = 0; ofs < queue->queue_length; ofs += PAGE_SIZE) {
> +		u64 virt_addr = (u64)ipz_qeit_calc(queue, ofs);
> +		page = virt_to_page(virt_addr);
> +		rc = vm_insert_page(vma, start, page);
> +		if (unlikely(rc)) {
> +			ehca_gen_err("vm_insert_page() failed rc=%x", rc);
> +			return rc;
> +		}
> +		start +=  PAGE_SIZE;

Not required for now, but long term you really should rework your
whole queue abstraction to operate on an array of struct pages, that
makes things like this and various other bits in ipz_pt_fn.[ch] a lot
simpler.

>  int ehca_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
>  {

Can you split this monster routine into individual functions for
each type of mmap please?  With two helpers to get and verify the cq/qp
shared by the individual sub-variants, that would also help to get rid
of all those magic offsets.

Actually, this routine directly comes from ib_device.mmap - Roland,
can you shed some light on what's going on here?

Also after applying this patch I have a prototype and various callers
for ehca_mmap_nopage but no actual implementation.  Could it be that
there are some bits missing?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/