Re: [PATCH] kernel/module: add name size info to pr_debug() calls
On Tue, Jun 30, 2020 at 3:37 AM Jessica Yu wrote:
>
> +++ Jim Cromie [11/06/20 08:20 -0600]:
> >when booted with arg: module.dyndbg=+p
> >dmesg gets volumes of info about loaded modules.
> >This adds module & symbol names, and sizes where pertinent.
> >Now I can know which module's info Im looking at.
>
> Hi,
>
> Could you please fix the changelog formatting according to
> Documentation/process/submitting-patches.rst? More specifically,
> complete sentences, line wrapped at 75 columns, and a Signed-off-by:
> line at the end. There are plenty of examples if you look through the
> lkml mailing list.
>
ack. less casual next time. and with -s
> >---
> > kernel/module.c | 14 +++---
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> >diff --git a/kernel/module.c b/kernel/module.c
> >index e8a198588f26..d871d9cee9eb 100644
> >--- a/kernel/module.c
> >+++ b/kernel/module.c
> >@@ -2294,8 +2294,8 @@ static int simplify_symbols(struct module *mod, const
> >struct load_info *info)
> >
> > case SHN_ABS:
> > /* Don't need to do anything */
> >- pr_debug("Absolute symbol: 0x%08lx\n",
> >- (long)sym[i].st_value);
> >+ pr_debug("Absolute symbol: 0x%08lx %s\n",
> >+ (long)sym[i].st_value, name);
>
> I would prefer "Absolute symbol %s:" rather than putting the symbol
> name at the end.
i chose this for a more tabular appearance,
names vary in length, so value afterwards looks cluttered.
plus its more consistent with preceding messages
[3.458007] 0xc0023d40 0x0030 .bss
[3.458008] 0xc0028000 0x1158 .symtab
[3.458010] 0xc0029158 0x0af5 .strtab
[3.458021] Absolute symbol: 0x rapl.mod.c
[3.458022] Absolute symbol: 0x rapl.c
>
> > break;
> >
> > case SHN_LIVEPATCH:
> >@@ -2409,7 +2409,7 @@ static void layout_sections(struct module *mod, struct
> >load_info *info)
> > for (i = 0; i < info->hdr->e_shnum; i++)
> > info->sechdrs[i].sh_entsize = ~0UL;
> >
> >- pr_debug("Core section allocation order:\n");
> >+ pr_debug("Core section allocation order for: %s\n", mod->name);
>
> I would slightly prefer "Core section allocation order for %s:", but
> it's a matter of taste.
>
done everywhere you noted.
> > for (m = 0; m < ARRAY_SIZE(masks); ++m) {
> > for (i = 0; i < info->hdr->e_shnum; ++i) {
> > Elf_Shdr *s = >sechdrs[i];
> >@@ -2442,7 +2442,7 @@ static void layout_sections(struct module *mod, struct
> >load_info *info)
> > }
> > }
> >
> >- pr_debug("Init section allocation order:\n");
> >+ pr_debug("Init section allocation order for: %s\n", mod->name);
>
> Same here.
>
> > for (m = 0; m < ARRAY_SIZE(masks); ++m) {
> > for (i = 0; i < info->hdr->e_shnum; ++i) {
> > Elf_Shdr *s = >sechdrs[i];
> >@@ -3276,7 +3276,7 @@ static int move_module(struct module *mod, struct
> >load_info *info)
> > mod->init_layout.base = NULL;
> >
> > /* Transfer each section which specifies SHF_ALLOC */
> >- pr_debug("final section addresses:\n");
> >+ pr_debug("final section addresses for: %s\n", mod->name);
>
> Let's capitalize the "f" in "final section addresses" to be consistent
> with the other two above.
>
> > for (i = 0; i < info->hdr->e_shnum; i++) {
> > void *dest;
> > Elf_Shdr *shdr = >sechdrs[i];
> >@@ -3294,8 +3294,8 @@ static int move_module(struct module *mod, struct
> >load_info *info)
> > memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
> > /* Update sh_addr to point to copy in image. */
> > shdr->sh_addr = (unsigned long)dest;
> >- pr_debug("\t0x%lx %s\n",
> >- (long)shdr->sh_addr, info->secstrings +
> >shdr->sh_name);
> >+ pr_debug("\t0x%lx 0x%.8lx %s\n", (long)shdr->sh_addr,
> >+ (long)shdr->sh_size, info->secstrings +
> >shdr->sh_name);
>
> Any reason why you added sh_size here? Is it really needed? You can
> usually deduce how much space a section is taking up in a module by
> looking at the final section address printout. Plus the elf section
> size is not entirely accurate here as each module section is aligned
> to page boundaries when the module loader allocates memory for each
> section. I would prefer to just leave it out.
>
mostly I added it cuz it was available.
I do not know when it might be useful, but seeing it can help decide that.
Having the data there makes your deduction possible,
without seeing the data, it wouldnt occur to me that there are "inaccuracies".
And seeing it in hex form might illuminate the page alignment too.
Also, this is in move-module, which I havent seen in action AFAIK
I'll send v2 shortly, as above, unless
Re: [PATCH] kernel/module: add name size info to pr_debug() calls
+++ Jim Cromie [11/06/20 08:20 -0600]:
when booted with arg: module.dyndbg=+p
dmesg gets volumes of info about loaded modules.
This adds module & symbol names, and sizes where pertinent.
Now I can know which module's info Im looking at.
Hi,
Could you please fix the changelog formatting according to
Documentation/process/submitting-patches.rst? More specifically,
complete sentences, line wrapped at 75 columns, and a Signed-off-by:
line at the end. There are plenty of examples if you look through the
lkml mailing list.
---
kernel/module.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index e8a198588f26..d871d9cee9eb 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2294,8 +2294,8 @@ static int simplify_symbols(struct module *mod, const
struct load_info *info)
case SHN_ABS:
/* Don't need to do anything */
- pr_debug("Absolute symbol: 0x%08lx\n",
- (long)sym[i].st_value);
+ pr_debug("Absolute symbol: 0x%08lx %s\n",
+(long)sym[i].st_value, name);
I would prefer "Absolute symbol %s:" rather than putting the symbol
name at the end.
break;
case SHN_LIVEPATCH:
@@ -2409,7 +2409,7 @@ static void layout_sections(struct module *mod, struct
load_info *info)
for (i = 0; i < info->hdr->e_shnum; i++)
info->sechdrs[i].sh_entsize = ~0UL;
- pr_debug("Core section allocation order:\n");
+ pr_debug("Core section allocation order for: %s\n", mod->name);
I would slightly prefer "Core section allocation order for %s:", but
it's a matter of taste.
for (m = 0; m < ARRAY_SIZE(masks); ++m) {
for (i = 0; i < info->hdr->e_shnum; ++i) {
Elf_Shdr *s = >sechdrs[i];
@@ -2442,7 +2442,7 @@ static void layout_sections(struct module *mod, struct
load_info *info)
}
}
- pr_debug("Init section allocation order:\n");
+ pr_debug("Init section allocation order for: %s\n", mod->name);
Same here.
for (m = 0; m < ARRAY_SIZE(masks); ++m) {
for (i = 0; i < info->hdr->e_shnum; ++i) {
Elf_Shdr *s = >sechdrs[i];
@@ -3276,7 +3276,7 @@ static int move_module(struct module *mod, struct
load_info *info)
mod->init_layout.base = NULL;
/* Transfer each section which specifies SHF_ALLOC */
- pr_debug("final section addresses:\n");
+ pr_debug("final section addresses for: %s\n", mod->name);
Let's capitalize the "f" in "final section addresses" to be consistent
with the other two above.
for (i = 0; i < info->hdr->e_shnum; i++) {
void *dest;
Elf_Shdr *shdr = >sechdrs[i];
@@ -3294,8 +3294,8 @@ static int move_module(struct module *mod, struct
load_info *info)
memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
/* Update sh_addr to point to copy in image. */
shdr->sh_addr = (unsigned long)dest;
- pr_debug("\t0x%lx %s\n",
-(long)shdr->sh_addr, info->secstrings + shdr->sh_name);
+ pr_debug("\t0x%lx 0x%.8lx %s\n", (long)shdr->sh_addr,
+(long)shdr->sh_size, info->secstrings + shdr->sh_name);
Any reason why you added sh_size here? Is it really needed? You can
usually deduce how much space a section is taking up in a module by
looking at the final section address printout. Plus the elf section
size is not entirely accurate here as each module section is aligned
to page boundaries when the module loader allocates memory for each
section. I would prefer to just leave it out.
Thanks,
Jessica
[PATCH] kernel/module: add name size info to pr_debug() calls
when booted with arg: module.dyndbg=+p
dmesg gets volumes of info about loaded modules.
This adds module & symbol names, and sizes where pertinent.
Now I can know which module's info Im looking at.
---
kernel/module.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index e8a198588f26..d871d9cee9eb 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2294,8 +2294,8 @@ static int simplify_symbols(struct module *mod, const
struct load_info *info)
case SHN_ABS:
/* Don't need to do anything */
- pr_debug("Absolute symbol: 0x%08lx\n",
- (long)sym[i].st_value);
+ pr_debug("Absolute symbol: 0x%08lx %s\n",
+(long)sym[i].st_value, name);
break;
case SHN_LIVEPATCH:
@@ -2409,7 +2409,7 @@ static void layout_sections(struct module *mod, struct
load_info *info)
for (i = 0; i < info->hdr->e_shnum; i++)
info->sechdrs[i].sh_entsize = ~0UL;
- pr_debug("Core section allocation order:\n");
+ pr_debug("Core section allocation order for: %s\n", mod->name);
for (m = 0; m < ARRAY_SIZE(masks); ++m) {
for (i = 0; i < info->hdr->e_shnum; ++i) {
Elf_Shdr *s = >sechdrs[i];
@@ -2442,7 +2442,7 @@ static void layout_sections(struct module *mod, struct
load_info *info)
}
}
- pr_debug("Init section allocation order:\n");
+ pr_debug("Init section allocation order for: %s\n", mod->name);
for (m = 0; m < ARRAY_SIZE(masks); ++m) {
for (i = 0; i < info->hdr->e_shnum; ++i) {
Elf_Shdr *s = >sechdrs[i];
@@ -3276,7 +3276,7 @@ static int move_module(struct module *mod, struct
load_info *info)
mod->init_layout.base = NULL;
/* Transfer each section which specifies SHF_ALLOC */
- pr_debug("final section addresses:\n");
+ pr_debug("final section addresses for: %s\n", mod->name);
for (i = 0; i < info->hdr->e_shnum; i++) {
void *dest;
Elf_Shdr *shdr = >sechdrs[i];
@@ -3294,8 +3294,8 @@ static int move_module(struct module *mod, struct
load_info *info)
memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
/* Update sh_addr to point to copy in image. */
shdr->sh_addr = (unsigned long)dest;
- pr_debug("\t0x%lx %s\n",
-(long)shdr->sh_addr, info->secstrings + shdr->sh_name);
+ pr_debug("\t0x%lx 0x%.8lx %s\n", (long)shdr->sh_addr,
+(long)shdr->sh_size, info->secstrings + shdr->sh_name);
}
return 0;
--
2.26.2
