netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/4] bpf docs and comments typo and formatting fixes
@ 2019-02-28 18:45 Andrii Nakryiko
  2019-02-28 18:45 ` [PATCH bpf-next 1/4] docs/btf: fix typos, improve wording Andrii Nakryiko
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2019-02-28 18:45 UTC (permalink / raw)
  To: andrii.nakryiko, ast, yhs, netdev, bpf, daniel; +Cc: Andrii Nakryiko

A bunch of BPF-related typo, formatting, and documentation fixes.

Andrii Nakryiko (4):
  docs/btf: fix typos, improve wording
  docs/btf: reflow text to fill up to 78 characters
  bpf: fix formatting, typos, reflow comments in syscall.c, verifier.c
  docs/bpf: minor fixes

 Documentation/bpf/bpf_design_QA.rst |  24 +--
 Documentation/bpf/btf.rst           | 316 +++++++++++++---------------
 Documentation/networking/filter.txt |   2 +-
 kernel/bpf/syscall.c                |   5 +-
 kernel/bpf/verifier.c               | 169 ++++++++-------
 5 files changed, 247 insertions(+), 269 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH bpf-next 1/4] docs/btf: fix typos, improve wording
  2019-02-28 18:45 [PATCH bpf-next 0/4] bpf docs and comments typo and formatting fixes Andrii Nakryiko
@ 2019-02-28 18:45 ` Andrii Nakryiko
  2019-02-28 23:47   ` Yonghong Song
  2019-02-28 18:45 ` [PATCH bpf-next 2/4] docs/btf: reflow text to fill up to 78 characters Andrii Nakryiko
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2019-02-28 18:45 UTC (permalink / raw)
  To: andrii.nakryiko, ast, yhs, netdev, bpf, daniel; +Cc: Andrii Nakryiko

Fix various typos, some of the formatting and wording for
Documentation/btf.rst.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 Documentation/bpf/btf.rst | 108 +++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 55 deletions(-)

diff --git a/Documentation/bpf/btf.rst b/Documentation/bpf/btf.rst
index 1d434c3a268d..1d761f1c5b2b 100644
--- a/Documentation/bpf/btf.rst
+++ b/Documentation/bpf/btf.rst
@@ -5,7 +5,7 @@ BPF Type Format (BTF)
 1. Introduction
 ***************
 
-BTF (BPF Type Format) is the meta data format which
+BTF (BPF Type Format) is the metadata format which
 encodes the debug info related to BPF program/map.
 The name BTF was used initially to describe
 data types. The BTF was later extended to include
@@ -40,8 +40,8 @@ details in :ref:`BTF_Type_String`.
 2. BTF Type and String Encoding
 *******************************
 
-The file ``include/uapi/linux/btf.h`` provides high
-level definition on how types/strings are encoded.
+The file ``include/uapi/linux/btf.h`` provides high-level
+definition of how types/strings are encoded.
 
 The beginning of data blob must be::
 
@@ -59,23 +59,23 @@ The beginning of data blob must be::
     };
 
 The magic is ``0xeB9F``, which has different encoding for big and little
-endian system, and can be used to test whether BTF is generated for
-big or little endian target.
-The btf_header is designed to be extensible with hdr_len equal to
-``sizeof(struct btf_header)`` when the data blob is generated.
+endian systems, and can be used to test whether BTF is generated for
+big- or little-endian target.
+The ``btf_header`` is designed to be extensible with ``hdr_len`` equal to
+``sizeof(struct btf_header)`` when a data blob is generated.
 
 2.1 String Encoding
 ===================
 
 The first string in the string section must be a null string.
-The rest of string table is a concatenation of other null-treminated
+The rest of string table is a concatenation of other null-terminated
 strings.
 
 2.2 Type Encoding
 =================
 
 The type id ``0`` is reserved for ``void`` type.
-The type section is parsed sequentially and the type id is assigned to
+The type section is parsed sequentially and type id is assigned to
 each recognized type starting from id ``1``.
 Currently, the following types are supported::
 
@@ -122,9 +122,9 @@ Each type contains the following common data::
         };
     };
 
-For certain kinds, the common data are followed by kind specific data.
-The ``name_off`` in ``struct btf_type`` specifies the offset in the string table.
-The following details encoding of each kind.
+For certain kinds, the common data are followed by kind-specific data.
+The ``name_off`` in ``struct btf_type`` specifies the offset in the string
+table. The following sections detail encoding of each kind.
 
 2.2.1 BTF_KIND_INT
 ~~~~~~~~~~~~~~~~~~
@@ -136,7 +136,7 @@ The following details encoding of each kind.
  * ``info.vlen``: 0
  * ``size``: the size of the int type in bytes.
 
-``btf_type`` is followed by a ``u32`` with following bits arrangement::
+``btf_type`` is followed by a ``u32`` with the following bits arrangement::
 
   #define BTF_INT_ENCODING(VAL)   (((VAL) & 0x0f000000) >> 24)
   #define BTF_INT_OFFSET(VAL)     (((VAL  & 0x00ff0000)) >> 16)
@@ -148,7 +148,7 @@ The ``BTF_INT_ENCODING`` has the following attributes::
   #define BTF_INT_CHAR    (1 << 1)
   #define BTF_INT_BOOL    (1 << 2)
 
-The ``BTF_INT_ENCODING()`` provides extra information, signness,
+The ``BTF_INT_ENCODING()`` provides extra information: signedness,
 char, or bool, for the int type. The char and bool encoding
 are mostly useful for pretty print. At most one encoding can
 be specified for the int type.
@@ -161,8 +161,7 @@ The maximum value of ``BTF_INT_BITS()`` is 128.
 
 The ``BTF_INT_OFFSET()`` specifies the starting bit offset to
 calculate values for this int. For example, a bitfield struct
-member has
-
+member has:
  * btf member bit offset 100 from the start of the structure,
  * btf member pointing to an int type,
  * the int type has ``BTF_INT_OFFSET() = 2`` and ``BTF_INT_BITS() = 4``
@@ -179,7 +178,7 @@ access the same bits as the above:
 
 The original intention of ``BTF_INT_OFFSET()`` is to provide
 flexibility of bitfield encoding.
-Currently, both llvm and pahole generates ``BTF_INT_OFFSET() = 0``
+Currently, both llvm and pahole generate ``BTF_INT_OFFSET() = 0``
 for all int types.
 
 2.2.2 BTF_KIND_PTR
@@ -204,7 +203,7 @@ No additional type data follow ``btf_type``.
   * ``info.vlen``: 0
   * ``size/type``: 0, not used
 
