Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200611142009.1098058-1-jim.cromie@gmail.com> <20200630093754.GA13271@linux-8ccs.fritz.box>
In-Reply-To: <20200630093754.GA13271@linux-8ccs.fritz.box>
From:   jim.cromie@gmail.com
Date:   Tue, 30 Jun 2020 07:34:51 -0600
Message-ID: <CAJfuBxz5YzZfko_M5ap3QW-7ZJsHJDqzEqfN=GZVtDz4W2edLQ@mail.gmail.com>
Subject: Re: [PATCH] kernel/module: add name size info to pr_debug() calls
To:     Jessica Yu <jeyu@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jun 30, 2020 at 3:37 AM Jessica Yu <jeyu@kernel.org> 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] 0xffffffffc0023d40 0x00000030 .bss
[    3.458008] 0xffffffffc0028000 0x00001158 .symtab
[    3.458010] 0xffffffffc0029158 0x00000af5 .strtab
[    3.458021] Absolute symbol: 0x00000000 rapl.mod.c
[    3.458022] Absolute symbol: 0x00000000 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 = &info->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 = &info->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 = &info->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 you want a tweak.

> Thanks,
>
> Jessica