LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Pekka Paalanen <pq@iki.fi>
To: Harvey Harrison <harvey.harrison@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>,
	linux-kernel@vger.kernel.org, Jan Beulich <jbeulich@novell.com>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>
Subject: Re: [PATCH] x86: Add a list for custom page fault handlers.
Date: Wed, 30 Jan 2008 20:08:27 +0200	[thread overview]
Message-ID: <20080130200827.322c4f7d@daedalus.pq.iki.fi> (raw)
In-Reply-To: <1201660453.8837.13.camel@brick>

On Tue, 29 Jan 2008 18:34:13 -0800
Harvey Harrison <harvey.harrison@gmail.com> wrote:

> Sorry, attached the wrong version to my last message missing the
> kdebug.h hunk.  This is still just a straight port to current x86.git.

Wow, thanks.

I have to say I already did this for myself, but I had to
change too many things to simply resubmit without testing. And it turned
out porting mmiotrace to x86/mm was a non-trivial task. I have not
been able to test yet even though I have been working on it every night.
Today I will see if things work for me, and hopefully also for someone else.
I might send new patches today or tomorrow, if all goes well.

btw. I have a bad feeling about global_flush_tlb() disappearing, it was used
in mmiotrace code, and I don't know what to use instead. The obvious
replacement for it is a static function elsewhere. Currently I am simply
hoping it was a redundant leftover from the time when we had trouble making
mmiotrace stable.

To be honest, I haven't yet had the time to educate myself about what is
TLB.

> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index e28cc52..c6c8164 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -49,6 +49,54 @@
>  #define PF_RSVD		(1<<3)
>  #define PF_INSTR	(1<<4)
>  
> +#ifdef CONFIG_PAGE_FAULT_HANDLERS
> +static HLIST_HEAD(pf_handlers); /* protected by RCU */
> +static DEFINE_SPINLOCK(pf_handlers_writer);
> +
> +void register_page_fault_handler(struct pf_handler *new_pfh)
> +{
> +	spin_lock(&pf_handlers_writer);
> +	hlist_add_head_rcu(&new_pfh->hlist, &pf_handlers);
> +	spin_unlock(&pf_handlers_writer);

Here I had to change to irqsave/irqrestore versions, as I found a possible
lock dependency issue. Lockdep debugging code pointed it out. I'm not sure
I fixed it completely, but using spin_lock_irqsave() is a safe bet.

> +}
> +EXPORT_SYMBOL_GPL(register_page_fault_handler);
> +
> +void unregister_page_fault_handler(struct pf_handler *old_pfh)
> +{
> +	might_sleep();
> +	spin_lock(&pf_handlers_writer);
> +	hlist_del_rcu(&old_pfh->hlist);
> +	spin_unlock(&pf_handlers_writer);
> +	synchronize_rcu();
> +}

And here. And I also removed sync RCU and might_sleep().