-btf_type is followed by one "struct btf_array"::
+``btf_type`` is followed by one ``struct btf_array``::
 
     struct btf_array {
         __u32   type;
@@ -217,27 +216,26 @@ The ``struct btf_array`` encoding:
   * ``index_type``: the index type
   * ``nelems``: the number of elements for this array (``0`` is also allowed).
 
-The ``index_type`` can be any regular int types
-(u8, u16, u32, u64, unsigned __int128).
-The original design of including ``index_type`` follows dwarf
-which has a ``index_type`` for its array type.
+The ``index_type`` can be any regular int type
+(``u8``, ``u16``, ``u32``, ``u64``, ``unsigned __int128``).
+The original design of including ``index_type`` follows DWARF,
+which has an ``index_type`` for its array type.
 Currently in BTF, beyond type verification, the ``index_type`` is not used.
 
 The ``struct btf_array`` allows chaining through element type to represent
-multiple dimensional arrays. For example, ``int a[5][6]``, the following
-type system illustrates the chaining:
+multidimensional arrays. For example, for ``int a[5][6]``, the following
+type information illustrates the chaining:
 
   * [1]: int
   * [2]: array, ``btf_array.type = [1]``, ``btf_array.nelems = 6``
   * [3]: array, ``btf_array.type = [2]``, ``btf_array.nelems = 5``
 
-Currently, both pahole and llvm collapse multiple dimensional array
-into one dimensional array, e.g., ``a[5][6]``, the btf_array.nelems
-equal to ``30``. This is because the original use case is map pretty
-print where the whole array is dumped out so one dimensional array
+Currently, both pahole and llvm collapse multidimensional array
+into one-dimensional array, e.g., for ``a[5][6]``, the ``btf_array.nelems``
+is equal to ``30``. This is because the original use case is map pretty
+print where the whole array is dumped out so one-dimensional array
 is enough. As more BTF usage is explored, pahole and llvm can be
-changed to generate proper chained representation for
-multiple dimensional arrays.
+changed to generate proper chained representation for multidimensional arrays.
 
 2.2.4 BTF_KIND_STRUCT
 ~~~~~~~~~~~~~~~~~~~~~
@@ -382,7 +380,7 @@ No additional type data follow ``btf_type``.
 
 No additional type data follow ``btf_type``.
 
-A BTF_KIND_FUNC defines, not a type, but a subprogram (function) whose
+A BTF_KIND_FUNC defines not a type, but a subprogram (function) whose
 signature is defined by ``type``. The subprogram is thus an instance of
 that type. The BTF_KIND_FUNC may in turn be referenced by a func_info in
 the :ref:`BTF_Ext_Section` (ELF) or in the arguments to
@@ -459,10 +457,10 @@ The workflow typically looks like:
 3.1 BPF_BTF_LOAD
 ================
 
-Load a blob of BTF data into kernel. A blob of data
-described in :ref:`BTF_Type_String`
+Load a blob of BTF data into kernel. A blob of data,
+described in :ref:`BTF_Type_String`,
 can be directly loaded into the kernel.
-A ``btf_fd`` returns to userspace.
+A ``btf_fd`` is returned to a userspace.
 
 3.2 BPF_MAP_CREATE
 ==================
@@ -487,7 +485,7 @@ In libbpf, the map can be defined with extra annotation like below:
 Here, the parameters for macro BPF_ANNOTATE_KV_PAIR are map name,
 key and value types for the map.
 During ELF parsing, libbpf is able to extract key/value type_id's
-and assigned them to BPF_MAP_CREATE attributes automatically.
+and assign them to BPF_MAP_CREATE attributes automatically.
 
 .. _BPF_Prog_Load:
 
@@ -532,7 +530,7 @@ Below are requirements for func_info:
     bpf func boundaries.
 
 Below are requirements for line_info:
-  * the first insn in each func must points to a line_info record.
+  * the first insn in each func must have a line_info record pointing to it.
   * the line_info insn_off is in strictly increasing order.
 
 For line_info, the line number and column number are defined as below:
@@ -544,26 +542,26 @@ For line_info, the line number and column number are defined as below:
 3.4 BPF_{PROG,MAP}_GET_NEXT_ID
 
 In kernel, every loaded program, map or btf has a unique id.
-The id won't change during the life time of the program, map or btf.
+The id won't change during the lifetime of a program, map, or btf.
 
 The bpf syscall command BPF_{PROG,MAP}_GET_NEXT_ID
 returns all id's, one for each command, to user space, for bpf
-program or maps,
-so the inspection tool can inspect all programs and maps.
+program or maps, respectively,
+so an inspection tool can inspect all programs and maps.
 
 3.5 BPF_{PROG,MAP}_GET_FD_BY_ID
 
-The introspection tool cannot use id to get details about program or maps.
-A file descriptor needs to be obtained first for reference counting purpose.
+An introspection tool cannot use id to get details about program or maps.
+A file descriptor needs to be obtained first for reference-counting purpose.
 
 3.6 BPF_OBJ_GET_INFO_BY_FD
 ==========================
 
-Once a program/map fd is acquired, the introspection tool can
+Once a program/map fd is acquired, an introspection tool can
 get the detailed information from kernel about this fd,
-some of which is btf related. For example,
-``bpf_map_info`` returns ``btf_id``, key/value type id.
-``bpf_prog_info`` returns ``btf_id``, func_info and line info
+some of which are BTF-related. For example,
+``bpf_map_info`` returns ``btf_id`` and key/value type ids.
+``bpf_prog_info`` returns ``btf_id``, func_info, and line info
 for translated bpf byte codes, and jited_line_info.
 
 3.7 BPF_BTF_GET_FD_BY_ID
@@ -574,9 +572,9 @@ bpf syscall command BPF_BTF_GET_FD_BY_ID can retrieve a btf fd.
 Then, with command BPF_OBJ_GET_INFO_BY_FD, the btf blob, originally
 loaded into the kernel with BPF_BTF_LOAD, can be retrieved.
 
-With the btf blob, ``bpf_map_info`` and ``bpf_prog_info``, the introspection
+With the btf blob, ``bpf_map_info``, and ``bpf_prog_info``, an introspection
 tool has full btf knowledge and is able to pretty print map key/values,
-dump func signatures, dump line info along with byte/jit codes.
+dump func signatures and line info, along with byte/jit codes.
 
 4. ELF File Format Interface
 ****************************
@@ -625,8 +623,8 @@ The func_info is organized as below.::
      ...
 
 ``func_info_rec_size`` specifies the size of ``bpf_func_info`` structure
-when .BTF.ext is generated. btf_ext_info_sec, defined below, is
-the func_info for each specific ELF section.::
+when .BTF.ext is generated. ``btf_ext_info_sec``, defined below, is
+a collection of func_info for each specific ELF section.::
 
      struct btf_ext_info_sec {
         __u32   sec_name_off; /* offset to section name */
@@ -661,7 +659,7 @@ from the beginning of section (``btf_ext_info_sec->sec_name_off``).
 
 With BTF, the map key/value can be printed based on fields rather than
 simply raw bytes. This is especially
-valuable for large structure or if you data structure
+valuable for large structure or if your data structure
 has bitfields. For example, for the following map,::
 
       enum A { A1, A2, A3, A4, A5 };
@@ -702,8 +700,8 @@ bpftool is able to pretty print like below:
 5.2 bpftool prog dump
 =====================
 
-The following is an example to show func_info and line_info
-can help prog dump with better kernel symbol name, function prototype
+The following is an example showing how func_info and line_info
+can help prog dump with better kernel symbol names, function prototypes
 and line information.::
 
     $ bpftool prog dump jited pinned /sys/fs/bpf/test_btf_haskv
@@ -733,10 +731,10 @@ and line information.::
     ; counts = bpf_map_lookup_elem(&btf_map, &key);
     [...]
 
-5.3 verifier log
+5.3 Verifier Log
 ================
 
-The following is an example how line_info can help verifier failure debug.::
+The following is an example of how line_info can help debugging verification failure.::
 
        /* The code at tools/testing/selftests/bpf/test_xdp_noinline.c
         * is modified as below.
@@ -867,4 +865,4 @@ The assembly code (-S) is able to show the BTF encoding in assembly format.::
 7. Testing
 **********
 
-Kernel bpf selftest `test_btf.c` provides extensive set of BTF related tests.
+Kernel bpf selftest `test_btf.c` provides extensive set of BTF-related tests.
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next 2/4] docs/btf: reflow text to fill up to 78 characters
  2019-02-28 18:45 [PATCH bpf-next 0/4] bpf docs and comments typo and formatting fixes Andrii Nakryiko
  2019-02-28 18:45 ` [PATCH bpf-next 1/4] docs/btf: fix typos, improve wording Andrii Nakryiko
@ 2019-02-28 18:45 ` Andrii Nakryiko
  2019-02-28 18:45 ` [PATCH bpf-next 3/4] bpf: fix formatting, typos, reflow comments in syscall.c, verifier.c Andrii Nakryiko
  2019-02-28 18:45 ` [PATCH bpf-next 4/4] docs/bpf: minor fixes Andrii Nakryiko
  3 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2019-02-28 18:45 UTC (permalink / raw)
  To: andrii.nakryiko, ast, yhs, netdev, bpf, daniel; +Cc: Andrii Nakryiko

Reflow paragraphs to more fully and evenly fill 78 character lines.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 Documentation/bpf/btf.rst | 300 ++++++++++++++++++--------------------
 1 file changed, 140 insertions(+), 160 deletions(-)

diff --git a/Documentation/bpf/btf.rst b/Documentation/bpf/btf.rst
index 1d761f1c5b2b..9a60a5d60e38 100644
--- a/Documentation/bpf/btf.rst
+++ b/Documentation/bpf/btf.rst
@@ -5,43 +5,35 @@ BPF Type Format (BTF)
 1. Introduction
 ***************
 
-BTF (BPF Type Format) is the metadata format which
-encodes the debug info related to BPF program/map.
-The name BTF was used initially to describe
-data types. The BTF was later extended to include
-function info for defined subroutines, and line info
-for source/line information.
-
-The debug info is used for map pretty print, function
-signature, etc. The function signature enables better
-bpf program/function kernel symbol.
-The line info helps generate
-source annotated translated byte code, jited code
-and verifier log.
+BTF (BPF Type Format) is the metadata format which encodes the debug info
+related to BPF program/map. The name BTF was used initially to describe data
+types. The BTF was later extended to include function info for defined
+subroutines, and line info for source/line information.
+
+The debug info is used for map pretty print, function signature, etc. The
+function signature enables better bpf program/function kernel symbol. The line
+info helps generate source annotated translated byte code, jited code and
+verifier log.
 
 The BTF specification contains two parts,
   * BTF kernel API
   * BTF ELF file format
 
-The kernel API is the contract between
-user space and kernel. The kernel verifies
-the BTF info before using it.
-The ELF file format is a user space contract
-between ELF file and libbpf loader.
+The kernel API is the contract between user space and kernel. The kernel
+verifies the BTF info before using it. The ELF file format is a user space
+contract between ELF file and libbpf loader.
 
-The type and string sections are part of the
-BTF kernel API, describing the debug info
-(mostly types related) referenced by the bpf program.
-These two sections are discussed in
-details in :ref:`BTF_Type_String`.
+The type and string sections are part of the BTF kernel API, describing the
+debug info (mostly types related) referenced by the bpf program. These two
+sections are discussed in details in :ref:`BTF_Type_String`.
 
 .. _BTF_Type_String:
 
 2. BTF Type and String Encoding
 *******************************
 
-The file ``include/uapi/linux/btf.h`` provides high-level
-definition of how types/strings are encoded.
+The file ``include/uapi/linux/btf.h`` provides high-level definition of how
+types/strings are encoded.
 
 The beginning of data blob must be::
 
@@ -59,25 +51,23 @@ The beginning of data blob must be::
     };
 
 The magic is ``0xeB9F``, which has different encoding for big and little
-endian systems, and can be used to test whether BTF is generated for
-big- or little-endian target.
-The ``btf_header`` is designed to be extensible with ``hdr_len`` equal to
-``sizeof(struct btf_header)`` when a data blob is generated.
+endian systems, and can be used to test whether BTF is generated for big- or
+little-endian target. The ``btf_header`` is designed to be extensible with
+``hdr_len`` equal to ``sizeof(struct btf_header)`` when a data blob is
+generated.
 
 2.1 String Encoding
 ===================
 
-The first string in the string section must be a null string.
-The rest of string table is a concatenation of other null-terminated
-strings.
+The first string in the string section must be a null string. The rest of
+string table is a concatenation of other null-terminated strings.
 
 2.2 Type Encoding
 =================
 
-The type id ``0`` is reserved for ``void`` type.
-The type section is parsed sequentially and type id is assigned to
-each recognized type starting from id ``1``.
-Currently, the following types are supported::
+The type id ``0`` is reserved for ``void`` type. The type section is parsed
+sequentially and type id is assigned to each recognized type starting from id
+``1``. Currently, the following types are supported::
 
     #define BTF_KIND_INT            1       /* Integer      */
     #define BTF_KIND_PTR            2       /* Pointer      */
@@ -122,9 +112,9 @@ Each type contains the following common data::
         };
     };
 
-For certain kinds, the common data are followed by kind-specific data.
-The ``name_off`` in ``struct btf_type`` specifies the offset in the string
-table. The following sections detail encoding of each kind.
+For certain kinds, the common data are followed by kind-specific data. The
+``name_off`` in ``struct btf_type`` specifies the offset in the string table.
+The following sections detail encoding of each kind.
 
 2.2.1 BTF_KIND_INT
 ~~~~~~~~~~~~~~~~~~
@@ -148,38 +138,33 @@ The ``BTF_INT_ENCODING`` has the following attributes::
   #define BTF_INT_CHAR    (1 << 1)
   #define BTF_INT_BOOL    (1 << 2)
 
-The ``BTF_INT_ENCODING()`` provides extra information: signedness,
-char, or bool, for the int type. The char and bool encoding
-are mostly useful for pretty print. At most one encoding can
-be specified for the int type.
-
-The ``BTF_INT_BITS()`` specifies the number of actual bits held by
-this int type. For example, a 4-bit bitfield encodes
-``BTF_INT_BITS()`` equals to 4. The ``btf_type.size * 8``
-must be equal to or greater than ``BTF_INT_BITS()`` for the type.
-The maximum value of ``BTF_INT_BITS()`` is 128.
-
-The ``BTF_INT_OFFSET()`` specifies the starting bit offset to
-calculate values for this int. For example, a bitfield struct
-member has:
- * btf member bit offset 100 from the start of the structure,
- * btf member pointing to an int type,
- * the int type has ``BTF_INT_OFFSET() = 2`` and ``BTF_INT_BITS() = 4``
+The ``BTF_INT_ENCODING()`` provides extra information: signedness, char, or
+bool, for the int type. The char and bool encoding are mostly useful for
+pretty print. At most one encoding can be specified for the int type.
+
+The ``BTF_INT_BITS()`` specifies the number of actual bits held by this int
+type. For example, a 4-bit bitfield encodes ``BTF_INT_BITS()`` equals to 4.
+The ``btf_type.size * 8`` must be equal to or greater than ``BTF_INT_BITS()``
+for the type. The maximum value of ``BTF_INT_BITS()`` is 128.
+
+The ``BTF_INT_OFFSET()`` specifies the starting bit offset to calculate values
+for this int. For example, a bitfield struct member has: * btf member bit
+offset 100 from the start of the structure, * btf member pointing to an int
+type, * the int type has ``BTF_INT_OFFSET() = 2`` and ``BTF_INT_BITS() = 4``
 
-Then in the struct memory layout, this member will occupy
-``4`` bits starting from bits ``100 + 2 = 102``.
+Then in the struct memory layout, this member will occupy ``4`` bits starting
+from bits ``100 + 2 = 102``.
 
-Alternatively, the bitfield struct member can be the following to
-access the same bits as the above:
+Alternatively, the bitfield struct member can be the following to access the
+same bits as the above:
 
  * btf member bit offset 102,
  * btf member pointing to an int type,
  * the int type has ``BTF_INT_OFFSET() = 0`` and ``BTF_INT_BITS() = 4``
 
-The original intention of ``BTF_INT_OFFSET()`` is to provide
-flexibility of bitfield encoding.
-Currently, both llvm and pahole generate ``BTF_INT_OFFSET() = 0``
-for all int types.
+The original intention of ``BTF_INT_OFFSET()`` is to provide flexibility of
+bitfield encoding. Currently, both llvm and pahole generate
+``BTF_INT_OFFSET() = 0`` for all int types.
 
 2.2.2 BTF_KIND_PTR
 ~~~~~~~~~~~~~~~~~~
@@ -216,26 +201,25 @@ The ``struct btf_array`` encoding:
   * ``index_type``: the index type
   * ``nelems``: the number of elements for this array (``0`` is also allowed).
 
-The ``index_type`` can be any regular int type
-(``u8``, ``u16``, ``u32``, ``u64``, ``unsigned __int128``).
-The original design of including ``index_type`` follows DWARF,
-which has an ``index_type`` for its array type.
+The ``index_type`` can be any regular int type (``u8``, ``u16``, ``u32``,
+``u64``, ``unsigned __int128``). The original design of including
+``index_type`` follows DWARF, which has an ``index_type`` for its array type.
 Currently in BTF, beyond type verification, the ``index_type`` is not used.
 
 The ``struct btf_array`` allows chaining through element type to represent
-multidimensional arrays. For example, for ``int a[5][6]``, the following
-type information illustrates the chaining:
+multidimensional arrays. For example, for ``int a[5][6]``, the following type
+information illustrates the chaining:
 
   * [1]: int
   * [2]: array, ``btf_array.type = [1]``, ``btf_array.nelems = 6``
   * [3]: array, ``btf_array.type = [2]``, ``btf_array.nelems = 5``
 
