2006-08-10 19:48:10

by Andi Kleen

[permalink] [raw]
Subject: [PATCH for review] [123/145] i386: make fault notifier unconditional and export it

r

It's needed for external debuggers and overhead is very small.

Also make the actual notifier chain they use static

Cc: [email protected]

Signed-off-by: Andi Kleen <[email protected]>

---
arch/x86_64/mm/fault.c | 12 +++---------
1 files changed, 3 insertions(+), 9 deletions(-)

Index: linux/arch/x86_64/mm/fault.c
===================================================================
--- linux.orig/arch/x86_64/mm/fault.c
+++ linux/arch/x86_64/mm/fault.c
@@ -40,8 +40,7 @@
#define PF_RSVD (1<<3)
#define PF_INSTR (1<<4)

-#ifdef CONFIG_KPROBES
-ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
+static ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);

/* Hook to register for page fault notifications */
int register_page_fault_notifier(struct notifier_block *nb)
@@ -49,11 +48,13 @@ int register_page_fault_notifier(struct
vmalloc_sync_all();
return atomic_notifier_chain_register(&notify_page_fault_chain, nb);
}
+EXPORT_SYMBOL_GPL(register_page_fault_notifier);

int unregister_page_fault_notifier(struct notifier_block *nb)
{
return atomic_notifier_chain_unregister(&notify_page_fault_chain, nb);
}
+EXPORT_SYMBOL_GPL(unregister_page_fault_notifier);

static inline int notify_page_fault(enum die_val val, const char *str,
struct pt_regs *regs, long err, int trap, int sig)
@@ -67,13 +68,6 @@ static inline int notify_page_fault(enum
};
return atomic_notifier_call_chain(&notify_page_fault_chain, val, &args);
}
-#else
-static inline int notify_page_fault(enum die_val val, const char *str,
- struct pt_regs *regs, long err, int trap, int sig)
-{
- return NOTIFY_DONE;
-}
-#endif

void bust_spinlocks(int yes)
{


2006-08-13 15:29:02

by Adrian Bunk

[permalink] [raw]
Subject: Re: [PATCH for review] [123/145] i386: make fault notifier unconditional and export it

> It's needed for external debuggers and overhead is very small.
>...

We are currently trying to remove exports not used by any in-kernel
code.

The patch description also lacks the name of what you call "external
debuggers" (I assume the exports are not for a theoretical usage but for
an already existing debugger?). There is no reason for keeping a patch
description small.

Especially nowadays where people demand deprecation periods for removing
exports without any in-kernel users there must be a _very_ good
justification when adding such exports.

This is true for both the i386 and the x86_64 patches.

cu
Adrian

BTW1: The subject of this email is wrong (it's the x86_64 patch).
BTW2: Please use a valid To: header.

--

Gentoo kernels are 42 times more popular than SUSE kernels among
KLive users (a service by SUSE contractor Andrea Arcangeli that
gathers data about kernels from many users worldwide).

There are three kinds of lies: Lies, Damn Lies, and Statistics.
Benjamin Disraeli

2006-08-13 16:53:53

by Alan

[permalink] [raw]
Subject: Re: [PATCH for review] [123/145] i386: make fault notifier unconditional and export it

Ar Sul, 2006-08-13 am 17:28 +0200, ysgrifennodd Adrian Bunk:
> > It's needed for external debuggers and overhead is very small.
> >...
>
> We are currently trying to remove exports not used by any in-kernel
> code.

Wrong pronoun. I think you meant to type "You".


2006-08-13 17:08:33

by Adrian Bunk

[permalink] [raw]
Subject: Re: [PATCH for review] [123/145] i386: make fault notifier unconditional and export it

On Sun, Aug 13, 2006 at 06:11:45PM +0100, Alan Cox wrote:
> Ar Sul, 2006-08-13 am 17:28 +0200, ysgrifennodd Adrian Bunk:
> > > It's needed for external debuggers and overhead is very small.
> > >...
> >
> > We are currently trying to remove exports not used by any in-kernel
> > code.
>
> Wrong pronoun. I think you meant to type "You".

"You are currently trying to remove exports..."?
Wouldn't this sound as if Andi was doing this?

I thought the "We" was correct since it's at least Arjan and me.

If this was wrong all I can say is that I'm not a native English
speaker.

cu
Adrian

--

Gentoo kernels are 42 times more popular than SUSE kernels among
KLive users (a service by SUSE contractor Andrea Arcangeli that
gathers data about kernels from many users worldwide).

There are three kinds of lies: Lies, Damn Lies, and Statistics.
Benjamin Disraeli

2006-08-13 17:46:31

by Alan

[permalink] [raw]
Subject: Re: [PATCH for review] [123/145] i386: make fault notifier unconditional and export it

Ar Sul, 2006-08-13 am 19:08 +0200, ysgrifennodd Adrian Bunk:
> > Wrong pronoun. I think you meant to type "You".
>
> "You are currently trying to remove exports..."?
> Wouldn't this sound as if Andi was doing this?
>
> I thought the "We" was correct since it's at least Arjan and me.
>
> If this was wrong all I can say is that I'm not a native English
> speaker.

"We" tends to imply 'I speak for all of us'. I was pointing out that you
don't speak for everyone. The humour in making that point didn't
translate and apparently I ended up confusing you as well which wasn't
the intent.

The joys of language.

To put it more clearly "Not everyone agrees with the remove exports"
approach, at least not the "all unused" part.

Alan

2006-08-13 20:17:54

by Andi Kleen

[permalink] [raw]
Subject: Re: [PATCH for review] [123/145] i386: make fault notifier unconditional and export it

On Sunday 13 August 2006 17:28, Adrian Bunk wrote:
> > It's needed for external debuggers and overhead is very small.
> >...
>
> We are currently trying to remove exports not used by any in-kernel
> code.

That ``we'' doesn't include me at least.

>
> The patch description also lacks the name of what you call "external
> debuggers" (I assume the exports are not for a theoretical usage but for
> an already existing debugger?).

The fault chain is needed for pretty much any debugger, including
kgdb, kdb, nlkd. The one in this case was NLKD.

> Especially nowadays where people demand deprecation periods for removing
> exports without any in-kernel users there must be a _very_ good
> justification when adding such exports.

I've always exported symbols when people can make a reasonable case that they
need it for extern free non broken by design code.

On the other hand I have no problems with removing unused exports
that don't have such a case or are clearly not a useful external
interface.

> BTW1: The subject of this email is wrong (it's the x86_64 patch).

Fixed, thanks

-Andi

2006-08-14 00:03:24

by Keith Owens

[permalink] [raw]
Subject: Re: [PATCH for review] [123/145] i386: make fault notifier unconditional and export it

Andi Kleen (on Sun, 13 Aug 2006 22:17:48 +0200) wrote:
>On Sunday 13 August 2006 17:28, Adrian Bunk wrote:
>> > It's needed for external debuggers and overhead is very small.
>> >...
>>
>> We are currently trying to remove exports not used by any in-kernel
>> code.
>
>That ``we'' doesn't include me at least.
>
>>
>> The patch description also lacks the name of what you call "external
>> debuggers" (I assume the exports are not for a theoretical usage but for
>> an already existing debugger?).
>
>The fault chain is needed for pretty much any debugger, including
>kgdb, kdb, nlkd. The one in this case was NLKD.

No. The page fault event was only used by kprobes, but it slowed down
all code. That is why it was changed to only be present if kprobes was
in the system. kdb and AFAIK kgdb do not need the page fault chain.
nlkd might need it, but that does not seem to have been explicitly
stated.