> +EXPORT_SYMBOL_GPL(unregister_page_fault_handler);
> +#endif
> +
> +/* returns non-zero if do_page_fault() should return */
> +static int handle_custom_pf(struct pt_regs *regs, unsigned long error_code,
> +			    unsigned long address)
> +{
> +#ifdef CONFIG_PAGE_FAULT_HANDLERS
> +	int ret = 0;
> +	struct pf_handler *cur;
> +	struct hlist_node *ncur;
> +
> +	if (hlist_empty(&pf_handlers))
> +		return 0;

This looks prettier than my code. I might change my code to this.

> +	rcu_read_lock();
> +	hlist_for_each_entry_rcu(cur, ncur, &pf_handlers, hlist) {
> +		ret = cur->handler(regs, error_code, address);
> +		if (ret)
> +			break;
> +	}
> +	rcu_read_unlock();
> +	return ret;
> +#else
> +	return 0;
> +#endif
> +}
> +
>  static inline int notify_page_fault(struct pt_regs *regs)
>  {
>  #ifdef CONFIG_KPROBES
> @@ -588,6 +636,9 @@ void __kprobes do_page_fault(struct pt_regs *regs, unsigned long error_code)
>  	if (notify_page_fault(regs))
>  		return;
>  
> +	if (handle_custom_pf(regs, error_code, address))
> +		return;
> +

This is the non-trivial change that I really want to test before
submitting again. The call site of the handlers changes, and vmalloc
related faults reach the custom handlers. I don't think this would be a
problem for mmiotrace, but I don't know enough to be sure.
I guess using vmalloc'd memory from a page fault handler would be doomed,
anyway, so no point worrying about it, right?

if (unlikely(handle_custom_pf(regs, error_code, address)))
Maybe like this, even?

It is still supposedly making a function call every time, at least
when CONFIG_PAGE_FAULT_HANDLERS is defined. Would it have a non-negligible
impact on performance when no custom handlers are registered?

My original version had the separate inline function to check if the list
is empty. Which one is preferrable?

I'm hoping distros would ship some kernels with PAGE_FAULT_HANDLERS
enabled as it would make contributors' lives easier. IIRC some
already have enabled RELAY and DEBUG_FS for mmiotrace. This is why I
think keeping the performance impact to the minimum here is important,
for the case of no custom handlers registered.


Thanks,

-- 
Pekka Paalanen
http://www.iki.fi/pq/

  reply	other threads:[~2008-01-30 18:08 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-01-27 16:52 Pekka Paalanen
2008-01-27 17:55 ` [RFC PATCH] x86: mmiotrace - trace memory mapped IO Pekka Paalanen
2008-01-30 22:39   ` Pekka Paalanen
2008-01-27 19:29 ` [PATCH] x86: Add a list for custom page fault handlers Ingo Molnar
2008-01-27 21:03 ` Peter Zijlstra
2008-01-30  2:28 ` Harvey Harrison
2008-01-30  2:34   ` Harvey Harrison
2008-01-30 18:08     ` Pekka Paalanen [this message]
2008-01-31 15:07       ` Ingo Molnar
2008-01-31 16:02         ` [PATCH v2] " Pekka Paalanen
2008-01-31 16:15           ` Arjan van de Ven
2008-02-03  6:55             ` Pekka Paalanen
2008-02-03  7:03               ` Ingo Molnar
2008-02-03 21:40                 ` Pekka Paalanen
2008-02-05 20:28                 ` [PATCH 1/4] x86 mmiotrace: use lookup_address() Pekka Paalanen
2008-02-05 20:30                   ` [PATCH 2/4] x86 mmiotrace: fix relay-buffer-full flag for SMP Pekka Paalanen
2008-02-05 20:44                     ` Eric Dumazet
2008-02-05 21:14                       ` Pekka Paalanen
2008-02-05 21:35                         ` Eric Dumazet
2008-02-09 17:53                           ` [PATCH] x86 mmiotrace: Use percpu instead of arrays Pekka Paalanen
2008-02-05 20:31                   ` [PATCH 3/4] x86 mmiotrace: comment about user space ABI Pekka Paalanen
2008-02-05 20:39                   ` [PATCH 4/4] x86 mmiotrace: move files into arch/x86/mm/ Pekka Paalanen
2008-02-06  3:02                     ` Randy Dunlap
2008-02-09 11:21                       ` Pekka Paalanen
2008-02-07 12:53                     ` Ingo Molnar
2008-02-07 12:56                       ` Christoph Hellwig
2008-02-09 17:52                         ` [RFC PATCH] x86: explicit call to mmiotrace in do_page_fault() Pekka Paalanen
2008-02-09 18:01                           ` Arjan van de Ven
2008-02-09 18:23                             ` Pekka Paalanen
2008-02-09 18:56                               ` Pekka Enberg
2008-02-09 19:11                                 ` Pekka Paalanen
2008-02-09 19:19                                   ` Pekka Enberg
2008-02-09 18:39                             ` Peter Zijlstra
2008-02-09 18:39                           ` Peter Zijlstra
2008-02-10 18:05                             ` [RFC PATCH v2] " Pekka Paalanen
2008-02-11  2:12                               ` Pavel Roskin
2008-02-11 18:04                                 ` Pekka Paalanen
2008-02-06  5:00                   ` [PATCH 1/4] x86 mmiotrace: use lookup_address() Christoph Hellwig
2008-02-07 12:52                     ` Ingo Molnar
2008-01-31 16:16           ` [RFC PATCH v2] x86: mmiotrace - trace memory mapped IO Pekka Paalanen
2008-01-31 16:29             ` Arjan van de Ven
2008-02-03  7:21               ` Pekka Paalanen
2008-01-30 18:20 ` [PATCH] x86: Add a list for custom page fault handlers Arjan van de Ven

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=20080130200827.322c4f7d@daedalus.pq.iki.fi \
    --to=pq@iki.fi \
    --cc=a.p.zijlstra@chello.nl \
    --cc=harvey.harrison@gmail.com \
    --cc=jbeulich@novell.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --subject='Re: [PATCH] x86: Add a list for custom page fault handlers.' \
    /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).