-Currently, both pahole and llvm collapse multidimensional array
-into one-dimensional array, e.g., for ``a[5][6]``, the ``btf_array.nelems``
-is equal to ``30``. This is because the original use case is map pretty
-print where the whole array is dumped out so one-dimensional array
-is enough. As more BTF usage is explored, pahole and llvm can be
-changed to generate proper chained representation for multidimensional arrays.
+Currently, both pahole and llvm collapse multidimensional array into
+one-dimensional array, e.g., for ``a[5][6]``, the ``btf_array.nelems`` is
+equal to ``30``. This is because the original use case is map pretty print
+where the whole array is dumped out so one-dimensional array is enough. As
+more BTF usage is explored, pahole and llvm can be changed to generate proper
+chained representation for multidimensional arrays.
 
 2.2.4 BTF_KIND_STRUCT
 ~~~~~~~~~~~~~~~~~~~~~
@@ -262,28 +246,26 @@ changed to generate proper chained representation for multidimensional arrays.
   * ``type``: the member type
   * ``offset``: <see below>
 
-If the type info ``kind_flag`` is not set, the offset contains
-only bit offset of the member. Note that the base type of the
-bitfield can only be int or enum type. If the bitfield size
-is 32, the base type can be either int or enum type.
-If the bitfield size is not 32, the base type must be int,
-and int type ``BTF_INT_BITS()`` encodes the bitfield size.
+If the type info ``kind_flag`` is not set, the offset contains only bit offset
+of the member. Note that the base type of the bitfield can only be int or enum
+type. If the bitfield size is 32, the base type can be either int or enum
+type. If the bitfield size is not 32, the base type must be int, and int type
+``BTF_INT_BITS()`` encodes the bitfield size.
 
-If the ``kind_flag`` is set, the ``btf_member.offset``
-contains both member bitfield size and bit offset. The
-bitfield size and bit offset are calculated as below.::
+If the ``kind_flag`` is set, the ``btf_member.offset`` contains both member
+bitfield size and bit offset. The bitfield size and bit offset are calculated
+as below.::
 
   #define BTF_MEMBER_BITFIELD_SIZE(val)   ((val) >> 24)
   #define BTF_MEMBER_BIT_OFFSET(val)      ((val) & 0xffffff)
 
-In this case, if the base type is an int type, it must
-be a regular int type:
+In this case, if the base type is an int type, it must be a regular int type:
 
   * ``BTF_INT_OFFSET()`` must be 0.
   * ``BTF_INT_BITS()`` must be equal to ``{1,2,4,8,16} * 8``.
 
-The following kernel patch introduced ``kind_flag`` and
-explained why both modes exist:
+The following kernel patch introduced ``kind_flag`` and explained why both
+modes exist:
 
   https://github.com/torvalds/linux/commit/9d5f9f701b1891466fb3dbb1806ad97716f95cc3#diff-fa650a64fdd3968396883d2fe8215ff3
 
@@ -381,10 +363,10 @@ No additional type data follow ``btf_type``.
 No additional type data follow ``btf_type``.
 
 A BTF_KIND_FUNC defines not a type, but a subprogram (function) whose
-signature is defined by ``type``. The subprogram is thus an instance of
-that type. The BTF_KIND_FUNC may in turn be referenced by a func_info in
-the :ref:`BTF_Ext_Section` (ELF) or in the arguments to
-:ref:`BPF_Prog_Load` (ABI).
+signature is defined by ``type``. The subprogram is thus an instance of that
+type. The BTF_KIND_FUNC may in turn be referenced by a func_info in the
+:ref:`BTF_Ext_Section` (ELF) or in the arguments to :ref:`BPF_Prog_Load`
+(ABI).
 
 2.2.13 BTF_KIND_FUNC_PROTO
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -403,13 +385,13 @@ the :ref:`BTF_Ext_Section` (ELF) or in the arguments to
         __u32   type;
     };
 
-If a BTF_KIND_FUNC_PROTO type is referred by a BTF_KIND_FUNC type,
-then ``btf_param.name_off`` must point to a valid C identifier
-except for the possible last argument representing the variable
-argument. The btf_param.type refers to parameter type.
+If a BTF_KIND_FUNC_PROTO type is referred by a BTF_KIND_FUNC type, then
+``btf_param.name_off`` must point to a valid C identifier except for the
+possible last argument representing the variable argument. The btf_param.type
+refers to parameter type.
 
-If the function has variable arguments, the last parameter
-is encoded with ``name_off = 0`` and ``type = 0``.
+If the function has variable arguments, the last parameter is encoded with
+``name_off = 0`` and ``type = 0``.
 
 3. BTF Kernel API
 *****************
@@ -457,10 +439,9 @@ The workflow typically looks like:
 3.1 BPF_BTF_LOAD
 ================
 
-Load a blob of BTF data into kernel. A blob of data,
-described in :ref:`BTF_Type_String`,
-can be directly loaded into the kernel.
-A ``btf_fd`` is returned to a userspace.
+Load a blob of BTF data into kernel. A blob of data, described in
+:ref:`BTF_Type_String`, can be directly loaded into the kernel. A ``btf_fd``
+is returned to a userspace.
 
 3.2 BPF_MAP_CREATE
 ==================
@@ -482,18 +463,18 @@ In libbpf, the map can be defined with extra annotation like below:
     };
     BPF_ANNOTATE_KV_PAIR(btf_map, int, struct ipv_counts);
 
-Here, the parameters for macro BPF_ANNOTATE_KV_PAIR are map name,
-key and value types for the map.
-During ELF parsing, libbpf is able to extract key/value type_id's
-and assign them to BPF_MAP_CREATE attributes automatically.
+Here, the parameters for macro BPF_ANNOTATE_KV_PAIR are map name, key and
+value types for the map. During ELF parsing, libbpf is able to extract
+key/value type_id's and assign them to BPF_MAP_CREATE attributes
+automatically.
 
 .. _BPF_Prog_Load:
 
 3.3 BPF_PROG_LOAD
 =================
 
-During prog_load, func_info and line_info can be passed to kernel with
-proper values for the following attributes:
+During prog_load, func_info and line_info can be passed to kernel with proper
+values for the following attributes:
 ::
 
     __u32           insn_cnt;
@@ -520,9 +501,9 @@ The func_info and line_info are an array of below, respectively.::
         __u32   line_col; /* line number and column number */
     };
 
-func_info_rec_size is the size of each func_info record, and line_info_rec_size
-is the size of each line_info record. Passing the record size to kernel make
-it possible to extend the record itself in the future.
+func_info_rec_size is the size of each func_info record, and
+line_info_rec_size is the size of each line_info record. Passing the record
+size to kernel make it possible to extend the record itself in the future.
 
 Below are requirements for func_info:
   * func_info[0].insn_off must be 0.
@@ -541,13 +522,12 @@ For line_info, the line number and column number are defined as below:
 
 3.4 BPF_{PROG,MAP}_GET_NEXT_ID
 
-In kernel, every loaded program, map or btf has a unique id.
-The id won't change during the lifetime of a program, map, or btf.
+In kernel, every loaded program, map or btf has a unique id. The id won't
+change during the lifetime of a program, map, or btf.
 
-The bpf syscall command BPF_{PROG,MAP}_GET_NEXT_ID
-returns all id's, one for each command, to user space, for bpf
-program or maps, respectively,
-so an inspection tool can inspect all programs and maps.
+The bpf syscall command BPF_{PROG,MAP}_GET_NEXT_ID returns all id's, one for
+each command, to user space, for bpf program or maps, respectively, so an
+inspection tool can inspect all programs and maps.
 
 3.5 BPF_{PROG,MAP}_GET_FD_BY_ID
 
@@ -557,24 +537,23 @@ A file descriptor needs to be obtained first for reference-counting purpose.
 3.6 BPF_OBJ_GET_INFO_BY_FD
 ==========================
 
-Once a program/map fd is acquired, an introspection tool can
-get the detailed information from kernel about this fd,
-some of which are BTF-related. For example,
-``bpf_map_info`` returns ``btf_id`` and key/value type ids.
-``bpf_prog_info`` returns ``btf_id``, func_info, and line info
-for translated bpf byte codes, and jited_line_info.
+Once a program/map fd is acquired, an introspection tool can get the detailed
+information from kernel about this fd, some of which are BTF-related. For
+example, ``bpf_map_info`` returns ``btf_id`` and key/value type ids.
+``bpf_prog_info`` returns ``btf_id``, func_info, and line info for translated
+bpf byte codes, and jited_line_info.
 
 3.7 BPF_BTF_GET_FD_BY_ID
 ========================
 
-With ``btf_id`` obtained in ``bpf_map_info`` and ``bpf_prog_info``,
-bpf syscall command BPF_BTF_GET_FD_BY_ID can retrieve a btf fd.
-Then, with command BPF_OBJ_GET_INFO_BY_FD, the btf blob, originally
-loaded into the kernel with BPF_BTF_LOAD, can be retrieved.
+With ``btf_id`` obtained in ``bpf_map_info`` and ``bpf_prog_info``, bpf
+syscall command BPF_BTF_GET_FD_BY_ID can retrieve a btf fd. Then, with
+command BPF_OBJ_GET_INFO_BY_FD, the btf blob, originally loaded into the
+kernel with BPF_BTF_LOAD, can be retrieved.
 
 With the btf blob, ``bpf_map_info``, and ``bpf_prog_info``, an introspection
-tool has full btf knowledge and is able to pretty print map key/values,
-dump func signatures and line info, along with byte/jit codes.
+tool has full btf knowledge and is able to pretty print map key/values, dump
+func signatures and line info, along with byte/jit codes.
 
 4. ELF File Format Interface
 ****************************
@@ -582,19 +561,19 @@ dump func signatures and line info, along with byte/jit codes.
 4.1 .BTF section
 ================
 
-The .BTF section contains type and string data. The format of this section
-is same as the one describe in :ref:`BTF_Type_String`.
+The .BTF section contains type and string data. The format of this section is
+same as the one describe in :ref:`BTF_Type_String`.
 
 .. _BTF_Ext_Section:
 
 4.2 .BTF.ext section
 ====================
 
-The .BTF.ext section encodes func_info and line_info which
-needs loader manipulation before loading into the kernel.
+The .BTF.ext section encodes func_info and line_info which needs loader
+manipulation before loading into the kernel.
 
-The specification for .BTF.ext section is defined at
-``tools/lib/bpf/btf.h`` and ``tools/lib/bpf/btf.c``.
+The specification for .BTF.ext section is defined at ``tools/lib/bpf/btf.h``
+and ``tools/lib/bpf/btf.c``.
 
 The current header of .BTF.ext section::
 
@@ -611,9 +590,9 @@ The current header of .BTF.ext section::
         __u32   line_info_len;
     };
 
-It is very similar to .BTF section. Instead of type/string section,
-it contains func_info and line_info section. See :ref:`BPF_Prog_Load`
-for details about func_info and line_info record format.
+It is very similar to .BTF section. Instead of type/string section, it
+contains func_info and line_info section. See :ref:`BPF_Prog_Load` for details
+about func_info and line_info record format.
 
 The func_info is organized as below.::
 
@@ -622,9 +601,9 @@ The func_info is organized as below.::
      btf_ext_info_sec for section #2 /* func_info for section #2 */
      ...
 
