Re: [3/3 PATCH] Kprobes: User space probes support- single stepping out-of-line

[Nick Piggin <nickpiggin@yahoo.com.au>]

Andrew Morton wrote:
> Prasanna S Panchamukhi <prasanna@in.ibm.com> wrote:

> > +
> > +/**
> > + * This routines get the pte of the page containing the specified address.
> > + */
> > +static pte_t  __kprobes *get_uprobe_pte(unsigned long address)
> > +{
> > +	pgd_t *pgd;
> > +	pud_t *pud;
> > +	pmd_t *pmd;
> > +	pte_t *pte = NULL;
> > +
> > +	pgd = pgd_offset(current->mm, address);
> > +	if (!pgd)
> > +		goto out;
> > +
> > +	pud = pud_offset(pgd, address);
> > +	if (!pud)
> > +		goto out;
> > +
> > +	pmd = pmd_offset(pud, address);
> > +	if (!pmd)
> > +		goto out;
> > +
> > +	pte = pte_alloc_map(current->mm, pmd, address);
> > +
> > +out:
> > +	return pte;
> > +}
>
>
> That's familiar looking code..
>
> I guess this should be given a more generic name then placed in
> mm/memory.c, which is where we do pagetable walking.

Apart from this, there looks like quite a bit of other mm code
that has been crammed into everywhere but mm/ (yes this has
happened before, but it shouldn't be encouraged in new code).

For this specific example, I'm not sure that a function returning
a pointer to a pte is a good idea to be exporting. I'd like to see
some good reasons why things like get_user_pages, find_*_page, and
other standard APIs can't be used. Then you can list those reasons
in an individual patch to add your required API to mm/. This can
be more easily reviewed by people who aren't as good at wading
through code as Andrew.

Also, adding your own mm code outside core files really makes
things hard to maintain and audit when somebody would like to
change anything.

--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 
-
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/