On Wed, Aug 24, 2022 at 11:44 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
>
> On 24/08/2022 00:05, Eyal Birger wrote:
> > Hi Quentin,
> >
> > On Tue, Aug 23, 2022 at 11:49 PM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
> >>
> >> On Fri, 19 Aug 2022 at 10:13, Eyal Birger <eyal.birger@xxxxxxxxx> wrote:
> >>>
> >>> The helper value is ABI as defined by enum bpf_func_id.
> >>> As bpf_helper_defs.h is used for the userpace part, it must be consistent
> >>> with this enum.
> >>>
> >>> Before this change, the enumerated value was derived from the comment
> >>> order, which assumes comments are always appended, however, there doesn't
> >>> seem to be an enforcement anywhere for maintaining a strict order.
> >>>
> >>> When adding new helpers it is very puzzling when the userspace application
> >>> breaks in weird places if the comment is inserted instead of appended -
> >>> because the generated helper ABI is incorrect and shifted.
> >>>
> >>> This commit attempts to ease this by always using bpf_func_id order as
> >>> the helper value.
> >>>
> >>> Signed-off-by: Eyal Birger <eyal.birger@xxxxxxxxx>
> >>> ---
> >>> scripts/bpf_doc.py | 19 ++++++++++---------
> >>> 1 file changed, 10 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> >>> index dfb260de17a8..7797aa032eca 100755
> >>> --- a/scripts/bpf_doc.py
> >>> +++ b/scripts/bpf_doc.py
> >>> @@ -88,7 +88,7 @@ class HeaderParser(object):
> >>> self.helpers = []
> >>> self.commands = []
> >>> self.desc_unique_helpers = set()
> >>> - self.define_unique_helpers = []
> >>> + self.define_unique_helpers = {}
> >>> self.desc_syscalls = []
> >>> self.enum_syscalls = []
> >>>
> >>> @@ -245,24 +245,24 @@ class HeaderParser(object):
> >>> break
> >>>
> >>> def parse_define_helpers(self):
> >>> - # Parse the number of FN(...) in #define __BPF_FUNC_MAPPER to compare
> >>> - # later with the number of unique function names present in description.
> >>> + # Parse FN(...) in #define __BPF_FUNC_MAPPER to compare later with the
> >>> + # number of unique function names present in description and use the
> >>> + # correct enumeration value.
> >>> # Note: seek_to(..) discards the first line below the target search text,
> >>> # resulting in FN(unspec) being skipped and not added to self.define_unique_helpers.
> >>> self.seek_to('#define __BPF_FUNC_MAPPER(FN)',
> >>> 'Could not find start of eBPF helper definition list')
> >>> # Searches for either one or more FN(\w+) defines or a backslash for newline
> >>> - p = re.compile('\s*(FN\(\w+\))+|\\\\')
> >>> - fn_defines_str = ''
> >>> + p = re.compile('\s*FN\((\w+)\)+|\\\\')
> >>
> >> Nit: I think the second '+' should be removed, I don't think you can
> >> have consecutive "FN(...)" without at least a comma. But you didn't
> >> add and it is harmless, so it can be a follow-up or wait until a
> >> future clean-up.
> >>
> >
> > Sure. I can remove that.
> >
> >>> + i = 1 # 'unspec' is skipped as mentioned above
> >>> while True:
> >>> capture = p.match(self.line)
> >>> if capture:
> >>> - fn_defines_str += self.line
> >>> + self.define_unique_helpers[capture.expand(r'bpf_\1')] = i
> >>> + i += 1
> >>> else:
> >>> break
> >>> self.line = self.reader.readline()
> >>> - # Find the number of occurences of FN(\w+)
> >>> - self.define_unique_helpers = re.findall('FN\(\w+\)', fn_defines_str)
> >>>
> >>> def run(self):
> >>> self.parse_desc_syscall()
> >>> @@ -573,6 +573,7 @@ class PrinterHelpers(Printer):
> >>> def __init__(self, parser):
> >>> self.elements = parser.helpers
> >>> self.elem_number_check(parser.desc_unique_helpers, parser.define_unique_helpers, 'helper', '__BPF_FUNC_MAPPER')
> >>> + self.define_unique_helpers = parser.define_unique_helpers
> >>>
> >>> type_fwds = [
> >>> 'struct bpf_fib_lookup',
> >>> @@ -761,7 +762,7 @@ class PrinterHelpers(Printer):
> >>> comma = ', '
> >>> print(one_arg, end='')
> >>>
> >>> - print(') = (void *) %d;' % len(self.seen_helpers))
> >>> + print(') = (void *) %d;' % self.define_unique_helpers[proto['name']])
> >>> print('')
> >>
> >> The code seems correct and should make the script more robust, and I
> >> checked that the man page and header file are generated identically.
> >>
> >> Reviewed-by: Quentin Monnet <quentin@xxxxxxxxxxxxx>
> >
> > Thanks for the review.
> >
> >>
> >> However, I would recommend against inserting the description of new
> >> helpers in the middle of the current documentation. Having the helpers
> >> listed in order of creation is maybe not ideal, but at least they are
> >> ordered, and the list remains consistent with the items of enum
> >> bpf_func_id. I'm not opposed to reworking the list to have them
> >> displayed in a more logical order, but in that case I think we should
> >> reorganise the whole list, not just start inserting new descriptions
> >> in the middle.
> >>
> >
> > I understand. Personally I don't mind the fact that they're ordered
> > relative to their enum value, only that this is implicitly enforced.
> >
> > Since we know both the enum value and the comment value, would it be
> > acceptible to add an assertion here so that at least wrongful insertions
> > break the file generation instead of skewing the values?
>
> As I understand it, your patch already solves the issue by making sure
> we use the correct value even if the descriptions do not come in the
> same order as the enum items. Do you mean adding an additional check to
> enforce that the description items are in the same order, in addition to
> your patch?
>
Yes. The patch would fetch the value from the enum, but also assert that
the enum value is the same as the comment order value.
> I don't have a strong opinion, if anything I'd say it's probably not the
> role of this script to ensure that the description items are in a
> particular order (provided your patch is applied and the values are
> correct). I'm not sure we want to strongly enforce the order; I would
> definitely recommend against inserting new items in the middle, but at
> the same time I wouldn't oppose some reorganisation into logical
> sections. On the other hand, it's probably cleaner to have the
> definitions in the generated header file listed in the correct order of
> the enum values, so why not having the assertion for now, and lifting it
> if we ever want to rework the order.
>
I agree. I think for now we can have the assertion, and lift it when
a reorganization of the comments is done. I'll send a v2.
> Quentin
