| Message ID | 1343483279-11572-2-git-send-email-marex@denx.de |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Marek Vasut, In message <1343483279-11572-2-git-send-email-marex@denx.de> you wrote: > This shall eliminate the need for bubblesorting of commands at runtime. > Every command definition structure is now put into it's own subsection > of section .u_boot_cmd, that is .u_boot_cmd.<name> . These are then put > into .u_boot_cmd by linker and lastly, linker uses SORT() over these > subsections to make proper order on them. This shall eliminate some > runtime overhead. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Wolfgang Denk <wd@denx.de> > Cc: Mike Frysinger <vapier@gentoo.org> > --- > arch/arm/cpu/u-boot.lds | 2 +- > common/cmd_help.c | 2 +- > include/command.h | 9 ++++++--- > 3 files changed, 8 insertions(+), 5 deletions(-) Seems incomplete in several aspects: 1) what about all the non-ARM architecures and the board specific linker scripts? 2) what about removing the sort code? Best regards, Wolfgang Denk
Dear Wolfgang Denk, > Dear Marek Vasut, > > In message <1343483279-11572-2-git-send-email-marex@denx.de> you wrote: > > This shall eliminate the need for bubblesorting of commands at runtime. > > Every command definition structure is now put into it's own subsection > > of section .u_boot_cmd, that is .u_boot_cmd.<name> . These are then put > > into .u_boot_cmd by linker and lastly, linker uses SORT() over these > > subsections to make proper order on them. This shall eliminate some > > runtime overhead. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Wolfgang Denk <wd@denx.de> > > Cc: Mike Frysinger <vapier@gentoo.org> > > --- > > > > arch/arm/cpu/u-boot.lds | 2 +- > > common/cmd_help.c | 2 +- > > include/command.h | 9 ++++++--- > > 3 files changed, 8 insertions(+), 5 deletions(-) > > Seems incomplete in several aspects: Below the section: * NOTE * THIS PATCH IS CRAZY There are a few notes. I'd actually like to know if this approach is correct at all, it might break on some crazy configurations or such. > 1) what about all the non-ARM architecures and the board specific > linker scripts? - This patch affects only arm926t, obviously to make it proper, every linkerscript would have to be adjusted Which sucks, since there're a lot of them. But it can probably be automated. > 2) what about removing the sort code? You mean in the _do_help() in common/command.c? We can do not only that, but we can do bisect search in find_cmd_tbl() now too. I'm still trying to figure out the most optimal implementation. The current one I have trimmed down the time by roughly 60%, but I don't like it. > Best regards, > > Wolfgang Denk Best regards, Marek Vasut
Dear Marek Vasut, In message <201207282039.34518.marex@denx.de> you wrote: > > > Seems incomplete in several aspects: > > Below the section: > > * NOTE * THIS PATCH IS CRAZY Then what is actually the purpose of such a posting? Just dumping unsorted thoughts to community? You are experienced enough to know what would be needed for a semi-clean patch, even if it's "just for RFC"... > There are a few notes. I'd actually like to know if this approach is correct at > all, it might break on some crazy configurations or such. Define "correct". It may be possible - but what would be the advantage? Which problem does it solve? In which way is it better than the current code? > > 1) what about all the non-ARM architecures and the board specific > > linker scripts? > > - This patch affects only arm926t, obviously to make it proper, every > linkerscript would have to be adjusted > > Which sucks, since there're a lot of them. But it can probably be automated. Actually I doubt it makes sense at all. I envision a situation where some "pluggable" code (say, a standalone application, or some form of loadable module whatever) can add new commands - it would be nice if these would still appear in sorted order, but this cannot be done at compile-time. So please explain which actual problem you are rying to solve. Best regards, Wolfgang Denk
Dear Wolfgang Denk, > Dear Marek Vasut, > > In message <201207282039.34518.marex@denx.de> you wrote: > > > Seems incomplete in several aspects: > > Below the section: > > > > * NOTE * THIS PATCH IS CRAZY > > Then what is actually the purpose of such a posting? Just dumping > unsorted thoughts to community? I'd prefer to get some feedback, you know ... > You are experienced enough to know what would be needed for a > semi-clean patch, even if it's "just for RFC"... If you mean droping the ascii art ... well, yes. But for draft patch, I'd like to actually see further ideas. > > There are a few notes. I'd actually like to know if this approach is > > correct at all, it might break on some crazy configurations or such. > > Define "correct". If there's not some obvious flub in the code. If this kind of abuse of CPP is correct or not. > It may be possible - but what would be the advantage? The list of commands will be already sorted. > Which problem does it solve? Optimization, nothing else. > In which way is it better than the current code? It's a bit faster. > > > 1) what about all the non-ARM architecures and the board specific > > > > > > linker scripts? > > > > - This patch affects only arm926t, obviously to make it proper, every > > > > linkerscript would have to be adjusted > > > > Which sucks, since there're a lot of them. But it can probably be > > automated. > > Actually I doubt it makes sense at all. It actually does ... but not in such a plain context. I did this patch because we want the driver lists sorted. So I did this research and implemented it on the command list. I wanted to gather some feedback on if this actually can be done in such a way or if there'll be problems with toolchains maybe. Or any other issues. > I envision a situation where some "pluggable" code (say, a standalone > application, or some form of loadable module whatever) can add new > commands - it would be nice if these would still appear in sorted > order, but this cannot be done at compile-time. Certainly ... but we can keep a separate runtime table for these added commands. > So please explain which actual problem you are rying to solve. > > Best regards, > > Wolfgang Denk Best regards, Marek Vasut
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index e49ca0c..c39193b 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -50,7 +50,7 @@ SECTIONS . = .; __u_boot_cmd_start = .; - .u_boot_cmd : { *(.u_boot_cmd) } + .u_boot_cmd : { *(SORT(.u_boot_cmd.*)) } __u_boot_cmd_end = .; . = ALIGN(4); diff --git a/common/cmd_help.c b/common/cmd_help.c index 8c8178e..5d778f5 100644 --- a/common/cmd_help.c +++ b/common/cmd_help.c @@ -41,7 +41,7 @@ U_BOOT_CMD( ); /* This does not use the U_BOOT_CMD macro as ? can't be used in symbol names */ -cmd_tbl_t __u_boot_cmd_question_mark Struct_Section = { +cmd_tbl_t __u_boot_cmd_question_mark Struct_Section(?) = { "?", CONFIG_SYS_MAXARGS, 1, do_help, "alias for 'help'", #ifdef CONFIG_SYS_LONGHELP diff --git a/include/command.h b/include/command.h index 6e1bdc2..42b4c6a 100644 --- a/include/command.h +++ b/include/command.h @@ -149,8 +149,11 @@ int cmd_process(int flag, int argc, char * const argv[], #define CMD_FLAG_REPEAT 0x0001 /* repeat last command */ #define CMD_FLAG_BOOTD 0x0002 /* command is from bootd */ -#define Struct_Section __attribute__((unused, section(".u_boot_cmd"), \ - aligned(4))) +#define __sectstr(__cmd,__name) .__cmd.__name +#define sectstr(type, __name) __stringify(__sectstr(type, __name)) + +#define Struct_Section(__name) \ + __attribute__((unused, section(sectstr(u_boot_cmd, __name)), aligned(4))) #ifdef CONFIG_AUTO_COMPLETE # define _CMD_COMPLETE(x) x, @@ -170,7 +173,7 @@ int cmd_process(int flag, int argc, char * const argv[], U_BOOT_CMD_MKENT_COMPLETE(name,maxargs,rep,cmd,usage,help,NULL) #define U_BOOT_CMD_COMPLETE(name,maxargs,rep,cmd,usage,help,comp) \ - cmd_tbl_t __u_boot_cmd_##name Struct_Section = \ + cmd_tbl_t __u_boot_cmd_##name Struct_Section(name) = \ U_BOOT_CMD_MKENT_COMPLETE(name,maxargs,rep,cmd,usage,help,comp) #define U_BOOT_CMD(name,maxargs,rep,cmd,usage,help) \
This shall eliminate the need for bubblesorting of commands at runtime. Every command definition structure is now put into it's own subsection of section .u_boot_cmd, that is .u_boot_cmd.<name> . These are then put into .u_boot_cmd by linker and lastly, linker uses SORT() over these subsections to make proper order on them. This shall eliminate some runtime overhead. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Wolfgang Denk <wd@denx.de> Cc: Mike Frysinger <vapier@gentoo.org> --- arch/arm/cpu/u-boot.lds | 2 +- common/cmd_help.c | 2 +- include/command.h | 9 ++++++--- 3 files changed, 8 insertions(+), 5 deletions(-) ******** * NOTE * THIS PATCH IS CRAZY ******** Further notes: - This is only compile-tested with gcc-4.7 (debian 4.7.1-5, binutils 2.22) - This patch affects only arm926t, obviously to make it proper, every linkerscript would have to be adjusted - I'm not sure at all the macro logic is correct, please check - Can this crash on *BSD or with older linker/cpp? - Please don't rip me limb to limb ;-)