* [PATCH] powerpc/bpf: populate extable entries only during the last pass
@ 2023-04-06 7:35 Hari Bathini
2023-04-07 6:01 ` Christophe Leroy
0 siblings, 1 reply; 5+ messages in thread
From: Hari Bathini @ 2023-04-06 7:35 UTC (permalink / raw)
To: linuxppc-dev, bpf
Cc: Michael Ellerman, Naveen N. Rao, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Song Liu, Christophe Leroy
Since commit 85e031154c7c ("powerpc/bpf: Perform complete extra passes
to update addresses"), two additional passes are performed to avoid
space and CPU time wastage on powerpc. But these extra passes led to
WARN_ON_ONCE() hits in bpf_add_extable_entry(). Fix it by not adding
extable entries during the extra pass.
Fixes: 85e031154c7c ("powerpc/bpf: Perform complete extra passes to update addresses")
Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
---
arch/powerpc/net/bpf_jit_comp32.c | 2 +-
arch/powerpc/net/bpf_jit_comp64.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
index 7f91ea064c08..e788b1fbeee6 100644
--- a/arch/powerpc/net/bpf_jit_comp32.c
+++ b/arch/powerpc/net/bpf_jit_comp32.c
@@ -977,7 +977,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
if (size != BPF_DW && !fp->aux->verifier_zext)
EMIT(PPC_RAW_LI(dst_reg_h, 0));
- if (BPF_MODE(code) == BPF_PROBE_MEM) {
+ if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) {
int insn_idx = ctx->idx - 1;
int jmp_off = 4;
diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
index 8dd3cabaa83a..1cc2777ec846 100644
--- a/arch/powerpc/net/bpf_jit_comp64.c
+++ b/arch/powerpc/net/bpf_jit_comp64.c
@@ -921,7 +921,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
if (size != BPF_DW && insn_is_zext(&insn[i + 1]))
addrs[++i] = ctx->idx * 4;
- if (BPF_MODE(code) == BPF_PROBE_MEM) {
+ if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) {
ret = bpf_add_extable_entry(fp, image, pass, ctx, ctx->idx - 1,
4, dst_reg);
if (ret)
--
2.39.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/bpf: populate extable entries only during the last pass
2023-04-06 7:35 [PATCH] powerpc/bpf: populate extable entries only during the last pass Hari Bathini
@ 2023-04-07 6:01 ` Christophe Leroy
2023-04-13 3:08 ` Hari Bathini
0 siblings, 1 reply; 5+ messages in thread
From: Christophe Leroy @ 2023-04-07 6:01 UTC (permalink / raw)
To: Hari Bathini, linuxppc-dev, bpf
Cc: Michael Ellerman, Naveen N. Rao, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Song Liu
Le 06/04/2023 à 09:35, Hari Bathini a écrit :
> Since commit 85e031154c7c ("powerpc/bpf: Perform complete extra passes
> to update addresses"), two additional passes are performed to avoid
> space and CPU time wastage on powerpc. But these extra passes led to
> WARN_ON_ONCE() hits in bpf_add_extable_entry(). Fix it by not adding
> extable entries during the extra pass.
Are you sure this change is correct ?
During the extra pass the code can get shrinked or expanded (within the
limits of the size of the preliminary pass). Shouldn't extable entries
be populated during the last pass ?
Christophe
>
> Fixes: 85e031154c7c ("powerpc/bpf: Perform complete extra passes to update addresses")
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> ---
> arch/powerpc/net/bpf_jit_comp32.c | 2 +-
> arch/powerpc/net/bpf_jit_comp64.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
> index 7f91ea064c08..e788b1fbeee6 100644
> --- a/arch/powerpc/net/bpf_jit_comp32.c
> +++ b/arch/powerpc/net/bpf_jit_comp32.c
> @@ -977,7 +977,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
> if (size != BPF_DW && !fp->aux->verifier_zext)
> EMIT(PPC_RAW_LI(dst_reg_h, 0));
>
> - if (BPF_MODE(code) == BPF_PROBE_MEM) {
> + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) {
> int insn_idx = ctx->idx - 1;
> int jmp_off = 4;
>
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> index 8dd3cabaa83a..1cc2777ec846 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -921,7 +921,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
> if (size != BPF_DW && insn_is_zext(&insn[i + 1]))
> addrs[++i] = ctx->idx * 4;
>
> - if (BPF_MODE(code) == BPF_PROBE_MEM) {
> + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) {
> ret = bpf_add_extable_entry(fp, image, pass, ctx, ctx->idx - 1,
> 4, dst_reg);
> if (ret)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/bpf: populate extable entries only during the last pass
2023-04-07 6:01 ` Christophe Leroy
@ 2023-04-13 3:08 ` Hari Bathini
2023-04-24 11:55 ` Naveen N. Rao
0 siblings, 1 reply; 5+ messages in thread
From: Hari Bathini @ 2023-04-13 3:08 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev, bpf
Cc: Song Liu, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
Naveen N. Rao
Hello Christophe,
Thanks for the review.
On 07/04/23 11:31 am, Christophe Leroy wrote:
>
>
> Le 06/04/2023 à 09:35, Hari Bathini a écrit :
>> Since commit 85e031154c7c ("powerpc/bpf: Perform complete extra passes
>> to update addresses"), two additional passes are performed to avoid
>> space and CPU time wastage on powerpc. But these extra passes led to
>> WARN_ON_ONCE() hits in bpf_add_extable_entry(). Fix it by not adding
>> extable entries during the extra pass.
>
> Are you sure this change is correct ?
Actually, I was in two minds about that owing to commit 04c04205bc35
("bpf powerpc: Remove extra_pass from bpf_jit_build_body()").
> During the extra pass the code can get shrinked or expanded (within the
> limits of the size of the preliminary pass). Shouldn't extable entries
> be populated during the last pass ?
Unlikely, but the intention there was to eliminate a regression in case
extra_pass ends up being 'false' always in any subsequent change.
- Hari
>>
>> Fixes: 85e031154c7c ("powerpc/bpf: Perform complete extra passes to update addresses")
>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>> ---
>> arch/powerpc/net/bpf_jit_comp32.c | 2 +-
>> arch/powerpc/net/bpf_jit_comp64.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
>> index 7f91ea064c08..e788b1fbeee6 100644
>> --- a/arch/powerpc/net/bpf_jit_comp32.c
>> +++ b/arch/powerpc/net/bpf_jit_comp32.c
>> @@ -977,7 +977,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>> if (size != BPF_DW && !fp->aux->verifier_zext)
>> EMIT(PPC_RAW_LI(dst_reg_h, 0));
>>
>> - if (BPF_MODE(code) == BPF_PROBE_MEM) {
>> + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) {
>> int insn_idx = ctx->idx - 1;
>> int jmp_off = 4;
>>
>> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
>> index 8dd3cabaa83a..1cc2777ec846 100644
>> --- a/arch/powerpc/net/bpf_jit_comp64.c
>> +++ b/arch/powerpc/net/bpf_jit_comp64.c
>> @@ -921,7 +921,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>> if (size != BPF_DW && insn_is_zext(&insn[i + 1]))
>> addrs[++i] = ctx->idx * 4;
>>
>> - if (BPF_MODE(code) == BPF_PROBE_MEM) {
>> + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) {
>> ret = bpf_add_extable_entry(fp, image, pass, ctx, ctx->idx - 1,
>> 4, dst_reg);
>> if (ret)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/bpf: populate extable entries only during the last pass
2023-04-13 3:08 ` Hari Bathini
@ 2023-04-24 11:55 ` Naveen N. Rao
2023-04-25 6:59 ` Hari Bathini
0 siblings, 1 reply; 5+ messages in thread
From: Naveen N. Rao @ 2023-04-24 11:55 UTC (permalink / raw)
To: bpf, Christophe Leroy, Hari Bathini, linuxppc-dev
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Song Liu
Hari Bathini wrote:
> Hello Christophe,
>
> Thanks for the review.
>
> On 07/04/23 11:31 am, Christophe Leroy wrote:
>>
>>
>> Le 06/04/2023 à 09:35, Hari Bathini a écrit :
>>> Since commit 85e031154c7c ("powerpc/bpf: Perform complete extra passes
>>> to update addresses"), two additional passes are performed to avoid
>>> space and CPU time wastage on powerpc. But these extra passes led to
>>> WARN_ON_ONCE() hits in bpf_add_extable_entry(). Fix it by not adding
>>> extable entries during the extra pass.
>>
>> Are you sure this change is correct ?
>
> Actually, I was in two minds about that owing to commit 04c04205bc35
> ("bpf powerpc: Remove extra_pass from bpf_jit_build_body()").
Right, but Christophe's series adding complete passes during the
extra_pass phase added 'extra_pass' parameter back to
bpf_jit_build_body().
>
>> During the extra pass the code can get shrinked or expanded (within the
>> limits of the size of the preliminary pass). Shouldn't extable entries
>> be populated during the last pass ?
>
> Unlikely, but the intention there was to eliminate a regression in case
> extra_pass ends up being 'false' always in any subsequent change.
But, the current approach risks generating incorrect offsets in the
extable. The main motivation for the extra pass is to generate more
compact code, so there is a good chance that offsets are going to change
(especially with bpf subprogs).
>
> - Hari
>
>>>
>>> Fixes: 85e031154c7c ("powerpc/bpf: Perform complete extra passes to update addresses")
>>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>>> ---
>>> arch/powerpc/net/bpf_jit_comp32.c | 2 +-
>>> arch/powerpc/net/bpf_jit_comp64.c | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
>>> index 7f91ea064c08..e788b1fbeee6 100644
>>> --- a/arch/powerpc/net/bpf_jit_comp32.c
>>> +++ b/arch/powerpc/net/bpf_jit_comp32.c
>>> @@ -977,7 +977,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>>> if (size != BPF_DW && !fp->aux->verifier_zext)
>>> EMIT(PPC_RAW_LI(dst_reg_h, 0));
>>>
>>> - if (BPF_MODE(code) == BPF_PROBE_MEM) {
>>> + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) {
It is probably better to pass 'extra_pass' into bpf_add_extable_entry()
to keep all those checks together.
- Naveen
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/bpf: populate extable entries only during the last pass
2023-04-24 11:55 ` Naveen N. Rao
@ 2023-04-25 6:59 ` Hari Bathini
0 siblings, 0 replies; 5+ messages in thread
From: Hari Bathini @ 2023-04-25 6:59 UTC (permalink / raw)
To: Naveen N. Rao, bpf, Christophe Leroy, linuxppc-dev
Cc: Song Liu, Andrii Nakryiko, Daniel Borkmann, Alexei Starovoitov
Hi Naveen,
On 24/04/23 5:25 pm, Naveen N. Rao wrote:
> Hari Bathini wrote:
>> Hello Christophe,
>>
>> Thanks for the review.
>>
>> On 07/04/23 11:31 am, Christophe Leroy wrote:
>>>
>>>
>>> Le 06/04/2023 à 09:35, Hari Bathini a écrit :
>>>> Since commit 85e031154c7c ("powerpc/bpf: Perform complete extra passes
>>>> to update addresses"), two additional passes are performed to avoid
>>>> space and CPU time wastage on powerpc. But these extra passes led to
>>>> WARN_ON_ONCE() hits in bpf_add_extable_entry(). Fix it by not adding
>>>> extable entries during the extra pass.
>>>
>>> Are you sure this change is correct ?
>>
>> Actually, I was in two minds about that owing to commit 04c04205bc35
>> ("bpf powerpc: Remove extra_pass from bpf_jit_build_body()").
>
> Right, but Christophe's series adding complete passes during the
> extra_pass phase added 'extra_pass' parameter back to bpf_jit_build_body().
>
>>
>>> During the extra pass the code can get shrinked or expanded (within the
>>> limits of the size of the preliminary pass). Shouldn't extable entries
>>> be populated during the last pass ?
>>
>> Unlikely, but the intention there was to eliminate a regression in case
>> extra_pass ends up being 'false' always in any subsequent change.
>
> But, the current approach risks generating incorrect offsets in the
> extable. The main motivation for the extra pass is to generate more
> compact code, so there is a good chance that offsets are going to change
> (especially with bpf subprogs).
>
>>
>> - Hari
>>
>>>>
>>>> Fixes: 85e031154c7c ("powerpc/bpf: Perform complete extra passes to
>>>> update addresses")
>>>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>>>> ---
>>>> arch/powerpc/net/bpf_jit_comp32.c | 2 +-
>>>> arch/powerpc/net/bpf_jit_comp64.c | 2 +-
>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/net/bpf_jit_comp32.c
>>>> b/arch/powerpc/net/bpf_jit_comp32.c
>>>> index 7f91ea064c08..e788b1fbeee6 100644
>>>> --- a/arch/powerpc/net/bpf_jit_comp32.c
>>>> +++ b/arch/powerpc/net/bpf_jit_comp32.c
>>>> @@ -977,7 +977,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32
>>>> *image, struct codegen_context *
>>>> if (size != BPF_DW && !fp->aux->verifier_zext)
>>>> EMIT(PPC_RAW_LI(dst_reg_h, 0));
>>>> - if (BPF_MODE(code) == BPF_PROBE_MEM) {
>>>> + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) {
>
> It is probably better to pass 'extra_pass' into bpf_add_extable_entry()
> to keep all those checks together.
>
Thanks for the review and also the suggestion (offline) to reset index
during extra pass, for my concern about possible regression. Posted v2.
- Hari
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-04-25 6:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-06 7:35 [PATCH] powerpc/bpf: populate extable entries only during the last pass Hari Bathini
2023-04-07 6:01 ` Christophe Leroy
2023-04-13 3:08 ` Hari Bathini
2023-04-24 11:55 ` Naveen N. Rao
2023-04-25 6:59 ` Hari Bathini
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).