| Message ID | 20230325010845.46000-1-inwardvessel@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f1cb927cdb6280e0ce283174cc1bf395dffc43d4 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] libbpf: synchronize access to print function pointer | expand |
On 03/24, JP Kobryn wrote: > This patch prevents races on the print function pointer, allowing the > libbpf_set_print() function to become thread safe. Why does it have to be thread-safe? The rest of the APIs aren't, so why can't use solve it on your side by wrapping those calls with a mutex? (is there some context I'm missing?) > Signed-off-by: JP Kobryn <inwardvessel@gmail.com> > --- > tools/lib/bpf/libbpf.c | 9 ++++++--- > tools/lib/bpf/libbpf.h | 3 +++ > 2 files changed, 9 insertions(+), 3 deletions(-) > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index f6a071db5c6e..15737d7b5a28 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -216,9 +216,10 @@ static libbpf_print_fn_t __libbpf_pr = __base_pr; > libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn) > { > - libbpf_print_fn_t old_print_fn = __libbpf_pr; > + libbpf_print_fn_t old_print_fn; > + > + old_print_fn = __atomic_exchange_n(&__libbpf_pr, fn, __ATOMIC_RELAXED); > - __libbpf_pr = fn; > return old_print_fn; > } > @@ -227,8 +228,10 @@ void libbpf_print(enum libbpf_print_level level, > const char *format, ...) > { > va_list args; > int old_errno; > + libbpf_print_fn_t print_fn; > - if (!__libbpf_pr) > + print_fn = __atomic_load_n(&__libbpf_pr, __ATOMIC_RELAXED); > + if (!print_fn) > return; > old_errno = errno; > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index 1615e55e2e79..4478809ff9ca 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -99,6 +99,9 @@ typedef int (*libbpf_print_fn_t)(enum > libbpf_print_level level, > /** > * @brief **libbpf_set_print()** sets user-provided log callback > function to > * be used for libbpf warnings and informational messages. > + * > + * This function is thread safe. > + * > * @param fn The log print function. If NULL, libbpf won't print > anything. > * @return Pointer to old print function. > */ > -- > 2.39.2
On Fri, Mar 24, 2023 at 8:09 PM Stanislav Fomichev <sdf@google.com> wrote: > > On 03/24, JP Kobryn wrote: > > This patch prevents races on the print function pointer, allowing the > > libbpf_set_print() function to become thread safe. > > Why does it have to be thread-safe? The rest of the APIs aren't, so It doesn't have to, but if it can be made thread-safe trivially, it probably should be. Rust users are especially sensitive to this (see discussion in [0] for example). Generally speaking, yep, libbpf APIs are not thread-safe by default (we don't do explicit locking anywhere inside libbpf), but there are a bunch of APIs that are inherently thread-safe as they are stateless (like feature probing, string conversion APIs, I suspect upon inspection we'll see that all bpf_program__attach_*() APIs are also probably thread-safe already). So we should start marking them as such to avoid confusion and uncertainty for users. I'd also like to document somewhere that two independent bpf_objects can be used safely on two separate threads, because whatever state they are sharing (like feature detection cache) is designed in such a way to be thread-safe and shareable with no locking. [0] https://github.com/libbpf/libbpf-rs/pull/374#issuecomment-1462565778 > why can't use solve it on your side by wrapping those calls with a > mutex? It would be very unfortunate to wrap libbpf_set_print and *all other libbpf API* in mutex. > > (is there some context I'm missing?) > > > Signed-off-by: JP Kobryn <inwardvessel@gmail.com> > > --- > > tools/lib/bpf/libbpf.c | 9 ++++++--- > > tools/lib/bpf/libbpf.h | 3 +++ > > 2 files changed, 9 insertions(+), 3 deletions(-) > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index f6a071db5c6e..15737d7b5a28 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -216,9 +216,10 @@ static libbpf_print_fn_t __libbpf_pr = __base_pr; > > > libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn) > > { > > - libbpf_print_fn_t old_print_fn = __libbpf_pr; > > + libbpf_print_fn_t old_print_fn; > > + > > + old_print_fn = __atomic_exchange_n(&__libbpf_pr, fn, __ATOMIC_RELAXED); > > > - __libbpf_pr = fn; > > return old_print_fn; > > } > > > @@ -227,8 +228,10 @@ void libbpf_print(enum libbpf_print_level level, > > const char *format, ...) > > { > > va_list args; > > int old_errno; > > + libbpf_print_fn_t print_fn; > > > - if (!__libbpf_pr) > > + print_fn = __atomic_load_n(&__libbpf_pr, __ATOMIC_RELAXED); > > + if (!print_fn) > > return; > > > old_errno = errno; > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > > index 1615e55e2e79..4478809ff9ca 100644 > > --- a/tools/lib/bpf/libbpf.h > > +++ b/tools/lib/bpf/libbpf.h > > @@ -99,6 +99,9 @@ typedef int (*libbpf_print_fn_t)(enum > > libbpf_print_level level, > > /** > > * @brief **libbpf_set_print()** sets user-provided log callback > > function to > > * be used for libbpf warnings and informational messages. > > + * > > + * This function is thread safe. > > + * > > * @param fn The log print function. If NULL, libbpf won't print > > anything. > > * @return Pointer to old print function. > > */ > > -- > > 2.39.2 >
On Fri, Mar 24, 2023 at 6:09 PM JP Kobryn <inwardvessel@gmail.com> wrote: > > This patch prevents races on the print function pointer, allowing the > libbpf_set_print() function to become thread safe. > > Signed-off-by: JP Kobryn <inwardvessel@gmail.com> > --- I reworked the patch subject to "libbpf: Ensure print callback usage is thread-safe", it felt more on point. Also changed to "thread-safe", which seems to be a proper spelling for this term. Applied to bpf-next, thanks! > tools/lib/bpf/libbpf.c | 9 ++++++--- > tools/lib/bpf/libbpf.h | 3 +++ > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index f6a071db5c6e..15737d7b5a28 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -216,9 +216,10 @@ static libbpf_print_fn_t __libbpf_pr = __base_pr; > > libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn) > { > - libbpf_print_fn_t old_print_fn = __libbpf_pr; > + libbpf_print_fn_t old_print_fn; > + > + old_print_fn = __atomic_exchange_n(&__libbpf_pr, fn, __ATOMIC_RELAXED); > > - __libbpf_pr = fn; > return old_print_fn; > } > > @@ -227,8 +228,10 @@ void libbpf_print(enum libbpf_print_level level, const char *format, ...) > { > va_list args; > int old_errno; > + libbpf_print_fn_t print_fn; > > - if (!__libbpf_pr) > + print_fn = __atomic_load_n(&__libbpf_pr, __ATOMIC_RELAXED); > + if (!print_fn) > return; > > old_errno = errno; > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index 1615e55e2e79..4478809ff9ca 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -99,6 +99,9 @@ typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level, > /** > * @brief **libbpf_set_print()** sets user-provided log callback function to > * be used for libbpf warnings and informational messages. > + * > + * This function is thread safe. > + * I moved this part to after @return spec, where details about functions are normally put. > * @param fn The log print function. If NULL, libbpf won't print anything. > * @return Pointer to old print function. > */ > -- > 2.39.2 >
Hello: This patch was applied to bpf/bpf-next.git (master) by Andrii Nakryiko <andrii@kernel.org>: On Fri, 24 Mar 2023 18:08:45 -0700 you wrote: > This patch prevents races on the print function pointer, allowing the > libbpf_set_print() function to become thread safe. > > Signed-off-by: JP Kobryn <inwardvessel@gmail.com> > --- > tools/lib/bpf/libbpf.c | 9 ++++++--- > tools/lib/bpf/libbpf.h | 3 +++ > 2 files changed, 9 insertions(+), 3 deletions(-) Here is the summary with links: - [bpf-next] libbpf: synchronize access to print function pointer https://git.kernel.org/bpf/bpf-next/c/f1cb927cdb62 You are awesome, thank you!
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index f6a071db5c6e..15737d7b5a28 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -216,9 +216,10 @@ static libbpf_print_fn_t __libbpf_pr = __base_pr; libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn) { - libbpf_print_fn_t old_print_fn = __libbpf_pr; + libbpf_print_fn_t old_print_fn; + + old_print_fn = __atomic_exchange_n(&__libbpf_pr, fn, __ATOMIC_RELAXED); - __libbpf_pr = fn; return old_print_fn; } @@ -227,8 +228,10 @@ void libbpf_print(enum libbpf_print_level level, const char *format, ...) { va_list args; int old_errno; + libbpf_print_fn_t print_fn; - if (!__libbpf_pr) + print_fn = __atomic_load_n(&__libbpf_pr, __ATOMIC_RELAXED); + if (!print_fn) return; old_errno = errno; diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 1615e55e2e79..4478809ff9ca 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -99,6 +99,9 @@ typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level, /** * @brief **libbpf_set_print()** sets user-provided log callback function to * be used for libbpf warnings and informational messages. + * + * This function is thread safe. + * * @param fn The log print function. If NULL, libbpf won't print anything. * @return Pointer to old print function. */
This patch prevents races on the print function pointer, allowing the libbpf_set_print() function to become thread safe. Signed-off-by: JP Kobryn <inwardvessel@gmail.com> --- tools/lib/bpf/libbpf.c | 9 ++++++--- tools/lib/bpf/libbpf.h | 3 +++ 2 files changed, 9 insertions(+), 3 deletions(-)