-``func_info_rec_size`` specifies the size of ``bpf_func_info`` structure
-when .BTF.ext is generated. ``btf_ext_info_sec``, defined below, is
-a collection of func_info for each specific ELF section.::
+``func_info_rec_size`` specifies the size of ``bpf_func_info`` structure when
+.BTF.ext is generated. ``btf_ext_info_sec``, defined below, is a collection of
+func_info for each specific ELF section.::
 
      struct btf_ext_info_sec {
         __u32   sec_name_off; /* offset to section name */
@@ -642,14 +621,14 @@ The line_info is organized as below.::
      btf_ext_info_sec for section #2 /* line_info for section #2 */
      ...
 
-``line_info_rec_size`` specifies the size of ``bpf_line_info`` structure
-when .BTF.ext is generated.
+``line_info_rec_size`` specifies the size of ``bpf_line_info`` structure when
+.BTF.ext is generated.
 
 The interpretation of ``bpf_func_info->insn_off`` and
-``bpf_line_info->insn_off`` is different between kernel API and ELF API.
-For kernel API, the ``insn_off`` is the instruction offset in the unit
-of ``struct bpf_insn``. For ELF API, the ``insn_off`` is the byte offset
-from the beginning of section (``btf_ext_info_sec->sec_name_off``).
+``bpf_line_info->insn_off`` is different between kernel API and ELF API. For
+kernel API, the ``insn_off`` is the instruction offset in the unit of ``struct
+bpf_insn``. For ELF API, the ``insn_off`` is the byte offset from the
+beginning of section (``btf_ext_info_sec->sec_name_off``).
 
 5. Using BTF
 ************
@@ -657,10 +636,9 @@ from the beginning of section (``btf_ext_info_sec->sec_name_off``).
 5.1 bpftool map pretty print
 ============================
 
-With BTF, the map key/value can be printed based on fields rather than
-simply raw bytes. This is especially
-valuable for large structure or if your data structure
-has bitfields. For example, for the following map,::
+With BTF, the map key/value can be printed based on fields rather than simply
+raw bytes. This is especially valuable for large structure or if your data
+structure has bitfields. For example, for the following map,::
 
       enum A { A1, A2, A3, A4, A5 };
       typedef enum A ___A;
@@ -700,9 +678,9 @@ bpftool is able to pretty print like below:
 5.2 bpftool prog dump
 =====================
 
-The following is an example showing how func_info and line_info
-can help prog dump with better kernel symbol names, function prototypes
-and line information.::
+The following is an example showing how func_info and line_info can help prog
+dump with better kernel symbol names, function prototypes and line
+information.::
 
     $ bpftool prog dump jited pinned /sys/fs/bpf/test_btf_haskv
     [...]
@@ -734,7 +712,8 @@ and line information.::
 5.3 Verifier Log
 ================
 
-The following is an example of how line_info can help debugging verification failure.::
+The following is an example of how line_info can help debugging verification
+failure.::
 
        /* The code at tools/testing/selftests/bpf/test_xdp_noinline.c
         * is modified as below.
@@ -763,8 +742,8 @@ You need latest pahole
 
   https://git.kernel.org/pub/scm/devel/pahole/pahole.git/
 
-or llvm (8.0 or later). The pahole acts as a dwarf2btf converter. It doesn't support .BTF.ext
-and btf BTF_KIND_FUNC type yet. For example,::
+or llvm (8.0 or later). The pahole acts as a dwarf2btf converter. It doesn't
+support .BTF.ext and btf BTF_KIND_FUNC type yet. For example,::
 
       -bash-4.4$ cat t.c
       struct t {
@@ -781,8 +760,9 @@ and btf BTF_KIND_FUNC type yet. For example,::
               c type_id=2 bitfield_size=2 bits_offset=5
       [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
 
-The llvm is able to generate .BTF and .BTF.ext directly with -g for bpf target only.
-The assembly code (-S) is able to show the BTF encoding in assembly format.::
+The llvm is able to generate .BTF and .BTF.ext directly with -g for bpf target
+only. The assembly code (-S) is able to show the BTF encoding in assembly
+format.::
 
     -bash-4.4$ cat t2.c
     typedef int __int32;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next 3/4] bpf: fix formatting, typos, reflow comments in syscall.c, verifier.c
  2019-02-28 18:45 [PATCH bpf-next 0/4] bpf docs and comments typo and formatting fixes Andrii Nakryiko
  2019-02-28 18:45 ` [PATCH bpf-next 1/4] docs/btf: fix typos, improve wording Andrii Nakryiko
  2019-02-28 18:45 ` [PATCH bpf-next 2/4] docs/btf: reflow text to fill up to 78 characters Andrii Nakryiko
@ 2019-02-28 18:45 ` Andrii Nakryiko
  2019-02-28 22:40   ` Song Liu
  2019-02-28 18:45 ` [PATCH bpf-next 4/4] docs/bpf: minor fixes Andrii Nakryiko
  3 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2019-02-28 18:45 UTC (permalink / raw)
  To: andrii.nakryiko, ast, yhs, netdev, bpf, daniel; +Cc: Andrii Nakryiko

Fix few formatting errors. Fix few typos and reflow long descriptive
comments for more even text fill.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 kernel/bpf/syscall.c  |   5 +-
 kernel/bpf/verifier.c | 169 +++++++++++++++++++++---------------------
 2 files changed, 87 insertions(+), 87 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 174581dfe225..5272ba491e00 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -214,6 +214,7 @@ static int bpf_map_init_memlock(struct bpf_map *map)
 static void bpf_map_release_memlock(struct bpf_map *map)
 {
 	struct user_struct *user = map->user;
+
 	bpf_uncharge_memlock(user, map->pages);
 	free_uid(user);
 }
@@ -299,7 +300,7 @@ static void bpf_map_put_uref(struct bpf_map *map)
 }
 
 /* decrement map refcnt and schedule it for freeing via workqueue
- * (unrelying map implementation ops->map_free() might sleep)
+ * (underlying map implementation ops->map_free() might sleep)
  */
 static void __bpf_map_put(struct bpf_map *map, bool do_idr_lock)
 {
@@ -1414,7 +1415,7 @@ struct bpf_prog *bpf_prog_inc_not_zero(struct bpf_prog *prog)
 EXPORT_SYMBOL_GPL(bpf_prog_inc_not_zero);
 
 bool bpf_prog_get_ok(struct bpf_prog *prog,
-			    enum bpf_prog_type *attach_type, bool attach_drv)
+		     enum bpf_prog_type *attach_type, bool attach_drv)
 {
 	/* not an attachment, just a refcount inc, always allow */
 	if (!attach_type)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0e4edd7e3c5f..0ee788bfd462 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -39,9 +39,9 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
 #undef BPF_MAP_TYPE
 };
 
-/* bpf_check() is a static code analyzer that walks eBPF program
- * instruction by instruction and updates register/stack state.
- * All paths of conditional branches are analyzed until 'bpf_exit' insn.
+/* bpf_check() is a static code analyzer that walks eBPF program instruction by
+ * instruction and updates register/stack state. All paths of conditional
+ * branches are analyzed until 'bpf_exit' insn.
  *
  * The first pass is depth-first-search to check that the program is a DAG.
  * It rejects the following programs:
@@ -49,15 +49,15 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  * - if loop is present (detected via back-edge)
  * - unreachable insns exist (shouldn't be a forest. program = one function)
  * - out of bounds or malformed jumps
- * The second pass is all possible path descent from the 1st insn.
- * Since it's analyzing all pathes through the program, the length of the
- * analysis is limited to 64k insn, which may be hit even if total number of
- * insn is less then 4K, but there are too many branches that change stack/regs.
- * Number of 'branches to be analyzed' is limited to 1k
+ * The second pass is all possible path descent from the 1st insn. Since it's
+ * analyzing all pathes through the program, the length of the analysis is
+ * limited to 64k insn, which may be hit even if total number of insn is less
+ * than 4K, but there are too many branches that change stack/regs. Number of
+ * 'branches to be analyzed' is limited to 1k.
  *
  * On entry to each instruction, each register has a type, and the instruction
- * changes the types of the registers depending on instruction semantics.
- * If instruction is BPF_MOV64_REG(BPF_REG_1, BPF_REG_5), then type of R5 is
+ * changes the types of the registers depending on instruction semantics. If
+ * instruction is BPF_MOV64_REG(BPF_REG_1, BPF_REG_5), then type of R5 is
  * copied to R1.
  *
  * All registers are 64-bit.
@@ -66,37 +66,36 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  * R6-R9 callee saved registers
  * R10 - frame pointer read-only
  *
- * At the start of BPF program the register R1 contains a pointer to bpf_context
- * and has type PTR_TO_CTX.
+ * At the start of BPF program the register R1 contains a pointer to
+ * bpf_context and has type PTR_TO_CTX.
  *
  * Verifier tracks arithmetic operations on pointers in case:
  *    BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
  *    BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -20),
- * 1st insn copies R10 (which has FRAME_PTR) type into R1
- * and 2nd arithmetic instruction is pattern matched to recognize
- * that it wants to construct a pointer to some element within stack.
- * So after 2nd insn, the register R1 has type PTR_TO_STACK
- * (and -20 constant is saved for further stack bounds checking).
- * Meaning that this reg is a pointer to stack plus known immediate constant.
+ * 1st insn copies R10 (which has FRAME_PTR) type into R1 and 2nd arithmetic
+ * instruction is pattern matched to recognize that it wants to construct
+ * a pointer to some element within stack. So after 2nd insn, the register R1
+ * has type PTR_TO_STACK (and -20 constant is saved for further stack bounds
+ * checking). Meaning that this reg is a pointer to stack plus known immediate
+ * constant.
  *
- * Most of the time the registers have SCALAR_VALUE type, which
- * means the register has some value, but it's not a valid pointer.
- * (like pointer plus pointer becomes SCALAR_VALUE type)
+ * Most of the time the registers have SCALAR_VALUE type, which means the
+ * register has some value, but it's not a valid pointer (like pointer plus
+ * pointer becomes SCALAR_VALUE type).
  *
- * When verifier sees load or store instructions the type of base register
- * can be: PTR_TO_MAP_VALUE, PTR_TO_CTX, PTR_TO_STACK, PTR_TO_SOCKET. These are
+ * When verifier sees load or store instructions the type of base register can
+ * be: PTR_TO_MAP_VALUE, PTR_TO_CTX, PTR_TO_STACK, PTR_TO_SOCKET. These are
  * four pointer types recognized by check_mem_access() function.
  *
  * PTR_TO_MAP_VALUE means that this register is pointing to 'map element value'
  * and the range of [ptr, ptr + map's value_size) is accessible.
  *
- * registers used to pass values to function calls are checked against
+ * Registers used to pass values to function calls are checked against
  * function argument constraints.
  *
- * ARG_PTR_TO_MAP_KEY is one of such argument constraints.
- * It means that the register type passed to this function must be
- * PTR_TO_STACK and it will be used inside the function as
- * 'pointer to map element key'
+ * ARG_PTR_TO_MAP_KEY is one of such argument constraints. It means that the
+ * register type passed to this function must be PTR_TO_STACK and it will be
+ * used inside the function as 'pointer to map element key'
  *
  * For example the argument constraints for bpf_map_lookup_elem():
  *   .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
@@ -105,8 +104,8 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  *
  * ret_type says that this function returns 'pointer to map elem value or null'
  * function expects 1st argument to be a const pointer to 'struct bpf_map' and
- * 2nd argument should be a pointer to stack, which will be used inside
- * the helper function as a pointer to map element key.
+ * 2nd argument should be a pointer to stack, which will be used inside the
+ * helper function as a pointer to map element key.
  *
  * On the kernel side the helper function looks like:
  * u64 bpf_map_lookup_elem(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
@@ -115,9 +114,9 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  *    void *key = (void *) (unsigned long) r2;
  *    void *value;
  *
- *    here kernel can access 'key' and 'map' pointers safely, knowing that
- *    [key, key + map->key_size) bytes are valid and were initialized on
- *    the stack of eBPF program.
+ *    Here kernel can access 'key' and 'map' pointers safely, knowing that
+ *    [key, key + map->key_size) bytes are valid and were initialized on the
+ *    stack of eBPF program.
  * }
  *
  * Corresponding eBPF program may look like:
@@ -126,21 +125,21 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  *    BPF_LD_MAP_FD(BPF_REG_1, map_fd),      // after this insn R1 type is CONST_PTR_TO_MAP
  *    BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
  * here verifier looks at prototype of map_lookup_elem() and sees:
- * .arg1_type == ARG_CONST_MAP_PTR and R1->type == CONST_PTR_TO_MAP, which is ok,
- * Now verifier knows that this map has key of R1->map_ptr->key_size bytes
+ * .arg1_type == ARG_CONST_MAP_PTR and R1->type == CONST_PTR_TO_MAP, which is
+ * ok. Now verifier knows that this map has key of R1->map_ptr->key_size bytes.
  *
- * Then .arg2_type == ARG_PTR_TO_MAP_KEY and R2->type == PTR_TO_STACK, ok so far,
- * Now verifier checks that [R2, R2 + map's key_size) are within stack limits
- * and were initialized prior to this call.
- * If it's ok, then verifier allows this BPF_CALL insn and looks at
- * .ret_type which is RET_PTR_TO_MAP_VALUE_OR_NULL, so it sets
- * R0->type = PTR_TO_MAP_VALUE_OR_NULL which means bpf_map_lookup_elem() function
- * returns ether pointer to map value or NULL.
+ * Then .arg2_type == ARG_PTR_TO_MAP_KEY and R2->type == PTR_TO_STACK, ok so
+ * far. Now verifier checks that [R2, R2 + map's key_size) are within stack
+ * limits and were initialized prior to this call. If it's ok, then verifier
+ * allows this BPF_CALL insn and looks at .ret_type which is
+ * RET_PTR_TO_MAP_VALUE_OR_NULL, so it sets R0->type = PTR_TO_MAP_VALUE_OR_NULL
+ * which means bpf_map_lookup_elem() function returns either pointer to a map
+ * value or NULL.
  *
  * When type PTR_TO_MAP_VALUE_OR_NULL passes through 'if (reg != 0) goto +off'
  * insn, the register holding that pointer in the true branch changes state to
- * PTR_TO_MAP_VALUE and the same register changes state to CONST_IMM in the false
- * branch. See check_cond_jmp_op().
+ * PTR_TO_MAP_VALUE and the same register changes state to CONST_IMM in the
+ * false branch. See check_cond_jmp_op().
  *
  * After the call R0 is set to return type of the function and registers R1-R5
  * are set to NOT_INIT to indicate that they are no longer readable.
@@ -148,10 +147,11 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
  * The following reference types represent a potential reference to a kernel
  * resource which, after first being allocated, must be checked and freed by
  * the BPF program:
- * - PTR_TO_SOCKET_OR_NULL, PTR_TO_SOCKET
+ * - PTR_TO_SOCKET_OR_NULL
+ * - PTR_TO_SOCKET
  *
- * When the verifier sees a helper call return a reference type, it allocates a
- * pointer id for the reference and stores it in the current function state.
+ * When the verifier sees a helper call return a reference type, it allocates
+ * a pointer id for the reference and stores it in the current function state.
  * Similar to the way that PTR_TO_MAP_VALUE_OR_NULL is converted into
  * PTR_TO_MAP_VALUE, PTR_TO_SOCKET_OR_NULL becomes PTR_TO_SOCKET when the type
  * passes through a NULL-check conditional. For the branch wherein the state is
@@ -258,7 +258,7 @@ void bpf_verifier_vlog(struct bpf_verifier_log *log, const char *fmt,
 		log->ubuf = NULL;
 }
 
-/* log_level controls verbosity level of eBPF verifier.
+/* env->log.level controls verbosity level of eBPF verifier.
  * bpf_verifier_log_write() is used to dump the verification trace to the log,
  * so the user can figure out what's wrong with the program
  */
@@ -389,7 +389,7 @@ static bool is_release_function(enum bpf_func_id func_id)
 static bool is_acquire_function(enum bpf_func_id func_id)
 {
 	return func_id == BPF_FUNC_sk_lookup_tcp ||
-		func_id == BPF_FUNC_sk_lookup_udp;
+	       func_id == BPF_FUNC_sk_lookup_udp;
 }
 
 /* string representation of 'enum bpf_reg_type' */
@@ -559,39 +559,39 @@ COPY_STATE_FN(reference, acquired_refs, refs, 1)
 COPY_STATE_FN(stack, allocated_stack, stack, BPF_REG_SIZE)
 #undef COPY_STATE_FN
 
-#define REALLOC_STATE_FN(NAME, COUNT, FIELD, SIZE)			\
-static int realloc_##NAME##_state(struct bpf_func_state *state, int size, \
-				  bool copy_old)			\
-{									\
-	u32 old_size = state->COUNT;					\
-	struct bpf_##NAME##_state *new_##FIELD;				\
-	int slot = size / SIZE;						\
-									\
-	if (size <= old_size || !size) {				\
-		if (copy_old)						\
-			return 0;					\
-		state->COUNT = slot * SIZE;				\
-		if (!size && old_size) {				\
-			kfree(state->FIELD);				\
-			state->FIELD = NULL;				\
-		}							\
-		return 0;						\
-	}								\
+#define REALLOC_STATE_FN(NAME, COUNT, FIELD, SIZE)			     \
+static int realloc_##NAME##_state(struct bpf_func_state *state, int size,    \
+				  bool copy_old)			     \
+{									     \
+	u32 old_size = state->COUNT;					     \
+	struct bpf_##NAME##_state *new_##FIELD;				     \
+	int slot = size / SIZE;						     \
+									     \
+	if (size <= old_size || !size) {				     \
+		if (copy_old)						     \
+			return 0;					     \
+		state->COUNT = slot * SIZE;				     \
+		if (!size && old_size) {				     \
+			kfree(state->FIELD);				     \
+			state->FIELD = NULL;				     \
+		}							     \
+		return 0;						     \
+	}								     \
 	new_##FIELD = kmalloc_array(slot, sizeof(struct bpf_##NAME##_state), \
-				    GFP_KERNEL);			\
-	if (!new_##FIELD)						\
-		return -ENOMEM;						\
-	if (copy_old) {							\
-		if (state->FIELD)					\
-			memcpy(new_##FIELD, state->FIELD,		\
-			       sizeof(*new_##FIELD) * (old_size / SIZE)); \
-		memset(new_##FIELD + old_size / SIZE, 0,		\
-		       sizeof(*new_##FIELD) * (size - old_size) / SIZE); \
-	}								\
-	state->COUNT = slot * SIZE;					\
-	kfree(state->FIELD);						\
-	state->FIELD = new_##FIELD;					\
-	return 0;							\
+				    GFP_KERNEL);			     \
+	if (!new_##FIELD)						     \
+		return -ENOMEM;						     \
+	if (copy_old) {							     \
+		if (state->FIELD)					     \
+			memcpy(new_##FIELD, state->FIELD,		     \
+			       sizeof(*new_##FIELD) * (old_size / SIZE));    \
+		memset(new_##FIELD + old_size / SIZE, 0,		     \
+		       sizeof(*new_##FIELD) * (size - old_size) / SIZE);     \
+	}								     \
+	state->COUNT = slot * SIZE;					     \
+	kfree(state->FIELD);						     \
+	state->FIELD = new_##FIELD;					     \
+	return 0;							     \
 }
 /* realloc_reference_state() */
 REALLOC_STATE_FN(reference, acquired_refs, refs, 1)
@@ -617,7 +617,7 @@ static int realloc_func_state(struct bpf_func_state *state, int stack_size,
 
 /* Acquire a pointer id from the env and update the state->refs to include
  * this new pointer reference.
- * On success, returns a valid pointer id to associate with the register
+ * On success, returns a valid pointer id to associate with the register.
  * On failure, returns a negative errno.
  */
 static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx)
@@ -714,7 +714,7 @@ static int copy_verifier_state(struct bpf_verifier_state *dst_state,
 	struct bpf_func_state *dst;
 	int i, err;
 
-	/* if dst has more stack frames then src frame, free them */
+	/* if dst has more stack frames than src frame, free them */
 	for (i = src->curframe + 1; i <= dst_state->curframe; i++) {
 		free_func_state(dst_state->frame[i]);
 		dst_state->frame[i] = NULL;
@@ -863,8 +863,7 @@ static bool reg_is_init_pkt_pointer(const struct bpf_reg_state *reg,
 				    enum bpf_reg_type which)
 {
 	/* The register can already have a range from prior markings.
-	 * This is fine as long as it hasn't been advanced from its
-	 * origin.
+	 * This is fine as long as it hasn't been advanced from its origin.
 	 */
 	return reg->type == which &&
 	       reg->id == 0 &&
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next 4/4] docs/bpf: minor fixes
  2019-02-28 18:45 [PATCH bpf-next 0/4] bpf docs and comments typo and formatting fixes Andrii Nakryiko
                   ` (2 preceding siblings ...)
  2019-02-28 18:45 ` [PATCH bpf-next 3/4] bpf: fix formatting, typos, reflow comments in syscall.c, verifier.c Andrii Nakryiko
@ 2019-02-28 18:45 ` Andrii Nakryiko
  3 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2019-02-28 18:45 UTC (permalink / raw)
  To: andrii.nakryiko, ast, yhs, netdev, bpf, daniel; +Cc: Andrii Nakryiko

Fix few casing and punctuation glitches.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 Documentation/bpf/bpf_design_QA.rst | 24 ++++++++++++------------
 Documentation/networking/filter.txt |  2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/Documentation/bpf/bpf_design_QA.rst b/Documentation/bpf/bpf_design_QA.rst
index 7cc9e368c1e9..10453c627135 100644
--- a/Documentation/bpf/bpf_design_QA.rst
+++ b/Documentation/bpf/bpf_design_QA.rst
@@ -36,27 +36,27 @@ consideration important quirks of other architectures) and
 defines calling convention that is compatible with C calling
 convention of the linux kernel on those architectures.
 
-Q: can multiple return values be supported in the future?
+Q: Can multiple return values be supported in the future?
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 A: NO. BPF allows only register R0 to be used as return value.
 
-Q: can more than 5 function arguments be supported in the future?
+Q: Can more than 5 function arguments be supported in the future?
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 A: NO. BPF calling convention only allows registers R1-R5 to be used
 as arguments. BPF is not a standalone instruction set.
 (unlike x64 ISA that allows msft, cdecl and other conventions)
 
-Q: can BPF programs access instruction pointer or return address?
+Q: Can BPF programs access instruction pointer or return address?
 -----------------------------------------------------------------
 A: NO.
 
-Q: can BPF programs access stack pointer ?
+Q: Can BPF programs access stack pointer ?
 ------------------------------------------
 A: NO.
 
 Only frame pointer (register R10) is accessible.
 From compiler point of view it's necessary to have stack pointer.
-For example LLVM defines register R11 as stack pointer in its
+For example, LLVM defines register R11 as stack pointer in its
 BPF backend, but it makes sure that generated code never uses it.
 
 Q: Does C-calling convention diminishes possible use cases?
@@ -66,8 +66,8 @@ A: YES.
 BPF design forces addition of major functionality in the form
 of kernel helper functions and kernel objects like BPF maps with
 seamless interoperability between them. It lets kernel call into
-BPF programs and programs call kernel helpers with zero overhead.
-As all of them were native C code. That is particularly the case
+BPF programs and programs call kernel helpers with zero overhead,
+as all of them were native C code. That is particularly the case
 for JITed BPF programs that are indistinguishable from
 native kernel C code.
 
@@ -75,9 +75,9 @@ Q: Does it mean that 'innovative' extensions to BPF code are disallowed?
 ------------------------------------------------------------------------
 A: Soft yes.
 
-At least for now until BPF core has support for
+At least for now, until BPF core has support for
 bpf-to-bpf calls, indirect calls, loops, global variables,
-jump tables, read only sections and all other normal constructs
+jump tables, read-only sections, and all other normal constructs
 that C code can produce.
 
 Q: Can loops be supported in a safe way?
@@ -109,16 +109,16 @@ For example why BPF_JNE and other compare and jumps are not cpu-like?
 A: This was necessary to avoid introducing flags into ISA which are
 impossible to make generic and efficient across CPU architectures.
 
-Q: why BPF_DIV instruction doesn't map to x64 div?
+Q: Why BPF_DIV instruction doesn't map to x64 div?
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 A: Because if we picked one-to-one relationship to x64 it would have made
 it more complicated to support on arm64 and other archs. Also it
 needs div-by-zero runtime check.
 
-Q: why there is no BPF_SDIV for signed divide operation?
+Q: Why there is no BPF_SDIV for signed divide operation?
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 A: Because it would be rarely used. llvm errors in such case and
-prints a suggestion to use unsigned divide instead
+prints a suggestion to use unsigned divide instead.
 
 Q: Why BPF has implicit prologue and epilogue?
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/Documentation/networking/filter.txt b/Documentation/networking/filter.txt
index b5e060edfc38..319e5e041f38 100644
--- a/Documentation/networking/filter.txt
+++ b/Documentation/networking/filter.txt
@@ -829,7 +829,7 @@ tracing filters may do to maintain counters of events, for example. Register R9
 is not used by socket filters either, but more complex filters may be running
 out of registers and would have to resort to spill/fill to stack.
 
-Internal BPF can used as generic assembler for last step performance
+Internal BPF can be used as a generic assembler for last step performance
 optimizations, socket filters and seccomp are using it as assembler. Tracing
 filters may use it as assembler to generate code from kernel. In kernel usage
 may not be bounded by security considerations, since generated internal BPF code
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 3/4] bpf: fix formatting, typos, reflow comments in syscall.c, verifier.c
  2019-02-28 18:45 ` [PATCH bpf-next 3/4] bpf: fix formatting, typos, reflow comments in syscall.c, verifier.c Andrii Nakryiko
@ 2019-02-28 22:40   ` Song Liu
  2019-03-01  0:27     ` Daniel Borkmann
  0 siblings, 1 reply; 10+ messages in thread
From: Song Liu @ 2019-02-28 22:40 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, ast, yhs, Networking, bpf, Daniel Borkmann

On Thu, Feb 28, 2019 at 10:59 AM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Fix few formatting errors. Fix few typos and reflow long descriptive
> comments for more even text fill.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

I think we should not change the code for formatting, as these changes make
git-blame more difficult. How about we only make changes to comments?

Thanks,
Song

> ---
>  kernel/bpf/syscall.c  |   5 +-
>  kernel/bpf/verifier.c | 169 +++++++++++++++++++++---------------------
>  2 files changed, 87 insertions(+), 87 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 174581dfe225..5272ba491e00 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -214,6 +214,7 @@ static int bpf_map_init_memlock(struct bpf_map *map)
>  static void bpf_map_release_memlock(struct bpf_map *map)
>  {
>         struct user_struct *user = map->user;
> +
>         bpf_uncharge_memlock(user, map->pages);
>         free_uid(user);
>  }
> @@ -299,7 +300,7 @@ static void bpf_map_put_uref(struct bpf_map *map)
>  }
>
>  /* decrement map refcnt and schedule it for freeing via workqueue
> - * (unrelying map implementation ops->map_free() might sleep)
> + * (underlying map implementation ops->map_free() might sleep)
>   */
>  static void __bpf_map_put(struct bpf_map *map, bool do_idr_lock)
>  {
> @@ -1414,7 +1415,7 @@ struct bpf_prog *bpf_prog_inc_not_zero(struct bpf_prog *prog)
>  EXPORT_SYMBOL_GPL(bpf_prog_inc_not_zero);
>
>  bool bpf_prog_get_ok(struct bpf_prog *prog,
> -                           enum bpf_prog_type *attach_type, bool attach_drv)
> +                    enum bpf_prog_type *attach_type, bool attach_drv)
>  {
>         /* not an attachment, just a refcount inc, always allow */
>         if (!attach_type)
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0e4edd7e3c5f..0ee788bfd462 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -39,9 +39,9 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
>  #undef BPF_MAP_TYPE
>  };
>
> -/* bpf_check() is a static code analyzer that walks eBPF program
> - * instruction by instruction and updates register/stack state.
> - * All paths of conditional branches are analyzed until 'bpf_exit' insn.
> +/* bpf_check() is a static code analyzer that walks eBPF program instruction by
> + * instruction and updates register/stack state. All paths of conditional
> + * branches are analyzed until 'bpf_exit' insn.
>   *
>   * The first pass is depth-first-search to check that the program is a DAG.
>   * It rejects the following programs:
> @@ -49,15 +49,15 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
>   * - if loop is present (detected via back-edge)
>   * - unreachable insns exist (shouldn't be a forest. program = one function)
>   * - out of bounds or malformed jumps
> - * The second pass is all possible path descent from the 1st insn.
> - * Since it's analyzing all pathes through the program, the length of the
> - * analysis is limited to 64k insn, which may be hit even if total number of
> - * insn is less then 4K, but there are too many branches that change stack/regs.
> - * Number of 'branches to be analyzed' is limited to 1k
> + * The second pass is all possible path descent from the 1st insn. Since it's
> + * analyzing all pathes through the program, the length of the analysis is
> + * limited to 64k insn, which may be hit even if total number of insn is less
> + * than 4K, but there are too many branches that change stack/regs. Number of
> + * 'branches to be analyzed' is limited to 1k.
>   *
>   * On entry to each instruction, each register has a type, and the instruction
> - * changes the types of the registers depending on instruction semantics.
> - * If instruction is BPF_MOV64_REG(BPF_REG_1, BPF_REG_5), then type of R5 is
> + * changes the types of the registers depending on instruction semantics. If
> + * instruction is BPF_MOV64_REG(BPF_REG_1, BPF_REG_5), then type of R5 is
>   * copied to R1.
>   *
>   * All registers are 64-bit.
> @@ -66,37 +66,36 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
>   * R6-R9 callee saved registers
>   * R10 - frame pointer read-only
>   *
> - * At the start of BPF program the register R1 contains a pointer to bpf_context
> - * and has type PTR_TO_CTX.
> + * At the start of BPF program the register R1 contains a pointer to
> + * bpf_context and has type PTR_TO_CTX.
>   *
>   * Verifier tracks arithmetic operations on pointers in case:
>   *    BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
>   *    BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -20),
> - * 1st insn copies R10 (which has FRAME_PTR) type into R1
> - * and 2nd arithmetic instruction is pattern matched to recognize
> - * that it wants to construct a pointer to some element within stack.
> - * So after 2nd insn, the register R1 has type PTR_TO_STACK
> - * (and -20 constant is saved for further stack bounds checking).
> - * Meaning that this reg is a pointer to stack plus known immediate constant.
> + * 1st insn copies R10 (which has FRAME_PTR) type into R1 and 2nd arithmetic
> + * instruction is pattern matched to recognize that it wants to construct
> + * a pointer to some element within stack. So after 2nd insn, the register R1
> + * has type PTR_TO_STACK (and -20 constant is saved for further stack bounds
> + * checking). Meaning that this reg is a pointer to stack plus known immediate
> + * constant.
>   *
> - * Most of the time the registers have SCALAR_VALUE type, which
> - * means the register has some value, but it's not a valid pointer.
> - * (like pointer plus pointer becomes SCALAR_VALUE type)
> + * Most of the time the registers have SCALAR_VALUE type, which means the
> + * register has some value, but it's not a valid pointer (like pointer plus
> + * pointer becomes SCALAR_VALUE type).
>   *
> - * When verifier sees load or store instructions the type of base register
> - * can be: PTR_TO_MAP_VALUE, PTR_TO_CTX, PTR_TO_STACK, PTR_TO_SOCKET. These are
> + * When verifier sees load or store instructions the type of base register can
> + * be: PTR_TO_MAP_VALUE, PTR_TO_CTX, PTR_TO_STACK, PTR_TO_SOCKET. These are
>   * four pointer types recognized by check_mem_access() function.
>   *
>   * PTR_TO_MAP_VALUE means that this register is pointing to 'map element value'
>   * and the range of [ptr, ptr + map's value_size) is accessible.
>   *
> - * registers used to pass values to function calls are checked against
> + * Registers used to pass values to function calls are checked against
>   * function argument constraints.
>   *
> - * ARG_PTR_TO_MAP_KEY is one of such argument constraints.
> - * It means that the register type passed to this function must be
> - * PTR_TO_STACK and it will be used inside the function as
> - * 'pointer to map element key'
> + * ARG_PTR_TO_MAP_KEY is one of such argument constraints. It means that the
> + * register type passed to this function must be PTR_TO_STACK and it will be
> + * used inside the function as 'pointer to map element key'
>   *
>   * For example the argument constraints for bpf_map_lookup_elem():
>   *   .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> @@ -105,8 +104,8 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
>   *
>   * ret_type says that this function returns 'pointer to map elem value or null'
>   * function expects 1st argument to be a const pointer to 'struct bpf_map' and
> - * 2nd argument should be a pointer to stack, which will be used inside
> - * the helper function as a pointer to map element key.
> + * 2nd argument should be a pointer to stack, which will be used inside the
> + * helper function as a pointer to map element key.
>   *
>   * On the kernel side the helper function looks like:
>   * u64 bpf_map_lookup_elem(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
> @@ -115,9 +114,9 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
>   *    void *key = (void *) (unsigned long) r2;
>   *    void *value;
>   *
> - *    here kernel can access 'key' and 'map' pointers safely, knowing that
> - *    [key, key + map->key_size) bytes are valid and were initialized on
> - *    the stack of eBPF program.
> + *    Here kernel can access 'key' and 'map' pointers safely, knowing that
> + *    [key, key + map->key_size) bytes are valid and were initialized on the
> + *    stack of eBPF program.
>   * }
>   *
>   * Corresponding eBPF program may look like:
> @@ -126,21 +125,21 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
>   *    BPF_LD_MAP_FD(BPF_REG_1, map_fd),      // after this insn R1 type is CONST_PTR_TO_MAP
>   *    BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
>   * here verifier looks at prototype of map_lookup_elem() and sees:
> - * .arg1_type == ARG_CONST_MAP_PTR and R1->type == CONST_PTR_TO_MAP, which is ok,
> - * Now verifier knows that this map has key of R1->map_ptr->key_size bytes
> + * .arg1_type == ARG_CONST_MAP_PTR and R1->type == CONST_PTR_TO_MAP, which is
> + * ok. Now verifier knows that this map has key of R1->map_ptr->key_size bytes.
>   *
> - * Then .arg2_type == ARG_PTR_TO_MAP_KEY and R2->type == PTR_TO_STACK, ok so far,
> - * Now verifier checks that [R2, R2 + map's key_size) are within stack limits
> - * and were initialized prior to this call.
> - * If it's ok, then verifier allows this BPF_CALL insn and looks at
> - * .ret_type which is RET_PTR_TO_MAP_VALUE_OR_NULL, so it sets
> - * R0->type = PTR_TO_MAP_VALUE_OR_NULL which means bpf_map_lookup_elem() function
> - * returns ether pointer to map value or NULL.
> + * Then .arg2_type == ARG_PTR_TO_MAP_KEY and R2->type == PTR_TO_STACK, ok so
> + * far. Now verifier checks that [R2, R2 + map's key_size) are within stack
> + * limits and were initialized prior to this call. If it's ok, then verifier
> + * allows this BPF_CALL insn and looks at .ret_type which is
> + * RET_PTR_TO_MAP_VALUE_OR_NULL, so it sets R0->type = PTR_TO_MAP_VALUE_OR_NULL
> + * which means bpf_map_lookup_elem() function returns either pointer to a map
> + * value or NULL.
>   *
>   * When type PTR_TO_MAP_VALUE_OR_NULL passes through 'if (reg != 0) goto +off'
>   * insn, the register holding that pointer in the true branch changes state to
> - * PTR_TO_MAP_VALUE and the same register changes state to CONST_IMM in the false
> - * branch. See check_cond_jmp_op().
> + * PTR_TO_MAP_VALUE and the same register changes state to CONST_IMM in the
> + * false branch. See check_cond_jmp_op().
>   *
>   * After the call R0 is set to return type of the function and registers R1-R5
>   * are set to NOT_INIT to indicate that they are no longer readable.
> @@ -148,10 +147,11 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
>   * The following reference types represent a potential reference to a kernel
>   * resource which, after first being allocated, must be checked and freed by
>   * the BPF program:
> - * - PTR_TO_SOCKET_OR_NULL, PTR_TO_SOCKET
> + * - PTR_TO_SOCKET_OR_NULL
> + * - PTR_TO_SOCKET
>   *
> - * When the verifier sees a helper call return a reference type, it allocates a
> - * pointer id for the reference and stores it in the current function state.
> + * When the verifier sees a helper call return a reference type, it allocates
> + * a pointer id for the reference and stores it in the current function state.
>   * Similar to the way that PTR_TO_MAP_VALUE_OR_NULL is converted into
>   * PTR_TO_MAP_VALUE, PTR_TO_SOCKET_OR_NULL becomes PTR_TO_SOCKET when the type
>   * passes through a NULL-check conditional. For the branch wherein the state is
> @@ -258,7 +258,7 @@ void bpf_verifier_vlog(struct bpf_verifier_log *log, const char *fmt,
>                 log->ubuf = NULL;
>  }
>
> -/* log_level controls verbosity level of eBPF verifier.
> +/* env->log.level controls verbosity level of eBPF verifier.
>   * bpf_verifier_log_write() is used to dump the verification trace to the log,
>   * so the user can figure out what's wrong with the program
>   */
> @@ -389,7 +389,7 @@ static bool is_release_function(enum bpf_func_id func_id)
>  static bool is_acquire_function(enum bpf_func_id func_id)
>  {
>         return func_id == BPF_FUNC_sk_lookup_tcp ||
> -               func_id == BPF_FUNC_sk_lookup_udp;
> +              func_id == BPF_FUNC_sk_lookup_udp;
>  }
>
>  /* string representation of 'enum bpf_reg_type' */
> @@ -559,39 +559,39 @@ COPY_STATE_FN(reference, acquired_refs, refs, 1)
>  COPY_STATE_FN(stack, allocated_stack, stack, BPF_REG_SIZE)
>  #undef COPY_STATE_FN
>
> -#define REALLOC_STATE_FN(NAME, COUNT, FIELD, SIZE)                     \
> -static int realloc_##NAME##_state(struct bpf_func_state *state, int size, \
> -                                 bool copy_old)                        \
> -{                                                                      \
> -       u32 old_size = state->COUNT;                                    \
> -       struct bpf_##NAME##_state *new_##FIELD;                         \
> -       int slot = size / SIZE;                                         \
> -                                                                       \
> -       if (size <= old_size || !size) {                                \
> -               if (copy_old)                                           \
> -                       return 0;                                       \
> -               state->COUNT = slot * SIZE;                             \
> -               if (!size && old_size) {                                \
> -                       kfree(state->FIELD);                            \
> -                       state->FIELD = NULL;                            \
> -               }                                                       \
> -               return 0;                                               \
> -       }                                                               \
> +#define REALLOC_STATE_FN(NAME, COUNT, FIELD, SIZE)                          \
> +static int realloc_##NAME##_state(struct bpf_func_state *state, int size,    \
> +                                 bool copy_old)                             \
> +{                                                                           \
> +       u32 old_size = state->COUNT;                                         \
> +       struct bpf_##NAME##_state *new_##FIELD;                              \
> +       int slot = size / SIZE;                                              \
> +                                                                            \
> +       if (size <= old_size || !size) {                                     \
> +               if (copy_old)                                                \
> +                       return 0;                                            \
> +               state->COUNT = slot * SIZE;                                  \
> +               if (!size && old_size) {                                     \
> +                       kfree(state->FIELD);                                 \
> +                       state->FIELD = NULL;                                 \
> +               }                                                            \
> +               return 0;                                                    \
> +       }                                                                    \
>         new_##FIELD = kmalloc_array(slot, sizeof(struct bpf_##NAME##_state), \
> -                                   GFP_KERNEL);                        \
> -       if (!new_##FIELD)                                               \
> -               return -ENOMEM;                                         \
> -       if (copy_old) {                                                 \
> -               if (state->FIELD)                                       \
> -                       memcpy(new_##FIELD, state->FIELD,               \
> -                              sizeof(*new_##FIELD) * (old_size / SIZE)); \
> -               memset(new_##FIELD + old_size / SIZE, 0,                \
> -                      sizeof(*new_##FIELD) * (size - old_size) / SIZE); \
> -       }                                                               \
> -       state->COUNT = slot * SIZE;                                     \
> -       kfree(state->FIELD);                                            \
> -       state->FIELD = new_##FIELD;                                     \
> -       return 0;                                                       \
> +                                   GFP_KERNEL);                             \
> +       if (!new_##FIELD)                                                    \
> +               return -ENOMEM;                                              \
> +       if (copy_old) {                                                      \
> +               if (state->FIELD)                                            \
> +                       memcpy(new_##FIELD, state->FIELD,                    \
> +                              sizeof(*new_##FIELD) * (old_size / SIZE));    \
> +               memset(new_##FIELD + old_size / SIZE, 0,                     \
> +                      sizeof(*new_##FIELD) * (size - old_size) / SIZE);     \
> +       }                                                                    \
> +       state->COUNT = slot * SIZE;                                          \
> +       kfree(state->FIELD);                                                 \
> +       state->FIELD = new_##FIELD;                                          \
> +       return 0;                                                            \
>  }
>  /* realloc_reference_state() */
>  REALLOC_STATE_FN(reference, acquired_refs, refs, 1)
> @@ -617,7 +617,7 @@ static int realloc_func_state(struct bpf_func_state *state, int stack_size,
>
>  /* Acquire a pointer id from the env and update the state->refs to include
>   * this new pointer reference.
> - * On success, returns a valid pointer id to associate with the register
> + * On success, returns a valid pointer id to associate with the register.
>   * On failure, returns a negative errno.
>   */
>  static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx)
> @@ -714,7 +714,7 @@ static int copy_verifier_state(struct bpf_verifier_state *dst_state,
>         struct bpf_func_state *dst;
>         int i, err;
>
> -       /* if dst has more stack frames then src frame, free them */
> +       /* if dst has more stack frames than src frame, free them */
>         for (i = src->curframe + 1; i <= dst_state->curframe; i++) {
>                 free_func_state(dst_state->frame[i]);
>                 dst_state->frame[i] = NULL;
> @@ -863,8 +863,7 @@ static bool reg_is_init_pkt_pointer(const struct bpf_reg_state *reg,
>                                     enum bpf_reg_type which)
>  {
>         /* The register can already have a range from prior markings.
> -        * This is fine as long as it hasn't been advanced from its
> -        * origin.
> +        * This is fine as long as it hasn't been advanced from its origin.
>          */
>         return reg->type == which &&
>                reg->id == 0 &&
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 1/4] docs/btf: fix typos, improve wording
  2019-02-28 18:45 ` [PATCH bpf-next 1/4] docs/btf: fix typos, improve wording Andrii Nakryiko
@ 2019-02-28 23:47   ` Yonghong Song
  0 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2019-02-28 23:47 UTC (permalink / raw)
  To: Andrii Nakryiko, andrii.nakryiko, Alexei Starovoitov, netdev,
	bpf, daniel



On 2/28/19 10:45 AM, Andrii Nakryiko wrote:
> Fix various typos, some of the formatting and wording for
> Documentation/btf.rst.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Ack for Patch #1 and #2:
   Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   Documentation/bpf/btf.rst | 108 +++++++++++++++++++-------------------
>   1 file changed, 53 insertions(+), 55 deletions(-)
> 
> diff --git a/Documentation/bpf/btf.rst b/Documentation/bpf/btf.rst
> index 1d434c3a268d..1d761f1c5b2b 100644
> --- a/Documentation/bpf/btf.rst
> +++ b/Documentation/bpf/btf.rst
> @@ -5,7 +5,7 @@ BPF Type Format (BTF)
>   1. Introduction
>   ***************
>   
> -BTF (BPF Type Format) is the meta data format which
> +BTF (BPF Type Format) is the metadata format which
>   encodes the debug info related to BPF program/map.
>   The name BTF was used initially to describe
>   data types. The BTF was later extended to include
> @@ -40,8 +40,8 @@ details in :ref:`BTF_Type_String`.
>   2. BTF Type and String Encoding
>   *******************************
>   
> -The file ``include/uapi/linux/btf.h`` provides high
> -level definition on how types/strings are encoded.
> +The file ``include/uapi/linux/btf.h`` provides high-level
> +definition of how types/strings are encoded.
>   
>   The beginning of data blob must be::
>   
> @@ -59,23 +59,23 @@ The beginning of data blob must be::
>       };
>   
>   The magic is ``0xeB9F``, which has different encoding for big and little
> -endian system, and can be used to test whether BTF is generated for
> -big or little endian target.
> -The btf_header is designed to be extensible with hdr_len equal to
> -``sizeof(struct btf_header)`` when the data blob is generated.
> +endian systems, and can be used to test whether BTF is generated for
> +big- or little-endian target.
> +The ``btf_header`` is designed to be extensible with ``hdr_len`` equal to
> +``sizeof(struct btf_header)`` when a data blob is generated.
>   
>   2.1 String Encoding
>   ===================
>   
>   The first string in the string section must be a null string.
> -The rest of string table is a concatenation of other null-treminated
> +The rest of string table is a concatenation of other null-terminated
>   strings.
>   
>   2.2 Type Encoding
>   =================
>   
>   The type id ``0`` is reserved for ``void`` type.
> -The type section is parsed sequentially and the type id is assigned to
> +The type section is parsed sequentially and type id is assigned to
>   each recognized type starting from id ``1``.
>   Currently, the following types are supported::
>   
> @@ -122,9 +122,9 @@ Each type contains the following common data::
>           };
>       };
>   
> -For certain kinds, the common data are followed by kind specific data.
> -The ``name_off`` in ``struct btf_type`` specifies the offset in the string table.
> -The following details encoding of each kind.
> +For certain kinds, the common data are followed by kind-specific data.
> +The ``name_off`` in ``struct btf_type`` specifies the offset in the string
> +table. The following sections detail encoding of each kind.
>   
>   2.2.1 BTF_KIND_INT
>   ~~~~~~~~~~~~~~~~~~
> @@ -136,7 +136,7 @@ The following details encoding of each kind.
>    * ``info.vlen``: 0
>    * ``size``: the size of the int type in bytes.
>   
> -``btf_type`` is followed by a ``u32`` with following bits arrangement::
> +``btf_type`` is followed by a ``u32`` with the following bits arrangement::
>   
>     #define BTF_INT_ENCODING(VAL)   (((VAL) & 0x0f000000) >> 24)
>     #define BTF_INT_OFFSET(VAL)     (((VAL  & 0x00ff0000)) >> 16)
> @@ -148,7 +148,7 @@ The ``BTF_INT_ENCODING`` has the following attributes::
>     #define BTF_INT_CHAR    (1 << 1)
>     #define BTF_INT_BOOL    (1 << 2)
>   
> -The ``BTF_INT_ENCODING()`` provides extra information, signness,
> +The ``BTF_INT_ENCODING()`` provides extra information: signedness,
>   char, or bool, for the int type. The char and bool encoding
>   are mostly useful for pretty print. At most one encoding can
>   be specified for the int type.
> @@ -161,8 +161,7 @@ The maximum value of ``BTF_INT_BITS()`` is 128.
>   
>   The ``BTF_INT_OFFSET()`` specifies the starting bit offset to
>   calculate values for this int. For example, a bitfield struct
> -member has
> -
> +member has:
>    * btf member bit offset 100 from the start of the structure,
>    * btf member pointing to an int type,
>    * the int type has ``BTF_INT_OFFSET() = 2`` and ``BTF_INT_BITS() = 4``
> @@ -179,7 +178,7 @@ access the same bits as the above:
>   
>   The original intention of ``BTF_INT_OFFSET()`` is to provide
>   flexibility of bitfield encoding.
> -Currently, both llvm and pahole generates ``BTF_INT_OFFSET() = 0``
> +Currently, both llvm and pahole generate ``BTF_INT_OFFSET() = 0``
>   for all int types.
>   
>   2.2.2 BTF_KIND_PTR
> @@ -204,7 +203,7 @@ No additional type data follow ``btf_type``.
>     * ``info.vlen``: 0
>     * ``size/type``: 0, not used
>   
> -btf_type is followed by one "struct btf_array"::
> +``btf_type`` is followed by one ``struct btf_array``::
>   
>       struct btf_array {
>           __u32   type;
> @@ -217,27 +216,26 @@ The ``struct btf_array`` encoding:
>     * ``index_type``: the index type
>     * ``nelems``: the number of elements for this array (``0`` is also allowed).
>   
> -The ``index_type`` can be any regular int types
> -(u8, u16, u32, u64, unsigned __int128).
> -The original design of including ``index_type`` follows dwarf
> -which has a ``index_type`` for its array type.
> +The ``index_type`` can be any regular int type
> +(``u8``, ``u16``, ``u32``, ``u64``, ``unsigned __int128``).
> +The original design of including ``index_type`` follows DWARF,
> +which has an ``index_type`` for its array type.
>   Currently in BTF, beyond type verification, the ``index_type`` is not used.
>   
>   The ``struct btf_array`` allows chaining through element type to represent
> -multiple dimensional arrays. For example, ``int a[5][6]``, the following
> -type system illustrates the chaining:
> +multidimensional arrays. For example, for ``int a[5][6]``, the following
> +type information illustrates the chaining:
>   
>     * [1]: int
>     * [2]: array, ``btf_array.type = [1]``, ``btf_array.nelems = 6``
>     * [3]: array, ``btf_array.type = [2]``, ``btf_array.nelems = 5``
>   
> -Currently, both pahole and llvm collapse multiple dimensional array
> -into one dimensional array, e.g., ``a[5][6]``, the btf_array.nelems
> -equal to ``30``. This is because the original use case is map pretty
> -print where the whole array is dumped out so one dimensional array
> +Currently, both pahole and llvm collapse multidimensional array
> +into one-dimensional array, e.g., for ``a[5][6]``, the ``btf_array.nelems``
> +is equal to ``30``. This is because the original use case is map pretty
> +print where the whole array is dumped out so one-dimensional array
>   is enough. As more BTF usage is explored, pahole and llvm can be
> -changed to generate proper chained representation for
> -multiple dimensional arrays.
> +changed to generate proper chained representation for multidimensional arrays.
>   
>   2.2.4 BTF_KIND_STRUCT
>   ~~~~~~~~~~~~~~~~~~~~~
> @@ -382,7 +380,7 @@ No additional type data follow ``btf_type``.
>   
>   No additional type data follow ``btf_type``.
>   
> -A BTF_KIND_FUNC defines, not a type, but a subprogram (function) whose
> +A BTF_KIND_FUNC defines not a type, but a subprogram (function) whose
>   signature is defined by ``type``. The subprogram is thus an instance of
>   that type. The BTF_KIND_FUNC may in turn be referenced by a func_info in
>   the :ref:`BTF_Ext_Section` (ELF) or in the arguments to
> @@ -459,10 +457,10 @@ The workflow typically looks like:
>   3.1 BPF_BTF_LOAD
>   ================
>   
> -Load a blob of BTF data into kernel. A blob of data
> -described in :ref:`BTF_Type_String`
> +Load a blob of BTF data into kernel. A blob of data,
> +described in :ref:`BTF_Type_String`,
>   can be directly loaded into the kernel.
> -A ``btf_fd`` returns to userspace.
> +A ``btf_fd`` is returned to a userspace.
>   
>   3.2 BPF_MAP_CREATE
>   ==================
> @@ -487,7 +485,7 @@ In libbpf, the map can be defined with extra annotation like below:
>   Here, the parameters for macro BPF_ANNOTATE_KV_PAIR are map name,
>   key and value types for the map.
>   During ELF parsing, libbpf is able to extract key/value type_id's
> -and assigned them to BPF_MAP_CREATE attributes automatically.
> +and assign them to BPF_MAP_CREATE attributes automatically.
>   
>   .. _BPF_Prog_Load:
>   
> @@ -532,7 +530,7 @@ Below are requirements for func_info:
>       bpf func boundaries.
>   
>   Below are requirements for line_info:
> -  * the first insn in each func must points to a line_info record.
> +  * the first insn in each func must have a line_info record pointing to it.
>     * the line_info insn_off is in strictly increasing order.
>   
>   For line_info, the line number and column number are defined as below:
> @@ -544,26 +542,26 @@ For line_info, the line number and column number are defined as below:
>   3.4 BPF_{PROG,MAP}_GET_NEXT_ID
>   
>   In kernel, every loaded program, map or btf has a unique id.
> -The id won't change during the life time of the program, map or btf.
> +The id won't change during the lifetime of a program, map, or btf.
>   
>   The bpf syscall command BPF_{PROG,MAP}_GET_NEXT_ID
>   returns all id's, one for each command, to user space, for bpf
> -program or maps,
> -so the inspection tool can inspect all programs and maps.
> +program or maps, respectively,
> +so an inspection tool can inspect all programs and maps.
>   
>   3.5 BPF_{PROG,MAP}_GET_FD_BY_ID
>   
> -The introspection tool cannot use id to get details about program or maps.
> -A file descriptor needs to be obtained first for reference counting purpose.
> +An introspection tool cannot use id to get details about program or maps.
> +A file descriptor needs to be obtained first for reference-counting purpose.
>   
>   3.6 BPF_OBJ_GET_INFO_BY_FD
>   ==========================
>   
> -Once a program/map fd is acquired, the introspection tool can
> +Once a program/map fd is acquired, an introspection tool can
>   get the detailed information from kernel about this fd,
> -some of which is btf related. For example,
> -``bpf_map_info`` returns ``btf_id``, key/value type id.
> -``bpf_prog_info`` returns ``btf_id``, func_info and line info
> +some of which are BTF-related. For example,
> +``bpf_map_info`` returns ``btf_id`` and key/value type ids.
> +``bpf_prog_info`` returns ``btf_id``, func_info, and line info
>   for translated bpf byte codes, and jited_line_info.
>   
>   3.7 BPF_BTF_GET_FD_BY_ID
> @@ -574,9 +572,9 @@ bpf syscall command BPF_BTF_GET_FD_BY_ID can retrieve a btf fd.
>   Then, with command BPF_OBJ_GET_INFO_BY_FD, the btf blob, originally
>   loaded into the kernel with BPF_BTF_LOAD, can be retrieved.
>   
> -With the btf blob, ``bpf_map_info`` and ``bpf_prog_info``, the introspection
> +With the btf blob, ``bpf_map_info``, and ``bpf_prog_info``, an introspection
>   tool has full btf knowledge and is able to pretty print map key/values,
> -dump func signatures, dump line info along with byte/jit codes.
> +dump func signatures and line info, along with byte/jit codes.
>   
>   4. ELF File Format Interface
>   ****************************
> @@ -625,8 +623,8 @@ The func_info is organized as below.::
>        ...
>   
>   ``func_info_rec_size`` specifies the size of ``bpf_func_info`` structure
> -when .BTF.ext is generated. btf_ext_info_sec, defined below, is
> -the func_info for each specific ELF section.::
> +when .BTF.ext is generated. ``btf_ext_info_sec``, defined below, is
> +a collection of func_info for each specific ELF section.::
>   
>        struct btf_ext_info_sec {
>           __u32   sec_name_off; /* offset to section name */
> @@ -661,7 +659,7 @@ from the beginning of section (``btf_ext_info_sec->sec_name_off``).
>   
>   With BTF, the map key/value can be printed based on fields rather than
>   simply raw bytes. This is especially
> -valuable for large structure or if you data structure
> +valuable for large structure or if your data structure
>   has bitfields. For example, for the following map,::
>   
>         enum A { A1, A2, A3, A4, A5 };
> @@ -702,8 +700,8 @@ bpftool is able to pretty print like below:
>   5.2 bpftool prog dump
>   =====================
>   
> -The following is an example to show func_info and line_info
> -can help prog dump with better kernel symbol name, function prototype
> +The following is an example showing how func_info and line_info
> +can help prog dump with better kernel symbol names, function prototypes
>   and line information.::
>   
>       $ bpftool prog dump jited pinned /sys/fs/bpf/test_btf_haskv
> @@ -733,10 +731,10 @@ and line information.::
>       ; counts = bpf_map_lookup_elem(&btf_map, &key);
>       [...]
>   
> -5.3 verifier log
> +5.3 Verifier Log
>   ================
>   
> -The following is an example how line_info can help verifier failure debug.::
> +The following is an example of how line_info can help debugging verification failure.::
>   
>          /* The code at tools/testing/selftests/bpf/test_xdp_noinline.c
>           * is modified as below.
> @@ -867,4 +865,4 @@ The assembly code (-S) is able to show the BTF encoding in assembly format.::
>   7. Testing
>   **********
>   
> -Kernel bpf selftest `test_btf.c` provides extensive set of BTF related tests.
> +Kernel bpf selftest `test_btf.c` provides extensive set of BTF-related tests.
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 3/4] bpf: fix formatting, typos, reflow comments in syscall.c, verifier.c
  2019-02-28 22:40   ` Song Liu
@ 2019-03-01  0:27     ` Daniel Borkmann
  2019-03-01  0:28       ` Daniel Borkmann
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Borkmann @ 2019-03-01  0:27 UTC (permalink / raw)
  To: Song Liu, Andrii Nakryiko; +Cc: Andrii Nakryiko, ast, yhs, Networking, bpf

On 02/28/2019 11:40 PM, Song Liu wrote:
> On Thu, Feb 28, 2019 at 10:59 AM Andrii Nakryiko <andriin@fb.com> wrote:
>>
>> Fix few formatting errors. Fix few typos and reflow long descriptive
>> comments for more even text fill.
>>
>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> 
> I think we should not change the code for formatting, as these changes make
> git-blame more difficult. How about we only make changes to comments?

Kind of true, though git blame is imho not too useful (unless I'm missing
some git magic :)); git log -p has much more historical context to search
through. I think these cleanups below are okay, though my worry is that
for someone doing backports changes that are scattered like this mainly
cause conflicts which then gets painful to resolve. I think we could make
it work if they are split and more isolated, e.g. syscall ones could be
one commit, another one only touching the big contiguous verifier comment,
and so on.

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 3/4] bpf: fix formatting, typos, reflow comments in syscall.c, verifier.c
  2019-03-01  0:27     ` Daniel Borkmann
@ 2019-03-01  0:28       ` Daniel Borkmann
  2019-03-01  0:52         ` Andrii Nakryiko
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Borkmann @ 2019-03-01  0:28 UTC (permalink / raw)
  To: Song Liu, Andrii Nakryiko; +Cc: Andrii Nakryiko, ast, yhs, Networking, bpf

On 03/01/2019 01:27 AM, Daniel Borkmann wrote:
> On 02/28/2019 11:40 PM, Song Liu wrote:
>> On Thu, Feb 28, 2019 at 10:59 AM Andrii Nakryiko <andriin@fb.com> wrote:
>>>
>>> Fix few formatting errors. Fix few typos and reflow long descriptive
>>> comments for more even text fill.
>>>
>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>
>> I think we should not change the code for formatting, as these changes make
>> git-blame more difficult. How about we only make changes to comments?
> 
> Kind of true, though git blame is imho not too useful (unless I'm missing
> some git magic :)); git log -p has much more historical context to search
> through. I think these cleanups below are okay, though my worry is that
> for someone doing backports changes that are scattered like this mainly
> cause conflicts which then gets painful to resolve. I think we could make
> it work if they are split and more isolated, e.g. syscall ones could be
> one commit, another one only touching the big contiguous verifier comment,
> and so on.

(Ideally also separate from the doc-only patches which is the rest of your
 series.)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 3/4] bpf: fix formatting, typos, reflow comments in syscall.c, verifier.c
  2019-03-01  0:28       ` Daniel Borkmann
@ 2019-03-01  0:52         ` Andrii Nakryiko
  0 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2019-03-01  0:52 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Song Liu, Andrii Nakryiko, Alexei Starovoitov, Yonghong Song,
	Networking, bpf

On Thu, Feb 28, 2019 at 4:28 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 03/01/2019 01:27 AM, Daniel Borkmann wrote:
> > On 02/28/2019 11:40 PM, Song Liu wrote:
> >> On Thu, Feb 28, 2019 at 10:59 AM Andrii Nakryiko <andriin@fb.com> wrote:
> >>>
> >>> Fix few formatting errors. Fix few typos and reflow long descriptive
> >>> comments for more even text fill.
> >>>
> >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> >>
> >> I think we should not change the code for formatting, as these changes make
> >> git-blame more difficult. How about we only make changes to comments?
> >
> > Kind of true, though git blame is imho not too useful (unless I'm missing
> > some git magic :)); git log -p has much more historical context to search
> > through. I think these cleanups below are okay, though my worry is that
> > for someone doing backports changes that are scattered like this mainly
> > cause conflicts which then gets painful to resolve. I think we could make
> > it work if they are split and more isolated, e.g. syscall ones could be
> > one commit, another one only touching the big contiguous verifier comment,
> > and so on.
>
> (Ideally also separate from the doc-only patches which is the rest of your
>  series.)

Yep, sure, I'll split this into two patchsets and this patch into
multiple patches (as part of second patchset).

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-03-01  0:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-28 18:45 [PATCH bpf-next 0/4] bpf docs and comments typo and formatting fixes Andrii Nakryiko
2019-02-28 18:45 ` [PATCH bpf-next 1/4] docs/btf: fix typos, improve wording Andrii Nakryiko
2019-02-28 23:47   ` Yonghong Song
2019-02-28 18:45 ` [PATCH bpf-next 2/4] docs/btf: reflow text to fill up to 78 characters Andrii Nakryiko
2019-02-28 18:45 ` [PATCH bpf-next 3/4] bpf: fix formatting, typos, reflow comments in syscall.c, verifier.c Andrii Nakryiko
2019-02-28 22:40   ` Song Liu
2019-03-01  0:27     ` Daniel Borkmann
2019-03-01  0:28       ` Daniel Borkmann
2019-03-01  0:52         ` Andrii Nakryiko
2019-02-28 18:45 ` [PATCH bpf-next 4/4] docs/bpf: minor fixes Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).