Re: [PATCH 3/6] pgo: modules Add module profile data export machinery.

[jarmo . tiitto]

Kirjoitit tiistaina 1. kesäkuuta 2021 20.27.01 EEST:
> On Tue, Jun 1, 2021 at 6:26 AM <jarmo.tiitto@xxxxxxxxx> wrote:
> > Kirjoitit tiistaina 1. kesäkuuta 2021 11.33.48 EEST:
> > > On Mon, May 31, 2021 at 12:09 PM Nathan Chancellor <nathan@xxxxxxxxxx>
> > 
> > wrote:
> > > > On Fri, May 28, 2021 at 11:08:21PM +0300, Jarmo Tiitto wrote:
> > > > > PGO profile data is exported from the kernel by
> > > > > creating debugfs files: pgo/<module>.profraw for each module.
> > > > 
> > > > Again, I do not really have many comments on the actual code as I am
> > 
> > not
> > 
> > > > super familiar with it.
> > > > 
> > > > However, fs_mod.c duplicates a lot of the functions in fs.c, which
> > 
> > makes
> > 
> > > > maintaining this code even more difficult, especially against LLVM PGO
> > > > profile data format changes. I just want to make sure I understand
> > 
> > this:
> > > > does PGO currently not work with modules? Or does this patch series
> > 
> > just
> > 
> > > > make it so that each module has its own .profraw file so individual
> > > > modules can be optimized?
> > > > 
> > > > If it is the latter, what is the point? Why would you want to optimize
> > > > just a module and not the entire kernel, if you already have to go
> > > > through the profiling steps?
> > > > 
> > > > If it is the former, there has to be a better way to share more of the
> > > > machinery between fs.c and fs_mod.c than basically duplicating
> > > > everything because there are some parameters and logic that have to
> > > > change. I do not have a ton of time to outline exactly what that might
> > > > look like but for example, prf_fill_header and prf_module_fill_header
> > > > are basically the same thing aside from the mod parameter and the
> > > > prf_..._count() calls. It seems like if mod was NULL, you would call
> > 
> > the
> > 
> > > > vmlinux versions of the functions.
> > > 
> > > Functions definitely shouldn't be duplicated with only minor changes.
> > > We should determine a way to combine them.
> > > 
> > > As for whether the original PGO patch supports profiling in modules,
> > > the answer is "it depends". :-) I believe that clang inserts profiling
> > > hooks into all code that's compiled with the "-fprofile..." flags.
> > > This would include the modules of course. In GCOV, it's possible to
> > > retrieve profiling information for a single file. Jarmo, is that the
> > > intention of your patches?
> > > 
> > > -bw
> > 
> > Thanks for replying Nathan and Bill!
> > 
> > My original motivation for writing this patch was the realization that no
> > profile data could be obtained from modules using the original patch only.
> > 
> > My insight when testing the original patch was that the compiler indeed
> > does
> > instrument+profile all code, including any loaded modules. But this is
> > where
> > the current instrument.c falls short:
> > The allocate_node() function may consume nodes from __llvm_prf_vnds_start
> > that are actually in a module and not in vmlinux.
> > So llvm_prf_data *p argument may point into an module section, and not
> > into
> > __llvm_prf_data_start range.
> > 
> > This can result in early exhaustion of of nodes for vmlinux and less
> > accurate
> > profile data. I think this is actually a bug in the original patch.
> > 
> > So no, the PGO does not currently work with modules. And it somewhat works
> > for vmlinux.
> 
> Hi Jarmo,
> Thanks for the series! Would you mind including the above in a cover letter
> in a v2? (You can use --cover-letter command line arg to `git format-patch`
> to generate a stub).  The please explicitly cc
> Sami Tolvanen <samitolvanen@xxxxxxxxxx>
> Bill Wendling <morbo@xxxxxxxxxx>
> on the series? Finally, please specify the cover letter and all patch files
> to git send-email in one command, so that the individual patch files are
> linked on lore.kernel.org. This makes it significantly easier to review and
> test.
> 

Hello,
 
Yeah, I realized afterwards that I screwed up at the git send-mail/message 
threading task. Sorry about that. I will correct all of it in my next v2 
patch. Make mistakes, and learn new things. 😃

I will post new v2 patch once I'm done writing and testing it. Based on the 
feed back here I will try keep it simple and unify the vmlinux + modules code 
such that there is no fs_mod.c source any more nor necessary code duplication.

Basically it will be an rewrite on my part but I'm just excited to do it.
I feel this first attempt was more like of RFC/prototype such that I could get 
in contact with you guys.

Just one question about copyrights: do I need to add my statement to the 
sources, if yes, then how should I proceed ?

Regards,
Jarmo Tiitto.

> > My code confines the llvm_prf_value_node reservation to vmlinux or module
> > in
> > instrument.c based on where the llvm_prf_data *p argument points into.
> > 
> > My next intention with the patch is that vmlinux and each loadable module
> > exports their own, separate profile data file in debugfs/pgo/ like the
> > vmlinux
> > already does.
> > So no file level information like in gcov. Only per whole "module.ko" and
> > the
> > vmlinux binary.
> > This way you can inspect what each module is doing individually using
> > "llvm-
> > profdata show mod.profraw"
> > 
> > For PGO final build I intended combining the profile data from vmlinux and
> > all
> > modules with "llvm-profdata merge" into single profile for optimized
> > build.
> > 
> > I agree fully that the current code duplication is bad.
> > Maybe I should pass in the mod->prf_xxx sections in a struct to the
> > serializing functions?
> > In that way, the struct can be initialized from either module or the
> > vmlinux
> > sections and all serializing code can share the same code.
> > 
> > Either way, thanks to your questions and info I can try an write better
> > version.😃
> > 
> > I have learned a lot, thanks.
> > -Jarmo Tiitto
> >