* [PATCH bpf-next] libbpf: synchronize access to print function pointer
@ 2023-03-25 1:08 JP Kobryn
2023-03-25 3:09 ` Stanislav Fomichev
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: JP Kobryn @ 2023-03-25 1:08 UTC (permalink / raw)
To: bpf, andrii; +Cc: kernel-team, inwardvessel
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(-)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: synchronize access to print function pointer
2023-03-25 1:08 [PATCH bpf-next] libbpf: synchronize access to print function pointer JP Kobryn
@ 2023-03-25 3:09 ` Stanislav Fomichev
2023-03-27 18:32 ` Andrii Nakryiko
2023-03-27 18:36 ` Andrii Nakryiko
2023-03-27 18:40 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 5+ messages in thread
From: Stanislav Fomichev @ 2023-03-25 3:09 UTC (permalink / raw)
To: JP Kobryn; +Cc: bpf, andrii, kernel-team
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: synchronize access to print function pointer
2023-03-25 3:09 ` Stanislav Fomichev
@ 2023-03-27 18:32 ` Andrii Nakryiko
0 siblings, 0 replies; 5+ messages in thread
From: Andrii Nakryiko @ 2023-03-27 18:32 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: JP Kobryn, bpf, andrii, kernel-team
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: synchronize access to print function pointer
2023-03-25 1:08 [PATCH bpf-next] libbpf: synchronize access to print function pointer JP Kobryn
2023-03-25 3:09 ` Stanislav Fomichev
@ 2023-03-27 18:36 ` Andrii Nakryiko
2023-03-27 18:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: Andrii Nakryiko @ 2023-03-27 18:36 UTC (permalink / raw)
To: JP Kobryn; +Cc: bpf, andrii, kernel-team
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] libbpf: synchronize access to print function pointer
2023-03-25 1:08 [PATCH bpf-next] libbpf: synchronize access to print function pointer JP Kobryn
2023-03-25 3:09 ` Stanislav Fomichev
2023-03-27 18:36 ` Andrii Nakryiko
@ 2023-03-27 18:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-03-27 18:40 UTC (permalink / raw)
To: JP Kobryn; +Cc: bpf, andrii, kernel-team
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-27 18:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-25 1:08 [PATCH bpf-next] libbpf: synchronize access to print function pointer JP Kobryn
2023-03-25 3:09 ` Stanislav Fomichev
2023-03-27 18:32 ` Andrii Nakryiko
2023-03-27 18:36 ` Andrii Nakryiko
2023-03-27 18:40 ` patchwork-bot+netdevbpf
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).