LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Hoang-Nam Nguyen <hnguyen@linux.vnet.ibm.com>
Cc: Roland Dreier <rdreier@cisco.com>,
	hch@infradead.org, linux-kernel@vger.kernel.org,
	linuxppc-dev@ozlabs.org, openfabrics-ewg@openib.org,
	openib-general@openib.org, raisch@de.ibm.com
Subject: Re: [PATCH/RFC 2.6.21 2/5] ehca: ehca_uverbs.c: "proper" use of mmap
Date: Thu, 11 Jan 2007 19:40:00 +0000	[thread overview]
Message-ID: <20070111194000.GE24623@infradead.org> (raw)
In-Reply-To: <200701112008.15841.hnguyen@linux.vnet.ibm.com>

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?

  reply	other threads:[~2007-01-11 19:40 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-01-11 19:08 Hoang-Nam Nguyen
2007-01-11 19:40 ` Christoph Hellwig [this message]
2007-01-11 19:54   ` Roland Dreier
2007-01-12 12:25     ` Christoph Raisch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070111194000.GE24623@infradead.org \
    --to=hch@infradead.org \
    --cc=hnguyen@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=openfabrics-ewg@openib.org \
    --cc=openib-general@openib.org \
    --cc=raisch@de.ibm.com \
    --cc=rdreier@cisco.com \
    --subject='Re: [PATCH/RFC 2.6.21 2/5] ehca: ehca_uverbs.c: "proper" use of mmap' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).