Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
branch trace from hardware (e.g. Intel LBR). To use the feature, the
user need to create perf_event with proper branch_record filtering
on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
Signed-off-by: Song Liu <[email protected]>
---
include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
kernel/bpf/trampoline.c | 3 ++-
kernel/trace/bpf_trace.c | 33 +++++++++++++++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
4 files changed, 79 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 791f31dd0abee..c986e6fad5bc0 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4877,6 +4877,27 @@ union bpf_attr {
* Get the struct pt_regs associated with **task**.
* Return
* A pointer to struct pt_regs.
+ *
+ * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
+ * Description
+ * Get branch trace from hardware engines like Intel LBR. The
+ * branch trace is taken soon after the trigger point of the
+ * BPF program, so it may contain some entries after the
+ * trigger point. The user need to filter these entries
+ * accordingly.
+ *
+ * The data is stored as struct perf_branch_entry into output
+ * buffer *entries*. *size* is the size of *entries* in bytes.
+ * *flags* is reserved for now and must be zero.
+ *
+ * Return
+ * On success, number of bytes written to *buf*. On error, a
+ * negative value.
+ *
+ * **-EINVAL** if arguments invalid or **size** not a multiple
+ * of **sizeof**\ (**struct perf_branch_entry**\ ).
+ *
+ * **-ENOENT** if architecture does not support branch records.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -5055,6 +5076,7 @@ union bpf_attr {
FN(get_func_ip), \
FN(get_attach_cookie), \
FN(task_pt_regs), \
+ FN(get_branch_snapshot), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index fe1e857324e66..39eaaff81953d 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -10,6 +10,7 @@
#include <linux/rcupdate_trace.h>
#include <linux/rcupdate_wait.h>
#include <linux/module.h>
+#include <linux/static_call.h>
/* dummy _ops. The verifier will operate on target program's ops. */
const struct bpf_verifier_ops bpf_extension_verifier_ops = {
@@ -526,7 +527,7 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
}
#define NO_START_TIME 1
-static u64 notrace bpf_prog_start_time(void)
+static __always_inline u64 notrace bpf_prog_start_time(void)
{
u64 start = NO_START_TIME;
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 8e2eb950aa829..1954bc113087c 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1017,6 +1017,37 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto_pe = {
.arg1_type = ARG_PTR_TO_CTX,
};
+BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
+{
+#ifndef CONFIG_X86
+ return -ENOENT;
+#else
+ static const u32 br_entry_size = sizeof(struct perf_branch_entry);
+ u32 entry_cnt = size / br_entry_size;
+
+ if (unlikely(flags))
+ return -EINVAL;
+
+ if (!buf || (size % br_entry_size != 0))
+ return -EINVAL;
+
+ entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
+
+ if (!entry_cnt)
+ return -ENOENT;
+
+ return entry_cnt * br_entry_size;
+#endif
+}
+
+static const struct bpf_func_proto bpf_get_branch_snapshot_proto = {
+ .func = bpf_get_branch_snapshot,
+ .gpl_only = true,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_UNINIT_MEM,
+ .arg2_type = ARG_CONST_SIZE_OR_ZERO,
+};
+
static const struct bpf_func_proto *
bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
{
@@ -1132,6 +1163,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_snprintf_proto;
case BPF_FUNC_get_func_ip:
return &bpf_get_func_ip_proto_tracing;
+ case BPF_FUNC_get_branch_snapshot:
+ return &bpf_get_branch_snapshot_proto;
default:
return bpf_base_func_proto(func_id);
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 791f31dd0abee..c986e6fad5bc0 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4877,6 +4877,27 @@ union bpf_attr {
* Get the struct pt_regs associated with **task**.
* Return
* A pointer to struct pt_regs.
+ *
+ * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
+ * Description
+ * Get branch trace from hardware engines like Intel LBR. The
+ * branch trace is taken soon after the trigger point of the
+ * BPF program, so it may contain some entries after the
+ * trigger point. The user need to filter these entries
+ * accordingly.
+ *
+ * The data is stored as struct perf_branch_entry into output
+ * buffer *entries*. *size* is the size of *entries* in bytes.
+ * *flags* is reserved for now and must be zero.
+ *
+ * Return
+ * On success, number of bytes written to *buf*. On error, a
+ * negative value.
+ *
+ * **-EINVAL** if arguments invalid or **size** not a multiple
+ * of **sizeof**\ (**struct perf_branch_entry**\ ).
+ *
+ * **-ENOENT** if architecture does not support branch records.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -5055,6 +5076,7 @@ union bpf_attr {
FN(get_func_ip), \
FN(get_attach_cookie), \
FN(task_pt_regs), \
+ FN(get_branch_snapshot), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
--
2.30.2
Song Liu wrote:
> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
> branch trace from hardware (e.g. Intel LBR). To use the feature, the
> user need to create perf_event with proper branch_record filtering
> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
>
> Signed-off-by: Song Liu <[email protected]>
> ---
[...]
>
> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
> +{
> +#ifndef CONFIG_X86
> + return -ENOENT;
> +#else
> + static const u32 br_entry_size = sizeof(struct perf_branch_entry);
> + u32 entry_cnt = size / br_entry_size;
> +
> + if (unlikely(flags))
> + return -EINVAL;
> +
> + if (!buf || (size % br_entry_size != 0))
> + return -EINVAL;
LGTM, but why fail if buffer is slightly larger than expected? I guess its a slightly
buggy program that would do this, but not actually harmful right?
Acked-by: John Fastabend <[email protected]>
> On Sep 2, 2021, at 1:56 PM, John Fastabend <[email protected]> wrote:
>
> Song Liu wrote:
>> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
>> branch trace from hardware (e.g. Intel LBR). To use the feature, the
>> user need to create perf_event with proper branch_record filtering
>> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
>> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
>>
>> Signed-off-by: Song Liu <[email protected]>
>> ---
>
> [...]
>
>>
>> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
>> +{
>> +#ifndef CONFIG_X86
>> + return -ENOENT;
>> +#else
>> + static const u32 br_entry_size = sizeof(struct perf_branch_entry);
>> + u32 entry_cnt = size / br_entry_size;
>> +
>> + if (unlikely(flags))
>> + return -EINVAL;
>> +
>> + if (!buf || (size % br_entry_size != 0))
>> + return -EINVAL;
>
> LGTM, but why fail if buffer is slightly larger than expected? I guess its a slightly
> buggy program that would do this, but not actually harmful right?
This check was added because bpf_read_branch_records() has a similar check.
I guess it is OK either way.
>
> Acked-by: John Fastabend <[email protected]>
Thanks for the review!
On Thu, Sep 2, 2021 at 9:58 AM Song Liu <[email protected]> wrote:
>
> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
> branch trace from hardware (e.g. Intel LBR). To use the feature, the
> user need to create perf_event with proper branch_record filtering
> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
>
> Signed-off-by: Song Liu <[email protected]>
> ---
> include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
> kernel/bpf/trampoline.c | 3 ++-
> kernel/trace/bpf_trace.c | 33 +++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
> 4 files changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 791f31dd0abee..c986e6fad5bc0 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -4877,6 +4877,27 @@ union bpf_attr {
> * Get the struct pt_regs associated with **task**.
> * Return
> * A pointer to struct pt_regs.
> + *
> + * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
> + * Description
> + * Get branch trace from hardware engines like Intel LBR. The
> + * branch trace is taken soon after the trigger point of the
> + * BPF program, so it may contain some entries after the
This part is a leftover from previous design, so not relevant anymore?
> + * trigger point. The user need to filter these entries
> + * accordingly.
> + *
> + * The data is stored as struct perf_branch_entry into output
> + * buffer *entries*. *size* is the size of *entries* in bytes.
> + * *flags* is reserved for now and must be zero.
> + *
> + * Return
> + * On success, number of bytes written to *buf*. On error, a
> + * negative value.
> + *
> + * **-EINVAL** if arguments invalid or **size** not a multiple
> + * of **sizeof**\ (**struct perf_branch_entry**\ ).
> + *
> + * **-ENOENT** if architecture does not support branch records.
[...]
On Thu, Sep 2, 2021 at 9:58 AM Song Liu <[email protected]> wrote:
>
> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
> +{
> +#ifndef CONFIG_X86
> + return -ENOENT;
> +#else
> + static const u32 br_entry_size = sizeof(struct perf_branch_entry);
> + u32 entry_cnt = size / br_entry_size;
> +
> + if (unlikely(flags))
> + return -EINVAL;
> +
> + if (!buf || (size % br_entry_size != 0))
> + return -EINVAL;
> +
> + entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
Not taken branches will not be counted even with PERF_SAMPLE_BRANCH_ANY,
right?
Probably the first unlikely(flags) will be a fallthrough in asm.
Maybe worth adding unlikely to 2nd condition as well to make
sure that the compiler will generate default fallthrough code for it ?
So both will not appear in lbr entries?
Or maybe do:
if (unlikely(!buf))
return -EINVAL;
entry_cnt = static_call
if (size % br_entry_size)
return -EINVAL;
The lbr trace will be collected anyway.
If there are jmps in lbr due to earlier "if"s we can move them
after static_call. Like if (unlikely(flags) can be done afterwards too.
Bigger bang for the buck would be static-inline-ing migrate_disable, though.
> On Sep 2, 2021, at 3:53 PM, Andrii Nakryiko <[email protected]> wrote:
>
> On Thu, Sep 2, 2021 at 9:58 AM Song Liu <[email protected]> wrote:
>>
>> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
>> branch trace from hardware (e.g. Intel LBR). To use the feature, the
>> user need to create perf_event with proper branch_record filtering
>> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
>> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
>>
>> Signed-off-by: Song Liu <[email protected]>
>> ---
>> include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
>> kernel/bpf/trampoline.c | 3 ++-
>> kernel/trace/bpf_trace.c | 33 +++++++++++++++++++++++++++++++++
>> tools/include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
>> 4 files changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 791f31dd0abee..c986e6fad5bc0 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -4877,6 +4877,27 @@ union bpf_attr {
>> * Get the struct pt_regs associated with **task**.
>> * Return
>> * A pointer to struct pt_regs.
>> + *
>> + * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
>> + * Description
>> + * Get branch trace from hardware engines like Intel LBR. The
>> + * branch trace is taken soon after the trigger point of the
>> + * BPF program, so it may contain some entries after the
>
> This part is a leftover from previous design, so not relevant anymore?
Hmm.. This is still relevant, but not very accurate. I guess we should
provide more information, like "For more information about branches before
the trigger point, this should be called early in the BPF program".
Song
>
>> + * trigger point. The user need to filter these entries
>> + * accordingly.
>> + *
>> + * The data is stored as struct perf_branch_entry into output
>> + * buffer *entries*. *size* is the size of *entries* in bytes.
>> + * *flags* is reserved for now and must be zero.
>> + *
>> + * Return
>> + * On success, number of bytes written to *buf*. On error, a
>> + * negative value.
>> + *
>> + * **-EINVAL** if arguments invalid or **size** not a multiple
>> + * of **sizeof**\ (**struct perf_branch_entry**\ ).
>> + *
>> + * **-ENOENT** if architecture does not support branch records.
>
> [...]
On Thu, Sep 2, 2021 at 4:03 PM Song Liu <[email protected]> wrote:
>
>
>
> > On Sep 2, 2021, at 3:53 PM, Andrii Nakryiko <[email protected]> wrote:
> >
> > On Thu, Sep 2, 2021 at 9:58 AM Song Liu <[email protected]> wrote:
> >>
> >> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
> >> branch trace from hardware (e.g. Intel LBR). To use the feature, the
> >> user need to create perf_event with proper branch_record filtering
> >> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
> >> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
> >>
> >> Signed-off-by: Song Liu <[email protected]>
> >> ---
> >> include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
> >> kernel/bpf/trampoline.c | 3 ++-
> >> kernel/trace/bpf_trace.c | 33 +++++++++++++++++++++++++++++++++
> >> tools/include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
> >> 4 files changed, 79 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >> index 791f31dd0abee..c986e6fad5bc0 100644
> >> --- a/include/uapi/linux/bpf.h
> >> +++ b/include/uapi/linux/bpf.h
> >> @@ -4877,6 +4877,27 @@ union bpf_attr {
> >> * Get the struct pt_regs associated with **task**.
> >> * Return
> >> * A pointer to struct pt_regs.
> >> + *
> >> + * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
> >> + * Description
> >> + * Get branch trace from hardware engines like Intel LBR. The
> >> + * branch trace is taken soon after the trigger point of the
> >> + * BPF program, so it may contain some entries after the
> >
> > This part is a leftover from previous design, so not relevant anymore?
>
> Hmm.. This is still relevant, but not very accurate. I guess we should
> provide more information, like "For more information about branches before
> the trigger point, this should be called early in the BPF program".
I read the part about "taken soon after the trigger point of BPF
program" as a reference to previous implementation. So maybe let's
clarify that because LBR is not frozen, from the time
bpf_get_branch_snapshot() is called to when we actually capture LBRs
we can waste few records due to internal kernel calls, so the user has
to be aware of that.
>
> Song
>
>
> >
> >> + * trigger point. The user need to filter these entries
> >> + * accordingly.
> >> + *
> >> + * The data is stored as struct perf_branch_entry into output
> >> + * buffer *entries*. *size* is the size of *entries* in bytes.
> >> + * *flags* is reserved for now and must be zero.
> >> + *
> >> + * Return
> >> + * On success, number of bytes written to *buf*. On error, a
> >> + * negative value.
> >> + *
> >> + * **-EINVAL** if arguments invalid or **size** not a multiple
> >> + * of **sizeof**\ (**struct perf_branch_entry**\ ).
> >> + *
> >> + * **-ENOENT** if architecture does not support branch records.
> >
> > [...]
>
On Thu, Sep 02, 2021 at 09:57:05AM -0700, Song Liu wrote:
> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
> +{
> +#ifndef CONFIG_X86
> + return -ENOENT;
> +#else
> + static const u32 br_entry_size = sizeof(struct perf_branch_entry);
> + u32 entry_cnt = size / br_entry_size;
> +
> + if (unlikely(flags))
> + return -EINVAL;
> +
> + if (!buf || (size % br_entry_size != 0))
> + return -EINVAL;
> +
> + entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
> +
> + if (!entry_cnt)
> + return -ENOENT;
> +
> + return entry_cnt * br_entry_size;
> +#endif
> +}
Do we really need that CONFIG_X86 thing? Seems rather bad practise.
On Thu, Sep 02, 2021 at 09:57:05AM -0700, Song Liu wrote:
> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
> +{
> + static const u32 br_entry_size = sizeof(struct perf_branch_entry);
> + u32 entry_cnt = size / br_entry_size;
> +
> + if (unlikely(flags))
> + return -EINVAL;
> +
> + if (!buf || (size % br_entry_size != 0))
> + return -EINVAL;
> +
> + entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
That's at least 2, possibly 3 branches just from the sanity checks, plus
at least one from starting the BPF prog and one from calling this
function, gets you at ~5 branch entries gone before you even do the
snapshot thing.
Less if you're in branch-stack mode.
Can't the validator help with getting rid of the some of that?
I suppose you have to have this helper function because the JIT cannot
emit static_call()... although in this case one could cheat and simply
emit a call to static_call_query() and not bother with dynamic updates
(because there aren't any).
On Fri, Sep 3, 2021 at 1:49 AM Peter Zijlstra <[email protected]> wrote:
>
> On Thu, Sep 02, 2021 at 09:57:05AM -0700, Song Liu wrote:
> > +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
> > +{
> > + static const u32 br_entry_size = sizeof(struct perf_branch_entry);
> > + u32 entry_cnt = size / br_entry_size;
> > +
> > + if (unlikely(flags))
> > + return -EINVAL;
> > +
> > + if (!buf || (size % br_entry_size != 0))
I think buf can't be NULL, this should be enforced already by verifier
due to ARG_PTR_TO_UNINIT_MEM, so we can drop that.
> > + return -EINVAL;
> > +
> > + entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
>
> That's at least 2, possibly 3 branches just from the sanity checks, plus
> at least one from starting the BPF prog and one from calling this
> function, gets you at ~5 branch entries gone before you even do the
> snapshot thing.
>
> Less if you're in branch-stack mode.
>
> Can't the validator help with getting rid of the some of that?
>
Good points. I think we can drop !buf check completely. The flags and
size checks can be moved after perf_snapshot_branch_stack call. In
common cases those checks should always pass, but even if they don't
we'll just capture the LBR anyways, but will return an error later,
which seems totally acceptable.
As Alexei mentioned, there is a whole non-inlined migrate_disable()
call before this, which we should inline as well. It's good for
performance, not just LBR.
> I suppose you have to have this helper function because the JIT cannot
> emit static_call()... although in this case one could cheat and simply
> emit a call to static_call_query() and not bother with dynamic updates
> (because there aren't any).
If that's safe, let's do it.
> On Sep 3, 2021, at 1:28 AM, Peter Zijlstra <[email protected]> wrote:
>
> On Thu, Sep 02, 2021 at 09:57:05AM -0700, Song Liu wrote:
>> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
>> +{
>> +#ifndef CONFIG_X86
>> + return -ENOENT;
>> +#else
>> + static const u32 br_entry_size = sizeof(struct perf_branch_entry);
>> + u32 entry_cnt = size / br_entry_size;
>> +
>> + if (unlikely(flags))
>> + return -EINVAL;
>> +
>> + if (!buf || (size % br_entry_size != 0))
>> + return -EINVAL;
>> +
>> + entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
>> +
>> + if (!entry_cnt)
>> + return -ENOENT;
>> +
>> + return entry_cnt * br_entry_size;
>> +#endif
>> +}
>
> Do we really need that CONFIG_X86 thing? Seems rather bad practise.
The ifndef will save a few cycles on architectures that do not support
branch stack. I personally don't have very strong preference on either
way.
Thanks,
Song
> On Sep 3, 2021, at 1:47 AM, Peter Zijlstra <[email protected]> wrote:
>
> On Thu, Sep 02, 2021 at 09:57:05AM -0700, Song Liu wrote:
>> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
>> +{
>> + static const u32 br_entry_size = sizeof(struct perf_branch_entry);
>> + u32 entry_cnt = size / br_entry_size;
>> +
>> + if (unlikely(flags))
>> + return -EINVAL;
>> +
>> + if (!buf || (size % br_entry_size != 0))
>> + return -EINVAL;
>> +
>> + entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
>
> That's at least 2, possibly 3 branches just from the sanity checks, plus
> at least one from starting the BPF prog and one from calling this
> function, gets you at ~5 branch entries gone before you even do the
> snapshot thing.
Let me try to shuffle the function and get rid of some of these checks.
>
> Less if you're in branch-stack mode.
>
> Can't the validator help with getting rid of the some of that?
>
> I suppose you have to have this helper function because the JIT cannot
> emit static_call()... although in this case one could cheat and simply
> emit a call to static_call_query() and not bother with dynamic updates
> (because there aren't any).
We only JIT some key helper functions. I didn't think about that because
current version is OK for mainstream and future hardware. I guess we
can try JIT if it turns out some architecture needs more optimization.
Thanks,
Song
On Fri, Sep 03, 2021 at 10:10:16AM -0700, Andrii Nakryiko wrote:
> > I suppose you have to have this helper function because the JIT cannot
> > emit static_call()... although in this case one could cheat and simply
> > emit a call to static_call_query() and not bother with dynamic updates
> > (because there aren't any).
>
> If that's safe, let's do it.
I'll try and remember to look into static_call_lock(), a means of
forever denying future static_call_update() calls. That should make this
more obvious.
On Sat, Sep 04, 2021 at 12:24:30PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 03, 2021 at 10:10:16AM -0700, Andrii Nakryiko wrote:
> > > I suppose you have to have this helper function because the JIT cannot
> > > emit static_call()... although in this case one could cheat and simply
> > > emit a call to static_call_query() and not bother with dynamic updates
> > > (because there aren't any).
> >
> > If that's safe, let's do it.
>
> I'll try and remember to look into static_call_lock(), a means of
> forever denying future static_call_update() calls. That should make this
> more obvious.
A little something like so I suppose.... we don't really have spare
bits in the !INLINE case :/
---
diff --git a/include/linux/static_call.h b/include/linux/static_call.h
index 3e56a9751c06..b0feccd56d37 100644
--- a/include/linux/static_call.h
+++ b/include/linux/static_call.h
@@ -174,6 +174,10 @@ struct static_call_tramp_key {
s32 key;
};
+extern void __static_call_lock(struct static_call_key *key);
+
+#define static_call_lock(name) __static_call_lock(&STATIC_CALL_KEY(name))
+
extern void __static_call_update(struct static_call_key *key, void *tramp, void *func);
extern int static_call_mod_init(struct module *mod);
extern int static_call_text_reserved(void *start, void *end);
@@ -215,6 +219,8 @@ extern long __static_call_return0(void);
#elif defined(CONFIG_HAVE_STATIC_CALL)
+#define static_call_lock(name)
+
static inline int static_call_init(void) { return 0; }
#define __DEFINE_STATIC_CALL(name, _func, _func_init) \
@@ -268,6 +274,8 @@ static inline long __static_call_return0(void)
#else /* Generic implementation */
+#define static_call_lock(name)
+
static inline int static_call_init(void) { return 0; }
static inline long __static_call_return0(void)
diff --git a/include/linux/static_call_types.h b/include/linux/static_call_types.h
index 5a00b8b2cf9f..e40a3b595c4a 100644
--- a/include/linux/static_call_types.h
+++ b/include/linux/static_call_types.h
@@ -62,6 +62,7 @@ struct static_call_key {
void *func;
union {
/* bit 0: 0 = mods, 1 = sites */
+ /* but 1: locked */
unsigned long type;
struct static_call_mod *mods;
struct static_call_site *sites;
diff --git a/kernel/static_call.c b/kernel/static_call.c
index 43ba0b1e0edb..a1ba93fbad29 100644
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -104,6 +104,11 @@ static inline bool static_call_key_has_mods(struct static_call_key *key)
return !(key->type & 1);
}
+static inline bool static_call_key_is_locked(struct static_call_key *key)
+{
+ return !!(key->type & 2);
+}
+
static inline struct static_call_mod *static_call_key_next(struct static_call_key *key)
{
if (!static_call_key_has_mods(key))
@@ -117,7 +122,7 @@ static inline struct static_call_site *static_call_key_sites(struct static_call_
if (static_call_key_has_mods(key))
return NULL;
- return (struct static_call_site *)(key->type & ~1);
+ return (struct static_call_site *)(key->type & ~3);
}
void __static_call_update(struct static_call_key *key, void *tramp, void *func)
@@ -125,6 +130,9 @@ void __static_call_update(struct static_call_key *key, void *tramp, void *func)
struct static_call_site *site, *stop;
struct static_call_mod *site_mod, first;
+ if (WARN_ON_ONCE(static_call_key_is_locked(key)))
+ return;
+
cpus_read_lock();
static_call_lock();
@@ -418,6 +426,18 @@ static void static_call_del_module(struct module *mod)
}
}
+void __static_call_lock(struct static_call_key *key)
+{
+ cpus_read_lock();
+ static_call_lock();
+
+ WARN_ON_ONCE(static_call_key_is_locked(key));
+ key->type |= 2;
+
+ static_call_unlock();
+ cpus_read_unlock();
+}
+
static int static_call_module_notify(struct notifier_block *nb,
unsigned long val, void *data)
{