Re: Error on loading some network Kernel modules

[Vincent Chen <vincent.chen@sifive.com>]

On Mon, Feb 10, 2020 at 2:37 PM Alex Ghiti <alex@ghiti.fr> wrote:
>
> Hi Vincent,
>
> On 2/7/20 9:51 AM, Vincent Chen wrote:
> > On Fri, Feb 7, 2020 at 10:39 PM Vincent Chen <vincent.chen@sifive.com> wrote:
> >>
> >>>> For point1, we don't need a dedicated region as long as we are allocating
> >>>> modules from VMALLOC area. Let's avoid another virtual memory region if
> >>>> possible.
> >>>>
> >>>
> >>> Your comments inspire me if we can know the end of the percpu data
> >>> region, we can dynamically calculate the start address in each module
> >>> allocation by "<end of the percpu data region> - 2GB" if needed. I am
> >>> finding a way to derive the <end of the percpu data region>.
> >>>
> >>
> >> In the implementation of the idea, I found that I did have some
> >> misunderstandings about the mechanism of accessing static percpu
> >> symbol. Currently, I think this issue can be solved by modifying the
> >> module_allocate() to the following.
> >> #define MODULE_START max(PFN_ALIGN(&_end - 2 *
> >> SZ_1G), PFN_ALIGN(&_text + 8 * SZ_1K - 2 * SZ_1G))
> >> void *module_alloc(unsigned long size)
> >> {
> >> return __vmalloc_node_range(size, 1, MODULE_START,
> >> VMALLOC_END, GFP_KERNEL,
> >> PAGE_KERNEL_EXEC, 0,
> >> NUMA_NO_NODE,
> >> __builtin_return_address(0));
> >> }
> >> After applying a similar modification above, the kernel can
> >> successfully insert the test module provided by Romain. Any feedback
> >> is welcome. If someone is curious about the reason, I roughly
> >> described it at the end of this mail. I am sorry if someone was misled
> >> by early discussion.
> >>
> >>
> >> As mentioned early, kernel pre-allocates a memory region for each CPU
> >> to place the instances of the percpu data. According to the
> >> declaration type, each memory region can be divided into three chunks.
> >> These three chunks from low memory to high memory are static chunk,
> >> reserved chunk, and dynamic chunk.
> >> a. The static chunk is used for the static percpu symbols declared in the kernel
> >> b. The reserved chunk is used for the static percpu symbols declared
> >> in the kernel module
> >> c. The dynamic chunk is used for all dynamic percpu symbols.
> >>
> >> The order of the percpu symbols in each memory region is the same and
> >> is recorded in a symbol list maintained by the kernel. Therefore, the
> >> address of a specific CPU's percpu symbol is "The address of this
> >> percpu symbol in the symbol list" + "The offset from symbols list to
> >> the specific CPU region".
> >> a. The symbol list is created based on the ".data..percpu" section. In
> >> other words, the start address of this symbol list is the start
> >> address of ".data..percpu". When a dynamic percpu symbol is created or
> >> a kernel module import a static percpu symbol, the kernel will follow
> >> the above chunk rule to add this percpu symbol to its suitable region.
> >> b. When each memory area was created, "The offset from symbols list to
> >> the specific CPU region" has been recorded.
> >>
> >> Back to the issue, according to the rules above, the kernel will
> >> relocate the ".data..percpu" section of the loaded module to the end
> >> of the ".data..percpu" section of the kernel image. The end of the
> >> ".data..percpu" section of the kernel image almost equals _text, and
> >> the current size of the reserved chunk is 8KB. Therefore, the
> >> appropriate range to place the loaded module is [max(PFN_ALIGN(&_end -
> >> 2 *SZ_1G), PFN_ALIGN(&_text + 8 * SZ_1K - 2 * SZ_1G)),  VMALLOC_END].
> >
> > I did not notice that the size of the kernel .text section is
> > impossible below 2 PAGES. Therefore, the modification of module_alloc
> > function can be reduced to the following.
> > void *module_alloc(unsigned long size)
> >   {
> >   return __vmalloc_node_range(size, 1, PFN_ALIGN(&_end - 2 *SZ_1G),
> >   VMALLOC_END, GFP_KERNEL,
> >   PAGE_KERNEL_EXEC, 0,
> >   NUMA_NO_NODE,
> >   __builtin_return_address(0));
> >   }
> > It is the same as Alex's suggestion. Is Alex willing to send this
> > patch to resolve this bug?
> >
>
> You did all the work, please send a patch explaining what you learnt and
> feel free to add a Suggested-By. Anyway, I'll add a Reviewed-by when you
> propose something.
>
> Thank you Vincent for all your research,
>
> Alex

OK, I understood. Thank you and Anup for giving me some suggestions
from different viewpoints. I will add Suggested-By for you all if
possible :)

Vincent