* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Dirk Behme @ 2011-05-18 15:26 UTC (permalink / raw)
To: Alessio Igor Bogani
Cc: Rusty Russell, Anders Kaseorg, Tim Abbott, Tim Bird, LKML,
Linux Embedded, Jason Wessel
In-Reply-To: <1305665763-3988-1-git-send-email-abogani@kernel.org>
On 17.05.2011 22:56, Alessio Igor Bogani wrote:
> This work was supported by a hardware donation from the CE Linux Forum.
>
> Signed-off-by: Alessio Igor Bogani<abogani@kernel.org>
> ---
> kernel/module.c | 7 ++-----
> 1 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 1e2b657..795bdc7 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2055,11 +2055,8 @@ static const struct kernel_symbol *lookup_symbol(const char *name,
> const struct kernel_symbol *start,
> const struct kernel_symbol *stop)
> {
> - const struct kernel_symbol *ks = start;
> - for (; ks< stop; ks++)
> - if (strcmp(ks->name, name) == 0)
> - return ks;
> - return NULL;
> + return bsearch(name, start, stop - start,
> + sizeof(struct kernel_symbol), cmp_name);
> }
>
> static int is_exported(const char *name, unsigned long value,
The old version with the warning is in linux-next now
http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commitdiff;h=903996de9b35213aaa4162c24351a2cb2931d9ac
Best regards
Dirk
^ permalink raw reply
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Christoph Hellwig @ 2011-05-18 7:54 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, Alessio Igor Bogani, Rusty Russell, Anders Kaseorg,
Tim Abbott, LKML, Linux Embedded, Jason Wessel, Dirk Behme
In-Reply-To: <4DD305B3.3000707@am.sony.com>
On Tue, May 17, 2011 at 04:33:07PM -0700, Tim Bird wrote:
> That said, I can answer Greg's question. This is to speed up
> the symbol resolution on module loading. The last numbers I
> saw showed a reduction of about 15-20% for the module load
> time, for large-ish modules. Of course this is highly dependent
> on the size of the modules, what they do at load time, and how many
> symbols are looked up to link them into the kernel.
How large are these very large modules, and what are good examples for
that? And why do people overly care for the load time?
^ permalink raw reply
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Rusty Russell @ 2011-05-18 1:10 UTC (permalink / raw)
To: Dirk Behme
Cc: Alessio Igor Bogani, Anders Kaseorg, Tim Abbott, Tim Bird, LKML,
Linux Embedded, Jason Wessel
In-Reply-To: <4DD2CA02.9040707@googlemail.com>
On Tue, 17 May 2011 21:18:26 +0200, Dirk Behme <dirk.behme@googlemail.com> wrote:
> On 17.05.2011 05:52, Rusty Russell wrote:
> > On Mon, 16 May 2011 22:23:40 +0200, Alessio Igor Bogani<abogani@kernel.org> wrote:
> >> This work was supported by a hardware donation from the CE Linux Forum.
> >>
> >> Signed-off-by: Alessio Igor Bogani<abogani@kernel.org>
> >> ---
> >> kernel/module.c | 6 ++----
> >> 1 files changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/kernel/module.c b/kernel/module.c
> >> index 6a34337..54355c5 100644
> >> --- a/kernel/module.c
> >> +++ b/kernel/module.c
> >> @@ -2055,10 +2055,8 @@ static const struct kernel_symbol *lookup_symbol(const char *name,
> >> const struct kernel_symbol *stop)
> >> {
> >> const struct kernel_symbol *ks = start;
> >> - for (; ks< stop; ks++)
> >> - if (strcmp(ks->name, name) == 0)
> >> - return ks;
> >> - return NULL;
> >> + return bsearch(name, start, stop - start,
> >> + sizeof(struct kernel_symbol), cmp_name);
> >> }
> >>
> >> static int is_exported(const char *name, unsigned long value,
> >
> > Applied.
>
> Sorry, but where have you applied it?
The -rr tree which goes to linux-next.
> With the version above we should get a warning
>
> kernel/module.c: In function 'lookup_symbol':
> kernel/module.c:1809: warning: unused variable 'ks'
Yep, fixed that up too, thanks.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Rusty Russell @ 2011-05-18 1:07 UTC (permalink / raw)
To: Greg KH, Alessio Igor Bogani
Cc: Anders Kaseorg, Tim Abbott, Tim Bird, LKML, Linux Embedded,
Jason Wessel, Dirk Behme
In-Reply-To: <20110517232241.GA19140@kroah.com>
On Tue, 17 May 2011 16:22:41 -0700, Greg KH <greg@kroah.com> wrote:
> On Tue, May 17, 2011 at 10:56:03PM +0200, Alessio Igor Bogani wrote:
> > This work was supported by a hardware donation from the CE Linux Forum.
> >
> > Signed-off-by: Alessio Igor Bogani <abogani@kernel.org>
> > ---
>
> That's nice, but _why_ do this change? What does it buy us?
>
> Please explain why you make a change, not just who sponsored the change,
> that's not very interesting to developers.
I was going to let this pass, but since Greg flagged it...
It's sufficient given the context (it's the tail end of a series of
patches), but it's preferable to allude to the other patches in a case
like this. For example:
Now we have sorted symbols, we can use binary search for
kallsyms lookups as well.
(1) "Now we have sorted symbols" indicates to the reader that this has
just recently become possible.
(2) "as well." indicates that this was not the main justification for
sorting the symbols.
Ideally you would add some numbers, like so:
On my machine 'cat /proc/kallsyms' only takes 0.02 seconds, but
this halves it to 0.01 seconds.
(That's my results under kvm, which is a poor way to do timing, but you
get the idea).
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Tim Bird @ 2011-05-17 23:33 UTC (permalink / raw)
To: Greg KH
Cc: Alessio Igor Bogani, Rusty Russell, Anders Kaseorg, Tim Abbott,
LKML, Linux Embedded, Jason Wessel, Dirk Behme
In-Reply-To: <20110517232241.GA19140@kroah.com>
On 05/17/2011 04:22 PM, Greg KH wrote:
> On Tue, May 17, 2011 at 10:56:03PM +0200, Alessio Igor Bogani wrote:
>> This work was supported by a hardware donation from the CE Linux Forum.
>>
>> Signed-off-by: Alessio Igor Bogani <abogani@kernel.org>
>> ---
>
> That's nice, but _why_ do this change? What does it buy us?
>
> Please explain why you make a change, not just who sponsored the change,
> that's not very interesting to developers.
Just a note here on the attribution...
Alessio - you can remove the "hardware donation from CELF" line
after the first submission or so. It doesn't need to be on
every submission of the patch set, and it doesn't need to go into
the commit message for the patch set. We only want it associated
with the patch set somewhere Google-able (like LKML).
That said, I can answer Greg's question. This is to speed up
the symbol resolution on module loading. The last numbers I
saw showed a reduction of about 15-20% for the module load
time, for large-ish modules. Of course this is highly dependent
on the size of the modules, what they do at load time, and how many
symbols are looked up to link them into the kernel.
Alessio - do you have any timings you can share for the speedup?
-- Tim
=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation
Senior Staff Engineer, Sony Network Entertainment
=============================
^ permalink raw reply
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Greg KH @ 2011-05-17 23:22 UTC (permalink / raw)
To: Alessio Igor Bogani
Cc: Rusty Russell, Anders Kaseorg, Tim Abbott, Tim Bird, LKML,
Linux Embedded, Jason Wessel, Dirk Behme
In-Reply-To: <1305665763-3988-1-git-send-email-abogani@kernel.org>
On Tue, May 17, 2011 at 10:56:03PM +0200, Alessio Igor Bogani wrote:
> This work was supported by a hardware donation from the CE Linux Forum.
>
> Signed-off-by: Alessio Igor Bogani <abogani@kernel.org>
> ---
That's nice, but _why_ do this change? What does it buy us?
Please explain why you make a change, not just who sponsored the change,
that's not very interesting to developers.
thanks,
greg k-h
^ permalink raw reply
* [PATCH] module: Use binary search in lookup_symbol()
From: Alessio Igor Bogani @ 2011-05-17 20:56 UTC (permalink / raw)
To: Rusty Russell
Cc: Anders Kaseorg, Tim Abbott, Tim Bird, LKML, Linux Embedded,
Jason Wessel, Dirk Behme, Alessio Igor Bogani
In-Reply-To: <BANLkTinPG100vbVAN1RZWb3h9Xuu6SKJ0Q@mail.gmail.com>
This work was supported by a hardware donation from the CE Linux Forum.
Signed-off-by: Alessio Igor Bogani <abogani@kernel.org>
---
kernel/module.c | 7 ++-----
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index 1e2b657..795bdc7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2055,11 +2055,8 @@ static const struct kernel_symbol *lookup_symbol(const char *name,
const struct kernel_symbol *start,
const struct kernel_symbol *stop)
{
- const struct kernel_symbol *ks = start;
- for (; ks < stop; ks++)
- if (strcmp(ks->name, name) == 0)
- return ks;
- return NULL;
+ return bsearch(name, start, stop - start,
+ sizeof(struct kernel_symbol), cmp_name);
}
static int is_exported(const char *name, unsigned long value,
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Alessio Igor Bogani @ 2011-05-17 19:41 UTC (permalink / raw)
To: Dirk Behme
Cc: Rusty Russell, Anders Kaseorg, Tim Abbott, Tim Bird, LKML,
Linux Embedded, Jason Wessel
In-Reply-To: <4DD2CA02.9040707@googlemail.com>
Dear Mr. Behme,
2011/5/17 Dirk Behme <dirk.behme@googlemail.com>:
[...]
> With the version above we should get a warning
>
> kernel/module.c: In function 'lookup_symbol':
> kernel/module.c:1809: warning: unused variable 'ks'
Sorry It's my fault. I'll provide a right version in few hours.
Ciao,
Alessio
^ permalink raw reply
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Dirk Behme @ 2011-05-17 19:18 UTC (permalink / raw)
To: Rusty Russell
Cc: Alessio Igor Bogani, Anders Kaseorg, Tim Abbott, Tim Bird, LKML,
Linux Embedded, Jason Wessel
In-Reply-To: <878vu6nftw.fsf@rustcorp.com.au>
On 17.05.2011 05:52, Rusty Russell wrote:
> On Mon, 16 May 2011 22:23:40 +0200, Alessio Igor Bogani<abogani@kernel.org> wrote:
>> This work was supported by a hardware donation from the CE Linux Forum.
>>
>> Signed-off-by: Alessio Igor Bogani<abogani@kernel.org>
>> ---
>> kernel/module.c | 6 ++----
>> 1 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 6a34337..54355c5 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -2055,10 +2055,8 @@ static const struct kernel_symbol *lookup_symbol(const char *name,
>> const struct kernel_symbol *stop)
>> {
>> const struct kernel_symbol *ks = start;
>> - for (; ks< stop; ks++)
>> - if (strcmp(ks->name, name) == 0)
>> - return ks;
>> - return NULL;
>> + return bsearch(name, start, stop - start,
>> + sizeof(struct kernel_symbol), cmp_name);
>> }
>>
>> static int is_exported(const char *name, unsigned long value,
>
> Applied.
Sorry, but where have you applied it?
With the version above we should get a warning
kernel/module.c: In function 'lookup_symbol':
kernel/module.c:1809: warning: unused variable 'ks'
?
Best regards
Dirk
^ permalink raw reply
* Re: [PATCH 00/11] RFC: KBUS messaging subsystem
From: Florian Fainelli @ 2011-05-17 8:50 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Tony Ibbs, Grant Likely, lkml, Linux-embedded, Tibs at Kynesim,
Richard Watts
In-Reply-To: <20110322133640.5d5c88e4@bike.lwn.net>
Hello,
Sorry for this late answer.
On Tuesday 22 March 2011 20:36:40 Jonathan Corbet wrote:
> On Fri, 18 Mar 2011 17:21:09 +0000
>
> Tony Ibbs <tibs@tonyibbs.co.uk> wrote:
> > KBUS is a lightweight, Linux kernel mediated messaging system,
> > particularly intended for use in embedded environments.
>
> I've spent a bit of time looking at this code...this isn't a detailed
> review by any stretch, more like a few impressions.
>
> - Why kbus over, say, a user-space daemon and unix-domain sockets? I'm
> not sure I see the advantage that comes with putting this into kernel
> space.
I also fail to see why this would be required. In my opininon you are trading
the reliability over complexity by putting this in the kernel.
Most implementations (if not all) involving system-wide message delivery for
other daemons are running in user-space. Having a daemon for message delivery
to other kbus clients/servers does not not seem less reliable to me. If you
had in mind that this daemon might be killed under OOM conditions, then maybe
your whole system has an issue, issue which could be circumvented by making
sure the messaging process gets respawned when possible (upstart like
mechanism or such).
>
> - The interface is ... creative. If you have to do this in kernel space,
> it would be nice to do away with the split write()/ioctl() API for
> reading or writing messages. It seems like either a write(), OR an
> ioctl() with a message data pointer would suffice; that would cut the
> number of syscalls the applications need to make too.
>
> Even better might be to just use the socket API.
Indeed, I would also suggest having a look at what generic netlink already
provides like messages per application PID, multicasting and marshaling. If
you intend to keep a part of it in the kernel, you should have a look at this,
because from my experience with generic netlink, most of the hard job you are
re-doing here, has already been done in a generic manner.
>
> - Does anything bound the size of a message fed into the kernel with
> write()? I couldn't find it. It seems like an application could
> consume arbitrary amounts of kernel memory.
>
> - It would be good to use the kernel's dynamic debugging and tracing
> facilities rather than rolling your own.
>
> - There's lots of kmalloc()/memset() pairs that could be kzalloc().
>
> That's as far as I could get for now.
>
> Thanks,
>
> jon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-embedded"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Rusty Russell @ 2011-05-17 3:52 UTC (permalink / raw)
To: Alessio Igor Bogani
Cc: Anders Kaseorg, Tim Abbott, Tim Bird, LKML, Linux Embedded,
Jason Wessel, Dirk Behme, Alessio Igor Bogani
In-Reply-To: <1305577420-3685-1-git-send-email-abogani@kernel.org>
On Mon, 16 May 2011 22:23:40 +0200, Alessio Igor Bogani <abogani@kernel.org> wrote:
> This work was supported by a hardware donation from the CE Linux Forum.
>
> Signed-off-by: Alessio Igor Bogani <abogani@kernel.org>
> ---
> kernel/module.c | 6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 6a34337..54355c5 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2055,10 +2055,8 @@ static const struct kernel_symbol *lookup_symbol(const char *name,
> const struct kernel_symbol *stop)
> {
> const struct kernel_symbol *ks = start;
> - for (; ks < stop; ks++)
> - if (strcmp(ks->name, name) == 0)
> - return ks;
> - return NULL;
> + return bsearch(name, start, stop - start,
> + sizeof(struct kernel_symbol), cmp_name);
> }
>
> static int is_exported(const char *name, unsigned long value,
Applied.
Thanks!
Rusty.
^ permalink raw reply
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Joe Perches @ 2011-05-16 21:08 UTC (permalink / raw)
To: Alessio Igor Bogani
Cc: Rusty Russell, Anders Kaseorg, Tim Abbott, Tim Bird, LKML,
Linux Embedded, Jason Wessel, Dirk Behme
In-Reply-To: <1305579694.2503.18.camel@Joe-Laptop>
On Mon, 2011-05-16 at 14:01 -0700, Joe Perches wrote:
> On Mon, 2011-05-16 at 22:23 +0200, Alessio Igor Bogani wrote:
> > Signed-off-by: Alessio Igor Bogani <abogani@kernel.org>
> > diff --git a/kernel/module.c b/kernel/module.c
> > @@ -2055,10 +2055,8 @@ static const struct kernel_symbol *lookup_symbol(const char *name,
> > const struct kernel_symbol *stop)
> > {
> > const struct kernel_symbol *ks = start;
> > - for (; ks < stop; ks++)
> > - if (strcmp(ks->name, name) == 0)
> > - return ks;
> > - return NULL;
> > + return bsearch(name, start, stop - start,
> > + sizeof(struct kernel_symbol), cmp_name);
[]
> why cmp_name and not strcmp?
Nevermind, cmp_name is obviously correct.
^ permalink raw reply
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Joe Perches @ 2011-05-16 21:01 UTC (permalink / raw)
To: Alessio Igor Bogani
Cc: Rusty Russell, Anders Kaseorg, Tim Abbott, Tim Bird, LKML,
Linux Embedded, Jason Wessel, Dirk Behme
In-Reply-To: <1305577420-3685-1-git-send-email-abogani@kernel.org>
On Mon, 2011-05-16 at 22:23 +0200, Alessio Igor Bogani wrote:
> This work was supported by a hardware donation from the CE Linux Forum.
>
> Signed-off-by: Alessio Igor Bogani <abogani@kernel.org>
> ---
> kernel/module.c | 6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 6a34337..54355c5 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2055,10 +2055,8 @@ static const struct kernel_symbol *lookup_symbol(const char *name,
> const struct kernel_symbol *stop)
> {
> const struct kernel_symbol *ks = start;
> - for (; ks < stop; ks++)
> - if (strcmp(ks->name, name) == 0)
> - return ks;
> - return NULL;
> + return bsearch(name, start, stop - start,
> + sizeof(struct kernel_symbol), cmp_name);
> }
>
> static int is_exported(const char *name, unsigned long value,
ks isn't used anymore.
why cmp_name and not strcmp?
^ permalink raw reply
* [PATCH] module: Use binary search in lookup_symbol()
From: Alessio Igor Bogani @ 2011-05-16 20:23 UTC (permalink / raw)
To: Rusty Russell
Cc: Anders Kaseorg, Tim Abbott, Tim Bird, LKML, Linux Embedded,
Jason Wessel, Dirk Behme, Alessio Igor Bogani
In-Reply-To: <BANLkTi=2jW5PHB6QCaddHeZH0P=9PKWosw@mail.gmail.com>
This work was supported by a hardware donation from the CE Linux Forum.
Signed-off-by: Alessio Igor Bogani <abogani@kernel.org>
---
kernel/module.c | 6 ++----
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index 6a34337..54355c5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2055,10 +2055,8 @@ static const struct kernel_symbol *lookup_symbol(const char *name,
const struct kernel_symbol *stop)
{
const struct kernel_symbol *ks = start;
- for (; ks < stop; ks++)
- if (strcmp(ks->name, name) == 0)
- return ks;
- return NULL;
+ return bsearch(name, start, stop - start,
+ sizeof(struct kernel_symbol), cmp_name);
}
static int is_exported(const char *name, unsigned long value,
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Anders Kaseorg @ 2011-05-16 18:02 UTC (permalink / raw)
To: Dirk Behme
Cc: Alessio Igor Bogani, Rusty Russell, Tim Abbott, Tim Bird, LKML,
Linux Embedded, Jason Wessel
In-Reply-To: <4DD14469.5030804@googlemail.com>
On Mon, May 16, 2011 at 11:36, Dirk Behme <dirk.behme@googlemail.com> wrote:
> Then, thinking again, results in the question if the first argument of
> bsearch() shouldn't be 'name' rather than 'ks->name'?
Yes, it should be.
> Then it would be the job of cmp_name() to check for start == stop == 0?
Actually bsearch already handles this case correctly (it will make no
calls to cmp_name and return NULL), so nothing needs to specially
check for it.
Anders
^ permalink raw reply
* Re: [PATCH] module: Use binary search in lookup_symbol()
From: Dirk Behme @ 2011-05-16 15:36 UTC (permalink / raw)
To: Alessio Igor Bogani
Cc: Rusty Russell, Tim Abbott, Anders Kaseorg, Tim Bird, LKML,
Linux Embedded, Jason Wessel
In-Reply-To: <1304455330-2728-1-git-send-email-abogani@kernel.org>
On 03.05.2011 22:42, Alessio Igor Bogani wrote:
> This work was supported by a hardware donation from the CE Linux Forum.
>
> Signed-off-by: Alessio Igor Bogani<abogani@kernel.org>
> ---
> kernel/module.c | 6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 6a34337..a1f841e 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2055,10 +2055,8 @@ static const struct kernel_symbol *lookup_symbol(const char *name,
> const struct kernel_symbol *stop)
> {
> const struct kernel_symbol *ks = start;
> - for (; ks< stop; ks++)
> - if (strcmp(ks->name, name) == 0)
> - return ks;
> - return NULL;
> + return bsearch(ks->name, start, stop - start,
> + sizeof(struct kernel_symbol), cmp_name);
> }
Back porting this patch to a 2.6.34.9 based ARM system fails with an
Oops at 0x00000004. Debugging shows that both start and stop are 0 in
this case resulting in ks->name accessing 0x00000004. The original
code checked for this by 'ks < stop' in the for loop.
So the first idea was that the code should be
if(k < stop)
return bsearch();
else
return NULL;
Then, thinking again, results in the question if the first argument of
bsearch() shouldn't be 'name' rather than 'ks->name'? Then it would be
the job of cmp_name() to check for start == stop == 0? I.e.
return bsearch(name, ...);
?
Best regards
Dirk
^ permalink raw reply
* Re: [PATCH 0/4] Speed up the symbols' resolution process V4
From: Alessio Igor Bogani @ 2011-05-15 8:28 UTC (permalink / raw)
To: Mike Frysinger
Cc: Rusty Russell, Tim Abbott, Anders Kaseorg, Jason Wessel, Tim Bird,
LKML, Linux Embedded
In-Reply-To: <BANLkTiktnQyEnNiQdTr_p=PG66rzVkr9ig@mail.gmail.com>
Dear Mr. Frysinger,
2011/5/14 Mike Frysinger <vapier.adi@gmail.com>:
> On Fri, May 13, 2011 at 03:01, Alessio Igor Bogani wrote:
>> I changed the "__" in "+". Could you test this, please?
>> http://git.kernel.org/?p=linux/kernel/git/abogani/linux-2.6.git;a=shortlog;h=refs/heads/ksym-sorted-v6
>> Thanks a lot!
>
> seems to build & boot ok for me. Â cheers.
> -mike
Thank you very much!
Ciao,
Alessio
^ permalink raw reply
* Re: [PATCH 0/4] Speed up the symbols' resolution process V4
From: Mike Frysinger @ 2011-05-14 17:32 UTC (permalink / raw)
To: Alessio Igor Bogani
Cc: Rusty Russell, Tim Abbott, Anders Kaseorg, Jason Wessel, Tim Bird,
LKML, Linux Embedded
In-Reply-To: <BANLkTinQx+8T3Hv0T5yPEAtxtU87CcOJVw@mail.gmail.com>
On Fri, May 13, 2011 at 03:01, Alessio Igor Bogani wrote:
> I changed the "__" in "+". Could you test this, please?
> http://git.kernel.org/?p=linux/kernel/git/abogani/linux-2.6.git;a=shortlog;h=refs/heads/ksym-sorted-v6
> Thanks a lot!
seems to build & boot ok for me. cheers.
-mike
^ permalink raw reply
* Re: [PATCH 0/4] Speed up the symbols' resolution process V4
From: Alessio Igor Bogani @ 2011-05-13 7:01 UTC (permalink / raw)
To: Mike Frysinger
Cc: Rusty Russell, Tim Abbott, Anders Kaseorg, Jason Wessel, Tim Bird,
LKML, Linux Embedded
In-Reply-To: <BANLkTikyR7ByAgxtbz-gOHspLMFo4a+vaQ@mail.gmail.com>
Dear Mr. Frysinger,
I changed the "__" in "+". Could you test this, please?
http://git.kernel.org/?p=linux/kernel/git/abogani/linux-2.6.git;a=shortlog;h=refs/heads/ksym-sorted-v6
Thanks a lot!
Ciao,
Alessio
^ permalink raw reply
* Re: [PATCH 0/4] Speed up the symbols' resolution process V4
From: Mike Frysinger @ 2011-05-12 14:30 UTC (permalink / raw)
To: Alessio Igor Bogani
Cc: Rusty Russell, Tim Abbott, Anders Kaseorg, Jason Wessel, Tim Bird,
LKML, Linux Embedded
In-Reply-To: <BANLkTi=fhsSP069WMNPN8BA=E1=1TZ6Zmw@mail.gmail.com>
On Thu, May 12, 2011 at 05:10, Alessio Igor Bogani wrote:
> 2011/5/11 Mike Frysinger:
> [...]
>> if you export _foo/foo, you'll get an error with the current code:
>> /* EXPORT_SYMBOL(foo); */
>> .section ___ksymtab__foo,"a",@progbits
>> ___ksymtab_foo:
>> /* EXPORT_SYMBOL(_foo); */
>> .section ___ksymtab___foo,"a",@progbits
>> ___ksymtab__foo:
> [...]
>
> So I can suggest two possible solutions for section names:
>
> 1) As you suggested change "__" to "+" so
> i.e. ___ksymtab+foo
>
> 2) Pick a more appropriate name:
> i.e. ___ksym__foo
> or
> i.e. ___ksymsec__foo
>
> In fact these section names aren't a table of symbols (in ksymtab the
> "tab" part stand for table, I suppose) so I think that name should be
> changed accordingly (my patchset create a temporary section for every
> symbol).
>
> Which do you prefer?
i suggested the diff split char as i dont know how embedded the
"ksymtab" section name is in the rest of the tree. but if changing
that is an option, that'd work too. either works for me, so whichever
is less effort for you.
-mike
^ permalink raw reply
* Re: [PATCH 0/4] Speed up the symbols' resolution process V4
From: Alessio Igor Bogani @ 2011-05-12 9:10 UTC (permalink / raw)
To: Mike Frysinger
Cc: Rusty Russell, Tim Abbott, Anders Kaseorg, Jason Wessel, Tim Bird,
LKML, Linux Embedded
In-Reply-To: <BANLkTikFSN-DkkPNPtv1VigCg08YoB_q2g@mail.gmail.com>
Dear Mr. Frysinger,
Thank you for excellent explanation!
2011/5/11 Mike Frysinger <vapier.adi@gmail.com>:
[...]
> if you export _foo/foo, you'll get an error with the current code:
> /* EXPORT_SYMBOL(foo); */
> .section ___ksymtab__foo,"a",@progbits
> ___ksymtab_foo:
> /* EXPORT_SYMBOL(_foo); */
> .section ___ksymtab___foo,"a",@progbits
> ___ksymtab__foo:
[...]
So I can suggest two possible solutions for section names:
1) As you suggested change "__" to "+" so
i.e. ___ksymtab+foo
2) Pick a more appropriate name:
i.e. ___ksym__foo
or
i.e. ___ksymsec__foo
In fact these section names aren't a table of symbols (in ksymtab the
"tab" part stand for table, I suppose) so I think that name should be
changed accordingly (my patchset create a temporary section for every
symbol).
Which do you prefer?
Ciao,
Alessio
^ permalink raw reply
* Re: [PATCH 0/4] Speed up the symbols' resolution process V4
From: Mike Frysinger @ 2011-05-11 15:52 UTC (permalink / raw)
To: Alessio Igor Bogani
Cc: Rusty Russell, Tim Abbott, Anders Kaseorg, Jason Wessel, Tim Bird,
LKML, Linux Embedded
In-Reply-To: <BANLkTi=OXuR4iJFqUEa4pA=yO0aXaqEzyA@mail.gmail.com>
On Wed, May 11, 2011 at 11:25, Alessio Igor Bogani wrote:
> 2011/5/11 Mike Frysinger:
>>> Sorry I don't think that is a good choice from a long term point of
>>> view. What do you think to add MODULE_SYMBOL_PREFIX to section names
>>> instead? In this way symbol and section names should always be
>>> different also on symbol prefixed archs (which are blackfin and
>>> h8300).
>>
>> that doesnt work. it simply delays the problem to another set of
>> underscores. so with that change, local_bh_enable/_local_bh_enable
>> work, but now send_remote_softirq/__send_remote_softirq fail:
>
> In my opinion it should work. if I use SYMBOL_PREFIX + two underscore
> for section name I should always obtain different names.
> So if SYMBOL_PREFIX is "_" section name will be "___", if
> SYMBOL_PREFIX is "__" section name will be "____" and so on.
i dont follow your logic. SYMBOL_PREFIX is simply an underscore for
Blackfin, so you are not guaranteed unique names when you're only
prefixing more underscores. the issue isnt with a symbol colliding
with itself, the issue is with a symbol colliding with another symbol
that has underscores added to its name.
if you export _foo/foo, you'll get an error with the current code:
/* EXPORT_SYMBOL(foo); */
.section ___ksymtab__foo,"a",@progbits
___ksymtab_foo:
/* EXPORT_SYMBOL(_foo); */
.section ___ksymtab___foo,"a",@progbits
___ksymtab__foo:
you can see how the first section and the last symbol collide
by putting the symbol prefix along the lines of the separator, you
delay it to __foo/foo:
/* EXPORT_SYMBOL(foo); */
.section ___ksymtab___foo,"a",@progbits
___ksymtab_foo:
/* EXPORT_SYMBOL(__foo); */
.section ___ksymtab_____foo,"a",@progbits
___ksymtab___foo:
the Blackfin toolchain puts the prefix before the ksymtab part while
you're putting it after, so there is always a possibility of collision
unless you pick a split char that doesnt include an underscore.
>> CC kernel/softirq.o
>> nano /tmp/cconhYy1.s: Assembler messages:
>> /tmp/cconhYy1.s:3664: Error: symbol `___ksymtab___send_remote_softirq'
>> is already defined
>> make[1]: *** [kernel/softirq.o] Error 1
>
> I'm a bit confused. I can build a kernel here:
> $ make ARCH=blackfin CROSS_COMPILE="bfin-uclinux-" defconfig
it's failing for me with current linux-next and the patch you posted
previously. also, you dont need to set CROSS_COMPILE as that is the
default value for ARCH=blackfin ...
> Unfortunately I can't make skyeye emulator works to test the obtained
> kernel image.
skyeye is crap. the latest upstream gdb/sim code can boot a kernel,
but if you aren't up-to-date with that, this binary i've been using
locally should work too:
http://blackfin.uclinux.org/uimages/bfin-elf-run
$ bfin-elf-run --env operating --model bf537 --sirev 2 ./vmlinux
earlyprintk=serial,uart0 console=ttyBF0
Linux version 2.6.39-rc7-ADI-2011R1-pre-00132-g9d91c9a
(vapier@vapier-m) (gcc version 4.3.5 (ADI-trunk/git-ce854ee) ) #16 Wed
May 11 11:55:28 EDT 2011
register early platform devices
bootconsole [early_shadow0] enabled
bootconsole [early_BFuart0] enabled
early printk enabled on early_BFuart0
Limiting kernel memory to 56MB due to anomaly 05000263
Board Memory: 64MB
Kernel Managed Memory: 64MB
Memory map:
fixedcode = 0x00000400-0x00000490
text = 0x00001000-0x000f7f30
rodata = 0x000f7f30-0x001456ac
.............
-mike
^ permalink raw reply
* Re: [PATCH 0/4] Speed up the symbols' resolution process V4
From: Alessio Igor Bogani @ 2011-05-11 15:25 UTC (permalink / raw)
To: Mike Frysinger
Cc: Rusty Russell, Tim Abbott, Anders Kaseorg, Jason Wessel, Tim Bird,
LKML, Linux Embedded
In-Reply-To: <BANLkTi=JXq+V783amiJB-qLX=7PaeH-83w@mail.gmail.com>
Dear Mr. Frysinger,
2011/5/11 Mike Frysinger <vapier.adi@gmail.com>:
[...]
>> Sorry I don't think that is a good choice from a long term point of
>> view. What do you think to add MODULE_SYMBOL_PREFIX to section names
>> instead? In this way symbol and section names should always be
>> different also on symbol prefixed archs (which are blackfin and
>> h8300).
>
> that doesnt work. it simply delays the problem to another set of
> underscores. so with that change, local_bh_enable/_local_bh_enable
> work, but now send_remote_softirq/__send_remote_softirq fail:
In my opinion it should work. if I use SYMBOL_PREFIX + two underscore
for section name I should always obtain different names.
So if SYMBOL_PREFIX is "_" section name will be "___", if
SYMBOL_PREFIX is "__" section name will be "____" and so on.
> CC kernel/softirq.o
> nano /tmp/cconhYy1.s: Assembler messages:
> /tmp/cconhYy1.s:3664: Error: symbol `___ksymtab___send_remote_softirq'
> is already defined
> make[1]: *** [kernel/softirq.o] Error 1
I'm a bit confused. I can build a kernel here:
$ make ARCH=blackfin CROSS_COMPILE="bfin-uclinux-" defconfig
*** Default configuration is based on 'BF537-STAMP_defconfig'
[...]
$ make ARCH=blackfin CROSS_COMPILE="bfin-uclinux-"
CHK include/linux/version.h
CHK include/generated/utsrelease.h
CALL scripts/checksyscalls.sh
[...]
OBJCOPY arch/blackfin/boot/vmlinux.bin
GZIP arch/blackfin/boot/vmlinux.bin.gz
UIMAGE arch/blackfin/boot/vmImage.gz
Image Name: bf537-0.2-2.6.39-rc3-00004-gf26a
Created: Wed May 11 17:06:45 2011
Image Type: Blackfin Linux Kernel Image (gzip compressed)
Data Size: 986471 Bytes = 963.35 kB = 0.94 MB
Load Address: 00001000
Entry Point: 001a8518
Building modules, stage 2.
MODPOST 69 modules
Unfortunately I can't make skyeye emulator works to test the obtained
kernel image.
Ciao,
Alessio
^ permalink raw reply
* Re: [PATCH 0/4] Speed up the symbols' resolution process V4
From: Mike Frysinger @ 2011-05-11 14:47 UTC (permalink / raw)
To: Alessio Igor Bogani
Cc: Rusty Russell, Tim Abbott, Anders Kaseorg, Jason Wessel, Tim Bird,
LKML, Linux Embedded
In-Reply-To: <BANLkTinDiGCaKPtS+ZpKf2swJpParJ4eew@mail.gmail.com>
On Wed, May 11, 2011 at 05:19, Alessio Igor Bogani wrote:
>> name. Â so rather than "__", use "+".
>
> Sorry I don't think that is a good choice from a long term point of
> view. What do you think to add MODULE_SYMBOL_PREFIX to section names
> instead? In this way symbol and section names should always be
> different also on symbol prefixed archs (which are blackfin and
> h8300).
that doesnt work. it simply delays the problem to another set of
underscores. so with that change, local_bh_enable/_local_bh_enable
work, but now send_remote_softirq/__send_remote_softirq fail:
CC kernel/softirq.o
nano /tmp/cconhYy1.s: Assembler messages:
/tmp/cconhYy1.s:3664: Error: symbol `___ksymtab___send_remote_softirq'
is already defined
make[1]: *** [kernel/softirq.o] Error 1
so any option that involves using underscores as the separator will
not work. pick something else, like "+" or "." or ... i dont see a
problem with using these.
-mike
^ permalink raw reply
* Re: [PATCH 0/4] Speed up the symbols' resolution process V4
From: Alessio Igor Bogani @ 2011-05-11 13:44 UTC (permalink / raw)
To: Mike Frysinger
Cc: Rusty Russell, Tim Abbott, Anders Kaseorg, Jason Wessel, Tim Bird,
LKML, Linux Embedded
In-Reply-To: <BANLkTinDiGCaKPtS+ZpKf2swJpParJ4eew@mail.gmail.com>
Dear Mr. Frysinger,
2011/5/11 Alessio Igor Bogani <abogani@kernel.org>:
[...]
>>> this breaks symbol prefixed arches (like Blackfin):
>>> CC kernel/softirq.o
>>> /tmp/ccp3A6LU.s: Assembler messages:
>>> /tmp/ccp3A6LU.s:3734: Error: symbol `___ksymtab__local_bh_enable' is
>>> already defined
[...]
>>> name. so rather than "__", use "+".
>
> Sorry I don't think that is a good choice from a long term point of
> view. What do you think to add MODULE_SYMBOL_PREFIX to section names
> instead? In this way symbol and section names should always be
> different also on symbol prefixed archs (which are blackfin and
> h8300).
I'm thinking to something like this:
diff --git a/include/linux/module.h b/include/linux/module.h
index 98ddaf0..c4aa266 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -223,7 +223,7 @@ struct module_use {
extern void *__crc_##sym __attribute__((weak)); \
static const unsigned long __kcrctab_##sym \
__used \
- __attribute__((section("___kcrctab" sec "__"#sym), unused)) \
+ __attribute__((section("___kcrctab" sec "___" MODULE_SYMBOL_PREFIX
#sym), unused)) \
= (unsigned long) &__crc_##sym;
#else
#define __CRC_SYMBOL(sym, sec)
@@ -238,7 +238,7 @@ struct module_use {
= MODULE_SYMBOL_PREFIX #sym; \
static const struct kernel_symbol __ksymtab_##sym \
__used \
- __attribute__((section("___ksymtab" sec "__"#sym), unused)) \
+ __attribute__((section("___ksymtab" sec "___" MODULE_SYMBOL_PREFIX
#sym), unused)) \
= { (unsigned long)&sym, __kstrtab_##sym }
#define EXPORT_SYMBOL(sym) \
Could you try this, please?
Thank you very much!
Ciao,
Alessio
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox