| Message ID | 20211115191840.496263-4-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Fixes for kfunc-mod regressions and warnings | expand |
On Mon, Nov 15, 2021 at 11:18 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > resolve_btfids prints a warning when it finds an unresolved symbol, > (id == 0) in id_patch. This can be the case for BTF sets that are empty > (due to disabled config options), hence printing warnings for certain > builds, most recently seen in [0]. Hence, demote the warning to debug > log level to avoid build time noise. > > [0]: https://lore.kernel.org/all/1b99ae14-abb4-d18f-cc6a-d7e523b25542@gmail.com > > Fixes: 0e32dfc80bae ("bpf: Enable TCP congestion control kfunc from modules") > Reported-by: Pavel Skripkin <paskripkin@gmail.com> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > tools/bpf/resolve_btfids/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > index a59cb0ee609c..833bfcc9ccf6 100644 > --- a/tools/bpf/resolve_btfids/main.c > +++ b/tools/bpf/resolve_btfids/main.c > @@ -569,7 +569,7 @@ static int id_patch(struct object *obj, struct btf_id *id) > int i; > > if (!id->id) { > - pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name); > + pr_debug("WARN: resolve_btfids: unresolved symbol %s\n", id->name); This is an important warning that helps detect some misconfiguration, we cannot just hide it. If it really is happening for empty lists (why?), we should handle empty lists better, but not hide the warning. > } > > for (i = 0; i < id->addr_cnt; i++) { > -- > 2.33.1 >
On Wed, Nov 17, 2021 at 10:50:43AM IST, Andrii Nakryiko wrote: > On Mon, Nov 15, 2021 at 11:18 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > resolve_btfids prints a warning when it finds an unresolved symbol, > > (id == 0) in id_patch. This can be the case for BTF sets that are empty > > (due to disabled config options), hence printing warnings for certain > > builds, most recently seen in [0]. Hence, demote the warning to debug > > log level to avoid build time noise. > > > > [0]: https://lore.kernel.org/all/1b99ae14-abb4-d18f-cc6a-d7e523b25542@gmail.com > > > > Fixes: 0e32dfc80bae ("bpf: Enable TCP congestion control kfunc from modules") > > Reported-by: Pavel Skripkin <paskripkin@gmail.com> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > tools/bpf/resolve_btfids/main.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > > index a59cb0ee609c..833bfcc9ccf6 100644 > > --- a/tools/bpf/resolve_btfids/main.c > > +++ b/tools/bpf/resolve_btfids/main.c > > @@ -569,7 +569,7 @@ static int id_patch(struct object *obj, struct btf_id *id) > > int i; > > > > if (!id->id) { > > - pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name); > > + pr_debug("WARN: resolve_btfids: unresolved symbol %s\n", id->name); > > This is an important warning that helps detect some misconfiguration, > we cannot just hide it. If it really is happening for empty lists > (why?), we should handle empty lists better, but not hide the warning. > +Cc Jiri Sorry for the delay. The 'why' is because in case of empty BTF set, the id->cnt aliasing over id->id (for non-set __BTF_ID_*) is zero, so it trips over when it sees it as 0 for empty set in id_patch. So, what would be your opinion on how to handle this case? Ignore the warning case for empty sets specifically? (e.g. using string comparison in id_patch for id->name, or split id and cnt out of union and assign non-zero to id in case cnt == 0, otherwise copy (saving strncmp for all symbols in id_patch)) Also, if I'm respinning, any comments on the first two which are also fixes for bugs I introduced, so that we can avoid another round? -- Kartikeya
On Sat, Nov 20, 2021 at 12:46:58AM IST, Kumar Kartikeya Dwivedi wrote: > On Wed, Nov 17, 2021 at 10:50:43AM IST, Andrii Nakryiko wrote: > > On Mon, Nov 15, 2021 at 11:18 AM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > > > > > > resolve_btfids prints a warning when it finds an unresolved symbol, > > > (id == 0) in id_patch. This can be the case for BTF sets that are empty > > > (due to disabled config options), hence printing warnings for certain > > > builds, most recently seen in [0]. Hence, demote the warning to debug > > > log level to avoid build time noise. > > > > > > [0]: https://lore.kernel.org/all/1b99ae14-abb4-d18f-cc6a-d7e523b25542@gmail.com > > > > > > Fixes: 0e32dfc80bae ("bpf: Enable TCP congestion control kfunc from modules") > > > Reported-by: Pavel Skripkin <paskripkin@gmail.com> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > --- > > > tools/bpf/resolve_btfids/main.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > > > index a59cb0ee609c..833bfcc9ccf6 100644 > > > --- a/tools/bpf/resolve_btfids/main.c > > > +++ b/tools/bpf/resolve_btfids/main.c > > > @@ -569,7 +569,7 @@ static int id_patch(struct object *obj, struct btf_id *id) > > > int i; > > > > > > if (!id->id) { > > > - pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name); > > > + pr_debug("WARN: resolve_btfids: unresolved symbol %s\n", id->name); > > > > This is an important warning that helps detect some misconfiguration, > > we cannot just hide it. If it really is happening for empty lists > > (why?), we should handle empty lists better, but not hide the warning. > > > > +Cc Jiri > > Sorry for the delay. The 'why' is because in case of empty BTF set, the id->cnt > aliasing over id->id (for non-set __BTF_ID_*) is zero, so it trips over when it > sees it as 0 for empty set in id_patch. > > So, what would be your opinion on how to handle this case? Ignore the warning > case for empty sets specifically? (e.g. using string comparison in id_patch for > id->name, or split id and cnt out of union and assign non-zero to id in case cnt Ah, I realized after sending that actually would be far more work (and also confusing/error prone), since it is copied to ptr[idx] as id->id once in id_patch and then used as cnt for sorting in sets_patch, which would break qsort. Just recording that it is a set in btf_id struct might be better. -- Kartikeya
diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c index a59cb0ee609c..833bfcc9ccf6 100644 --- a/tools/bpf/resolve_btfids/main.c +++ b/tools/bpf/resolve_btfids/main.c @@ -569,7 +569,7 @@ static int id_patch(struct object *obj, struct btf_id *id) int i; if (!id->id) { - pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name); + pr_debug("WARN: resolve_btfids: unresolved symbol %s\n", id->name); } for (i = 0; i < id->addr_cnt; i++) {
resolve_btfids prints a warning when it finds an unresolved symbol, (id == 0) in id_patch. This can be the case for BTF sets that are empty (due to disabled config options), hence printing warnings for certain builds, most recently seen in [0]. Hence, demote the warning to debug log level to avoid build time noise. [0]: https://lore.kernel.org/all/1b99ae14-abb4-d18f-cc6a-d7e523b25542@gmail.com Fixes: 0e32dfc80bae ("bpf: Enable TCP congestion control kfunc from modules") Reported-by: Pavel Skripkin <paskripkin@gmail.com> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- tools/bpf/resolve_btfids/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)