| Message ID | 20221026025941.2621795-1-xukuohai@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: Fix a typo in comment for DFS algorithm | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 10 this patch: 10 |
| netdev/cc_maintainers | success | CCed 13 of 13 maintainers |
| netdev/build_clang | success | Errors and warnings before: 5 this patch: 5 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 10 this patch: 10 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-12 | fail | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
On Tue, Oct 25, 2022 at 7:42 PM Xu Kuohai <xukuohai@huaweicloud.com> wrote: > > From: Xu Kuohai <xukuohai@huawei.com> > > There is a typo in comment for DFS algorithm in bpf/verifier.c. The top > element should not be popped until all its neighbors have been checked. > Fix it. > > Fixes: 475fb78fbf48 ("bpf: verifier (add branch/goto checks)") > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > --- > kernel/bpf/verifier.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index b83a8d420520..96ba5ea6d1a6 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -10662,7 +10662,7 @@ static int check_return_code(struct bpf_verifier_env *env) > * 3 let S be a stack > * 4 S.push(v) > * 5 while S is not empty > - * 6 t <- S.pop() > + * 6 t <- S.top() Even with this fix the comment is not quite accurate. I wonder whether we should keep it or delete it completely. At least please use 'peek' instead of 'top'.
On Tue, Oct 25, 2022 at 11:32:55PM -0700, Alexei Starovoitov wrote: > On Tue, Oct 25, 2022 at 7:42 PM Xu Kuohai <xukuohai@huaweicloud.com> wrote: > > > > From: Xu Kuohai <xukuohai@huawei.com> > > > > There is a typo in comment for DFS algorithm in bpf/verifier.c. The top > > element should not be popped until all its neighbors have been checked. > > Fix it. > > > > Fixes: 475fb78fbf48 ("bpf: verifier (add branch/goto checks)") > > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > > --- > > kernel/bpf/verifier.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index b83a8d420520..96ba5ea6d1a6 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -10662,7 +10662,7 @@ static int check_return_code(struct bpf_verifier_env *env) > > * 3 let S be a stack > > * 4 S.push(v) > > * 5 while S is not empty > > - * 6 t <- S.pop() > > + * 6 t <- S.top() > > Even with this fix the comment is not quite accurate. > I wonder whether we should keep it or delete it completely. > At least please use 'peek' instead of 'top'. I think the comment should be in words (like other code comments in the kernel) instead.
On 10/26/2022 2:32 PM, Alexei Starovoitov wrote: > On Tue, Oct 25, 2022 at 7:42 PM Xu Kuohai <xukuohai@huaweicloud.com> wrote: >> >> From: Xu Kuohai <xukuohai@huawei.com> >> >> There is a typo in comment for DFS algorithm in bpf/verifier.c. The top >> element should not be popped until all its neighbors have been checked. >> Fix it. >> >> Fixes: 475fb78fbf48 ("bpf: verifier (add branch/goto checks)") >> Signed-off-by: Xu Kuohai <xukuohai@huawei.com> >> --- >> kernel/bpf/verifier.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index b83a8d420520..96ba5ea6d1a6 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -10662,7 +10662,7 @@ static int check_return_code(struct bpf_verifier_env *env) >> * 3 let S be a stack >> * 4 S.push(v) >> * 5 while S is not empty >> - * 6 t <- S.pop() >> + * 6 t <- S.top() > > Even with this fix the comment is not quite accurate. > I wonder whether we should keep it or delete it completely. The comment describes the non-recursive DFS algorithm used by the C code. Although it does not describe the full details, it helps us to understand the code, so I think it should be kept. > At least please use 'peek' instead of 'top'. OK.
On 10/26/2022 3:52 PM, Bagas Sanjaya wrote: > On Tue, Oct 25, 2022 at 11:32:55PM -0700, Alexei Starovoitov wrote: >> On Tue, Oct 25, 2022 at 7:42 PM Xu Kuohai <xukuohai@huaweicloud.com> wrote: >>> >>> From: Xu Kuohai <xukuohai@huawei.com> >>> >>> There is a typo in comment for DFS algorithm in bpf/verifier.c. The top >>> element should not be popped until all its neighbors have been checked. >>> Fix it. >>> >>> Fixes: 475fb78fbf48 ("bpf: verifier (add branch/goto checks)") >>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com> >>> --- >>> kernel/bpf/verifier.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>> index b83a8d420520..96ba5ea6d1a6 100644 >>> --- a/kernel/bpf/verifier.c >>> +++ b/kernel/bpf/verifier.c >>> @@ -10662,7 +10662,7 @@ static int check_return_code(struct bpf_verifier_env *env) >>> * 3 let S be a stack >>> * 4 S.push(v) >>> * 5 while S is not empty >>> - * 6 t <- S.pop() >>> + * 6 t <- S.top() >> >> Even with this fix the comment is not quite accurate. >> I wonder whether we should keep it or delete it completely. >> At least please use 'peek' instead of 'top'. > > I think the comment should be in words (like other code comments in the > kernel) instead. > The beginning of the comment already says this is a piece of pseudo code. And I don't think it's clearer to describe the algorithm in words than to describe it in pseudo code.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index b83a8d420520..96ba5ea6d1a6 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -10662,7 +10662,7 @@ static int check_return_code(struct bpf_verifier_env *env) * 3 let S be a stack * 4 S.push(v) * 5 while S is not empty - * 6 t <- S.pop() + * 6 t <- S.top() * 7 if t is what we're looking for: * 8 return t * 9 for all edges e in G.adjacentEdges(t) do