Re: [PATCH v2] samples/bpf: Add more instructions to build dependencies and, configuration in README.rst

[Stanislav Fomichev <sdf@google.com>]

On 07/09, Khalid Masum wrote:
> Hi,
> 
> On Sun, Jul 9, 2023 at 8:38 PM Anh Tuan Phan <tuananhlfc@gmail.com> wrote:
> >
> > Hi Stanislav,
> >
> > I have updated the Documentation according to your suggestion. Please
> > see it in the below patch. Thanks!
> >
> > On 7/7/23 23:57, Stanislav Fomichev wrote:
> > > On 07/07, Anh Tuan Phan wrote:
> > >>
> > >>
> > >> On 7/7/23 01:16, Stanislav Fomichev wrote:
> > >>> On 07/06, Anh Tuan Phan wrote:
> > >>>> Update the Documentation to mention that some samples require pahole
> > >>>> v1.16 and kernel built with CONFIG_DEBUG_INFO_BTF=y
> > >>>>
> > >>>> Signed-off-by: Anh Tuan Phan <tuananhlfc@gmail.com>
> > >>>> ---
> > >>>>  samples/bpf/README.rst | 7 +++++++
> > >>>>  1 file changed, 7 insertions(+)
> > >>>>
> > >>>> diff --git a/samples/bpf/README.rst b/samples/bpf/README.rst
> > >>>> index 57f93edd1957..631592b83d60 100644
> > >>>> --- a/samples/bpf/README.rst
> > >>>> +++ b/samples/bpf/README.rst
> > >>>> @@ -14,6 +14,9 @@ Compiling requires having installed:
> > >>>>  Note that LLVM's tool 'llc' must support target 'bpf', list version
> > >>>>  and supported targets with command: ``llc --version``
> > >>>>
> > >>>> +Some samples require pahole version 1.16 as a dependency. See
> > >>>> +https://docs.kernel.org/bpf/bpf_devel_QA.html for reference.
> > >>>> +
> > >>>
> > >>> Any reason no to add pahole 1.16 to this section above?
> > >>>> Compiling requires having installed:
> > >>>  * clang >= version 3.4.0
> > >>>  * llvm >= version 3.7.1
> > >>>  * pahole >= version 1.16
> > >>>
> > >>> Although clang 3.4 probably won't get you anywhere these days. The
> > >>> whole README seems a bit outdated :-)
> > >>>
> > >>
> > >> Put pahole requirement as your idea is better, thanks for suggestion.
> > >> Will update it and clang version as well. For clang version, I think I
> > >> can update min version as 11.0.0 (reference from
> > >> https://www.kernel.org/doc/html/next/process/changes.html). Do you see
> > >> any other potential outdated things in this document? I follow the above
> > >> steps and it help me compile the sample code successfully.
> > >
> > > Maybe we can reference that doc instead here? Otherwise that copy-pasted
> > > 11.0.0 will also get old. Just mention here that we need
> > > clang/llvm/pahole to compile the samples and for specific versions
> > > put a link to process/changes.rst
> > >
> > >>>>  Clean and configuration
> > >>>>  -----------------------
> > >>>>
> > >>>> @@ -28,6 +31,10 @@ Configure kernel, defconfig for instance::
> > >>>>
> > >>>>   make defconfig
> > >>>>
> > >>>> +Some samples require support for BPF Type Format (BTF). To enable it,
> > >>>> open the
> > >>>> +generated config file, or use menuconfig (by "make menuconfig") to
> > >>>> enable the
> > >>>> +following configs: CONFIG_BPF_SYSCALL and CONFIG_DEBUG_INFO_BTF.
> > >>>> +
> > >>>
> > >>> This is usually enabled by default, so why special case it here?
> > >>> Maybe, if you want some hints about the config, we should add
> > >>> a reference to tools/testing/selftests/bpf/config ?
> > >>>
> > >>
> > >> The config CONFIG_DEBUG_INFO_BTF is disabled for some distros at least
> > >> for mine. I ran "make defconfig" and it's not enabled by default so I
> > >> think it worth to mention it here to help novice get started. I'll
> > >> update it to reference to tools/testing/selftests/bpf/config .
> > >>
> > >>>>  Kernel headers
> > >>>>  --------------
> > >>>>
> > >>>> --
> > >>>> 2.34.1
> >
> > Signed-off-by: Anh Tuan Phan <tuananhlfc@gmail.com>
> > ---
> >
> > Change from the original patch:
> >
> > - Move pahole to the list installed requirements
> > - Remove minimal version and link the related doc
> > - Add a reference of kernel configuration
> >
> >  samples/bpf/README.rst | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/samples/bpf/README.rst b/samples/bpf/README.rst
> > index 57f93edd1957..e18500753ba5 100644
> > --- a/samples/bpf/README.rst
> > +++ b/samples/bpf/README.rst
> > @@ -8,9 +8,12 @@ Build dependencies
> >  ==================
> >
> >  Compiling requires having installed:
> > - * clang >= version 3.4.0
> > - * llvm >= version 3.7.1
> > + * clang
> > + * llvm
> > + * pahole
> >
> > +The minimal version of the above software is referenced in
> > +https://www.kernel.org/doc/html/next/process/changes.html.
> 
> I think it is better to not use docs from linux-next as it keeps changing
> too frequently. How about using the latest documentation's link instead? :)
> 
> https://www.kernel.org/doc/html/latest/process/changes.html

+1

We should put Documentation/process/changes.rst here (or whatever
the correct path). The tooling that generates html from rst will
put a proper link.