| Message ID | 20230509151511.3937-3-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: bpf trampoline improvements | expand |
On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector > is only used to indicate how many times the bpf trampoline image are > updated and been displayed in the trampoline ksym name. After the > trampoline is freed, the count will start from 0 again. > So the count is a useless value to the user, we'd better > show a more meaningful value like how many progs are linked to this > trampoline. After that change, the selector can be removed eventally. > If the user want to check whether the bpf trampoline image has been updated > or not, the user can also compare the address. Each time the trampoline > image is updated, the address will change consequently. I wonder whether this will cause confusion to some users. Maybe the saving doesn't worth the churn. Song > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > include/linux/bpf.h | 1 - > kernel/bpf/trampoline.c | 7 ++----- > 2 files changed, 2 insertions(+), 6 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 456f33b..36e4b2d 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1125,7 +1125,6 @@ struct bpf_trampoline { > int progs_cnt[BPF_TRAMP_MAX]; > /* Executable image of trampoline */ > struct bpf_tramp_image *cur_image; > - u64 selector; > struct module *mod; > }; > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > index 7067cdf..be37d87 100644 > --- a/kernel/bpf/trampoline.c > +++ b/kernel/bpf/trampoline.c > @@ -410,11 +410,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut > err = unregister_fentry(tr, tr->cur_image->image); > bpf_tramp_image_put(tr->cur_image); > tr->cur_image = NULL; > - tr->selector = 0; > goto out; > } > > - im = bpf_tramp_image_alloc(tr->key, tr->selector); > + im = bpf_tramp_image_alloc(tr->key, total); > if (IS_ERR(im)) { > err = PTR_ERR(im); > goto out; > @@ -451,8 +450,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut > > set_memory_rox((long)im->image, 1); > > - WARN_ON(tr->cur_image && tr->selector == 0); > - WARN_ON(!tr->cur_image && tr->selector); > + WARN_ON(tr->cur_image && total == 0); > if (tr->cur_image) > /* progs already running at this address */ > err = modify_fentry(tr, tr->cur_image->image, im->image, lock_direct_mutex); > @@ -482,7 +480,6 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut > if (tr->cur_image) > bpf_tramp_image_put(tr->cur_image); > tr->cur_image = im; > - tr->selector++; > out: > /* If any error happens, restore previous flags */ > if (err) > -- > 1.8.3.1 > >
On Wed, May 10, 2023 at 1:43 AM Song Liu <song@kernel.org> wrote: > > On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector > > is only used to indicate how many times the bpf trampoline image are > > updated and been displayed in the trampoline ksym name. After the > > trampoline is freed, the count will start from 0 again. > > So the count is a useless value to the user, we'd better > > show a more meaningful value like how many progs are linked to this > > trampoline. After that change, the selector can be removed eventally. > > If the user want to check whether the bpf trampoline image has been updated > > or not, the user can also compare the address. Each time the trampoline > > image is updated, the address will change consequently. > > I wonder whether this will cause confusion to some users. Maybe the saving > doesn't worth the churn. The trampoline ksym name as such: ffffffffc06c3000 t bpf_trampoline_6442453466_1 [bpf] I don't know what the user may use the selector for. It seems that the selector is meaningless. While the cnt of linked progs can really help users, with which the user can easily figure out how many progs are linked to a kernel function. However the key in the ksym name really confused me before, because its meaning was changed. In the old kernel(for example, the 4.19), it is bpf_trampoline_[BTF_ID] while in the new kernel, it is bpf_trampoline_[ OBJ_ID | BTF_ID ]_[ SELECTOR ] But I think that is not a big problem, because the user tools can be changed to (key & 0x7fffffff) to make it backward compatible. Currently there's no document on the name, so we can choose what we prefer. After we doc it, we have to keep it backward compatible. BTW, I think we should add a document on the name, otherwise the user has to read the kernel code to parse it.
On Tue, May 9, 2023 at 7:56 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > On Wed, May 10, 2023 at 1:43 AM Song Liu <song@kernel.org> wrote: > > > > On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector > > > is only used to indicate how many times the bpf trampoline image are > > > updated and been displayed in the trampoline ksym name. After the > > > trampoline is freed, the count will start from 0 again. > > > So the count is a useless value to the user, we'd better > > > show a more meaningful value like how many progs are linked to this > > > trampoline. After that change, the selector can be removed eventally. > > > If the user want to check whether the bpf trampoline image has been updated > > > or not, the user can also compare the address. Each time the trampoline > > > image is updated, the address will change consequently. > > > > I wonder whether this will cause confusion to some users. Maybe the saving > > doesn't worth the churn. > > The trampoline ksym name as such: > ffffffffc06c3000 t bpf_trampoline_6442453466_1 [bpf] > > I don't know what the user may use the selector for. It seems that the > selector is meaningless. While the cnt of linked progs can really help > users, with which the user can easily figure out how many progs are > linked to a kernel function. Hmm, agreed that the chance to break user space is low. Maybe we can just remove it? IOW, only keep bpf_trampoline_6442453466 Thanks, Song
On Wed, May 10, 2023 at 2:30 PM Song Liu <song@kernel.org> wrote: > > On Tue, May 9, 2023 at 7:56 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > On Wed, May 10, 2023 at 1:43 AM Song Liu <song@kernel.org> wrote: > > > > > > On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > > > After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector > > > > is only used to indicate how many times the bpf trampoline image are > > > > updated and been displayed in the trampoline ksym name. After the > > > > trampoline is freed, the count will start from 0 again. > > > > So the count is a useless value to the user, we'd better > > > > show a more meaningful value like how many progs are linked to this > > > > trampoline. After that change, the selector can be removed eventally. > > > > If the user want to check whether the bpf trampoline image has been updated > > > > or not, the user can also compare the address. Each time the trampoline > > > > image is updated, the address will change consequently. > > > > > > I wonder whether this will cause confusion to some users. Maybe the saving > > > doesn't worth the churn. > > > > The trampoline ksym name as such: > > ffffffffc06c3000 t bpf_trampoline_6442453466_1 [bpf] > > > > I don't know what the user may use the selector for. It seems that the > > selector is meaningless. While the cnt of linked progs can really help > > users, with which the user can easily figure out how many progs are > > linked to a kernel function. > > Hmm, agreed that the chance to break user space is low. Maybe we can just > remove it? IOW, only keep bpf_trampoline_6442453466 > Agree. I will do it as you suggested.
On Wed, May 10, 2023 at 11:33:21PM +0800, Yafang Shao wrote: > On Wed, May 10, 2023 at 2:30 PM Song Liu <song@kernel.org> wrote: > > > > On Tue, May 9, 2023 at 7:56 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > On Wed, May 10, 2023 at 1:43 AM Song Liu <song@kernel.org> wrote: > > > > > > > > On Tue, May 9, 2023 at 8:15 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > > > > > After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector > > > > > is only used to indicate how many times the bpf trampoline image are > > > > > updated and been displayed in the trampoline ksym name. After the > > > > > trampoline is freed, the count will start from 0 again. > > > > > So the count is a useless value to the user, we'd better > > > > > show a more meaningful value like how many progs are linked to this > > > > > trampoline. After that change, the selector can be removed eventally. > > > > > If the user want to check whether the bpf trampoline image has been updated > > > > > or not, the user can also compare the address. Each time the trampoline > > > > > image is updated, the address will change consequently. > > > > > > > > I wonder whether this will cause confusion to some users. Maybe the saving > > > > doesn't worth the churn. > > > > > > The trampoline ksym name as such: > > > ffffffffc06c3000 t bpf_trampoline_6442453466_1 [bpf] > > > > > > I don't know what the user may use the selector for. It seems that the > > > selector is meaningless. While the cnt of linked progs can really help > > > users, with which the user can easily figure out how many progs are > > > linked to a kernel function. > > > > Hmm, agreed that the chance to break user space is low. Maybe we can just > > remove it? IOW, only keep bpf_trampoline_6442453466 > > > > Agree. I will do it as you suggested. perf is actually is still trying parse the old name /* .. and only for trampolines and dispatchers */ if ((sscanf(name, "bpf_trampoline_%lu", &id) == 1) || (sscanf(name, "bpf_dispatcher_%s", disp) == 1)) err = process_bpf_image(name, start, data); so this change would actualy fix it ;-) thanks, jirka
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 456f33b..36e4b2d 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1125,7 +1125,6 @@ struct bpf_trampoline { int progs_cnt[BPF_TRAMP_MAX]; /* Executable image of trampoline */ struct bpf_tramp_image *cur_image; - u64 selector; struct module *mod; }; diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index 7067cdf..be37d87 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -410,11 +410,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut err = unregister_fentry(tr, tr->cur_image->image); bpf_tramp_image_put(tr->cur_image); tr->cur_image = NULL; - tr->selector = 0; goto out; } - im = bpf_tramp_image_alloc(tr->key, tr->selector); + im = bpf_tramp_image_alloc(tr->key, total); if (IS_ERR(im)) { err = PTR_ERR(im); goto out; @@ -451,8 +450,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut set_memory_rox((long)im->image, 1); - WARN_ON(tr->cur_image && tr->selector == 0); - WARN_ON(!tr->cur_image && tr->selector); + WARN_ON(tr->cur_image && total == 0); if (tr->cur_image) /* progs already running at this address */ err = modify_fentry(tr, tr->cur_image->image, im->image, lock_direct_mutex); @@ -482,7 +480,6 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut if (tr->cur_image) bpf_tramp_image_put(tr->cur_image); tr->cur_image = im; - tr->selector++; out: /* If any error happens, restore previous flags */ if (err)
After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector is only used to indicate how many times the bpf trampoline image are updated and been displayed in the trampoline ksym name. After the trampoline is freed, the count will start from 0 again. So the count is a useless value to the user, we'd better show a more meaningful value like how many progs are linked to this trampoline. After that change, the selector can be removed eventally. If the user want to check whether the bpf trampoline image has been updated or not, the user can also compare the address. Each time the trampoline image is updated, the address will change consequently. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- include/linux/bpf.h | 1 - kernel/bpf/trampoline.c | 7 ++----- 2 files changed, 2 insertions(+), 6 deletions(-)