[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Alexei Starovoitov via Phabricator via cfe-commits]

ast added a comment.

In D133361#4637008 , @eddyz87 wrote:

> In D133361#4629292 , @ast wrote:
>
>> ...
>> Right. Such recursive propagation of PAI is necessary. For btf_tag we cannot 
>> do it. Always propagating it won't be correct.
>> New preserve_const_field_offset would need to be propagated too and we 
>> actually have nested unions.
>> __bpf_md_ptr is such example. btf_tag wouldn't propagate into that union, 
>> but attr(preserve_const_field_offset) should.
>
> Hi Alexei,
>
> It just occurred to me that such an attribute would also require DWARF and 
> BTF encoding in order to get reflected in vmlinux.h (which we already have 
> for btf_decl_tag). Given this I think we can rename decl tag "ctx" to 
> `btf_decl_tag("preserve_const_field_offset")` but we should still keep it a 
> `btf_decl_tag`. I'll try to replace usage of `bpf_context_marker` intrinsic 
> by metadata, if that fails will just rename the intrinsic to 
> `preserve_const_field_offset`.
> What do you think? (Sorry, I should have thought this through last week).

I still feel that new attr is much cleaner from llvm implementation/design 
perspective and vmlinux.h inconvenience should be low priority in this 
considerations.
Since ctx only applies to uapi/bpf.h header the users don't have to use 
vmlinux.h. I know that today we have pains combining uapi headers and vmlinux.h 
and several solutions were
proposed. None were accepted yet, but that shouldn't mean that we should 
sacrifice llvm implementation due to orthogonal issues.
As a temporary workaround for vmlinux.h we can have uapi/bpf.h to do 
attr(preserve_const_field_offset) _and_ btf_decl_tag("bpf_ctx_struct"). Then 
teach pahole to emit attr(preserve_const_field_offset)
when it sees btf_decl_tag("bpf_ctx_struct") in vmlinux BTF.
Other workarounds are possible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D133361#4629292 , @ast wrote:

> ...
> Right. Such recursive propagation of PAI is necessary. For btf_tag we cannot 
> do it. Always propagating it won't be correct.
> New preserve_const_field_offset would need to be propagated too and we 
> actually have nested unions.
> __bpf_md_ptr is such example. btf_tag wouldn't propagate into that union, but 
> attr(preserve_const_field_offset) should.

Hi Alexei,

It just occurred to me that such an attribute would also require DWARF and BTF 
encoding in order to get reflected in vmlinux.h (which we already have for 
btf_decl_tag). Given this I think we can rename decl tag "ctx" to 
`btf_decl_tag("preserve_const_field_offset")` but we should still keep it a 
`btf_decl_tag`. I'll try to replace usage of `bpf_context_marker` intrinsic by 
metadata, if that fails will just rename the intrinsic to 
`preserve_const_field_offset`.
What do you think? (Sorry, I should have thought this through last week).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

After retesting kernel build with LLVM=1 and libbpf patch 
 to reconstruct 
btf_decl_tag [1], statistics for BPF selftests looks as follows:

- out of 653 object files 13 have some differences with and w/o this change;
- for 2 programs there is small instruction count increase (+2 insn total);
- for 5 programs there is small instruction decrease (-6 insn total);
- 6 programs differ slightly but number of instructions is the same.

(The differences are insignificant, the rest of the comment could be skipped as 
it is probably not interesting for anyone but me).

---

Differences for first 5 programs were already described in this 
 comment. The rest of the differences 
in described below.

netns_cookie_prog.bpf.o
---

Without ctx: 46 instructions
With ctx: 46 instructions

Instruction reordering:

   :
r6 = r1
  - r2 = *(u64 *)(r6 + 0x48)
r1 = *(u32 *)(r6 + 0x10)
  - if w1 != 0xa goto +0xb 
  + if w1 != 0xa goto +0xc 
  + r2 = *(u64 *)(r6 + 0x48)
if r2 == 0x0 goto +0xa 
r1 = 0x0 ll
r3 = 0x0

The difference is introduced by "Machine code sinking" transformation. Before 
the transformation both 0x48 and 0x10 loads reside in the same basic block:

  ;; Old:
  bb.0.entry:
...
%0:gpr = CORE_LD64 345, %2:gpr, @"llvm.sk_msg_md:0:72$0:10:0"
%9:gpr32 = CORE_LD32 350, %2:gpr, @"llvm.sk_msg_md:0:16$0:2"
JNE_ri_32 killed %9:gpr32, 10, %bb.3
  
  ;; New:
  bb.0.entry:
...
%0:gpr = LDD %2:gpr, 72
%3:gpr32 = LDW32 %2:gpr, 16
JNE_ri_32 killed %3:gpr32, 10, %bb.3

Note: CORE pseudo-instructions are replaced by regular loads because 
btf_decl_tag("ctx") has priority over preserve_access_index attribute. The 
"Machine code sinking" transformation (MachineSink.cpp) can move `LDD`, `LDW` 
instructions, but can't move `CORE_LD*` because `CORE_LD*` instructions are 
marked as `MCID::UnmodeledSideEffects` in `BPFGenInstrInfo.inc` (maybe 
something to adjust):

  // called from MachineSinking::SinkInstruction
  bool MachineInstr::isSafeToMove(AAResults *AA, bool ) const {
if (... hasUnmodeledSideEffects())
  return false;
...
  }



sock_destroy_prog.bpf.o
---

Without ctx: 102 instructions
With ctx: 101 instructions

In the following code fragment:

if (ctx->protocol == IPPROTO_TCP)
bpf_map_update_elem(_conn_sockets, , _cookie, 0);
else if (ctx->protocol == IPPROTO_UDP)
bpf_map_update_elem(_conn_sockets, , _cookie, 0);
else
return 1;

Version w/o btf_decl_tag("ctx") keeps two loads for `ctx->protocol` because of 
the llvm.bpf.passthrough call. Version with btf_decl_tag("ctx") eliminates 
second load via a combination of EarlyCSEPass/InstCombinePass/SimplifyCFGPass 
passes.

socket_cookie_prog.bpf.o


Without ctx: 66 instructions
With ctx: 66 instructions

For the following C code fragment:

  SEC("sockops")
  int update_cookie_sockops(struct bpf_sock_ops *ctx)
  {
struct bpf_sock *sk = ctx->sk;
struct socket_cookie *p;
  
if (ctx->family != AF_INET6)
return 1;
  
if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
return 1;
  
if (!sk)
return 1;
  ...
  }

Code with decl_tag("ctx") does reordering for ctx->sk load relative to 
ctx->family and ctx->op loads:

  //  old   new
   :  :
  r6 = r1   r6 = r1
  -   r2 = *(u64 *)(r6 + 0xb8)
  r1 = *(u32 *)(r6 + 0x14)  r1 = *(u32 *)(r6 + 0x14)
  if w1 != 0xa goto +0x13   if w1 != 0xa goto +0x14 
  r1 = *(u32 *)(r6 + 0x0)   r1 = *(u32 *)(r6 + 0x0)
  if w1 != 0x3 goto +0x11   if w1 != 0x3 goto +0x12 
+   r2 = *(u64 *)(r6 + 0xb8)
  if r2 == 0x0 goto +0x10   if r2 == 0x0 goto +0x10 
  r1 = 0x0 ll   r1 = 0x0 ll
  r3 = 0x0  r3 = 0x0

Code w/o decl_tag("ctx") uses `CORE_LD*` instructions for these loads and does 
not reorder loads due to reasons as in netns_cookie_prog.bpf.o.

test_lwt_reroute.bpf.o
--

Without ctx: 18 instructions
With ctx: 17 instructions

The difference boils down EarlyCSEPass being able to remove last load in the 
store/load pair:

  llvm
  ; Before EarlyCSEPass
store i32 %and, ptr %mark24, align 8
%mark25 = getelementptr inbounds %struct.__sk_buff, ptr %skb, i32 0, i32 2
%19 = load i32, ptr %mark25, align 8
%cmp26 = icmp eq i32 %19, 0
  ; After EarlyCSEPass
%and = and i32 %cond, 255
store i32 %and, ptr %mark, align 8
%cmp26 = icmp eq i32 %and, 0

And unable to do so when get.element.and.{store,load} intrinsics are used. 

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Alexei Starovoitov via Phabricator via cfe-commits]

ast added a comment.

In D133361#4629081 , @eddyz87 wrote:

> Note on current propagation logic: whenever there is an expression E of type 
> T, where T is a struct annotated with btf_decl_tag("ctx"), all usages of  E 
> are traversed recursively visiting `getelementptr` and calls to 
> `preserve_{struct,union,array}_index`, the latter are replaced by 
> `getelementptr`. E.g.:
>
>   #define __pai __attribute__((preserve_access_context));
>   #define __ctx __attribute__((btf_decl_tag("ctx")))
>   struct bar { int a; int b; } __pai;
>   struct foo {
> struct bar b;
>   } __ctx __pai;
>   ... struct foo f; ...
>   ... f.b.bb ...
>
> The call to `preserve_struct_index` generated for `bb` in `f.b.bb` would be 
> replaced by `getelementptr` and later by `getelementptr.and.load`.
> However, context structures don't have nested structures at the moment. The 
> list of context structures is:
>
> - __sk_buff
> - bpf_cgroup_dev_ctx
> - bpf_sk_lookup
> - bpf_sock
> - bpf_sock_addr
> - bpf_sock_ops
> - bpf_sockopt
> - bpf_sysctl
> - sk_msg_md
> - sk_reuseport_md
> - xdp_md

Right. Such recursive propagation of PAI is necessary. For btf_tag we cannot do 
it. Always propagating it won't be correct.
New preserve_const_field_offset would need to be propagated too and we actually 
have nested unions.
__bpf_md_ptr is such example. btf_tag wouldn't propagate into that union, but 
attr(preserve_const_field_offset) should.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D133361#4628951 , @ast wrote:

> I agree that it's not useful outside of BPF, but it's useful outside of 
> 'ctx'. I think 'preserve_constant_field_offset' would be more accurate 
> description of the restriction.
> We can expand in the doc that it's a constant offset when field of the struct 
> is accessed.
>
> Also instead of btf_tag it would be better to add another builtin similar to 
> preserve_access_index.
> Currently we add __attribute__((preserve_access_index)) to trigger CO-RE.
> This one will be a new __attribute__((preserve_constant_field_offset)) that 
> will be specified manually either in uapi/bpf.h or in vmlinux.h on some 
> structs

This makes sense, I'll adjust the implementation to use new attribute (also 
will try to use metadata instead of intrinsic call, replace by intrinsic on the 
back-end side).

> and it will have precedence over preserve_access_index, so
> (__attribute__((preserve_access_index)), apply_to = record) in vmlinux.h will 
> be ignored on such structs.
> Otherwise it's a bit odd that special names inside btf_tag have stronger 
> rules than other __attribute__((preserve_access_index)).

Note on current propagation logic: whenever there is an expression E of type T, 
where T is a struct annotated with btf_decl_tag("ctx"), all usages of  E are 
traversed recursively visiting `getelementptr` and calls to 
`preserve_{struct,union,array}_index`, the latter are replaced by 
`getelementptr`. E.g.:

  #define __pai __attribute__((preserve_access_context));
  #define __ctx __attribute__((btf_decl_tag("ctx")))
  struct bar { int a; int b; } __pai;
  struct foo {
struct bar b;
  } __ctx __pai;
  ... struct foo f; ...
  ... f.b.bb ...

The call to `preserve_struct_index` generated for `bb` in `f.b.bb` would be 
replaced by `getelementptr` and later by `getelementptr.and.load`.
However, context structures don't have nested structures at the moment. The 
list of context structures is:

- __sk_buff
- bpf_cgroup_dev_ctx
- bpf_sk_lookup
- bpf_sock
- bpf_sock_addr
- bpf_sock_ops
- bpf_sockopt
- bpf_sysctl
- sk_msg_md
- sk_reuseport_md
- xdp_md


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Alexei Starovoitov via Phabricator via cfe-commits]

ast added a comment.

> I can rename these things, but tbh I don't think this functionality would be 
> useful anywhere outside BPF, thus such renaming would be kind-of deceptive 
> (and in case it would be useful, the renaming could be done at the time of 
> second use).

I agree that it's not useful outside of BPF, but it's useful outside of 'ctx'. 
I think 'preserve_constant_field_offset' would be more accurate description of 
the restriction.
We can expand in the doc that it's a constant offset when field of the struct 
is accessed.

Also instead of btf_tag it would be better to add another builtin similar to 
preserve_access_index.
Currently we add __attribute__((preserve_access_index)) to trigger CO-RE.
This one will be a new __attribute__((preserve_constant_field_offset)) that 
will be specified manually either in uapi/bpf.h or in vmlinux.h on some structs
and it will have precedence over preserve_access_index, so
(__attribute__((preserve_access_index)), apply_to = record) in vmlinux.h will 
be ignored on such structs.
Otherwise it's a bit odd that special names inside btf_tag have stronger rules 
than other __attribute__((preserve_access_index)).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

> I can rename these things, but tbh I don't think this functionality would be 
> useful anywhere outside BPF, thus such renaming would be kind-of deceptive 
> (and in case it would be useful, the renaming could be done at the time of 
> second use).

Then let us keep this for now. We can decide what to do after clang frontend 
people reviewed this code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:2432
   ImmArg>]>;
+def int_context_marker_bpf : DefaultAttrsIntrinsic<[llvm_ptr_ty],
+   [llvm_ptr_ty],

yonghong-song wrote:
> eddyz87 wrote:
> > yonghong-song wrote:
> > > Is it possible to make this builtin as BPF specific one?
> > Currently `llvm::Intrinsic::context_marker_bpf` gets defined in 
> > `llvm/IR/Intrinsics.h` (via include of generated file `IntrinsicEnums.inc`, 
> > same as `preserve_{struct,union,array}_access_index`).
> > 
> > BPF specific intrinsics are defined in `llvm/IR/IntrinsicsBPF.h` (generated 
> > directly w/o .inc intermediary).
> > 
> > Thus, if I move `context_marker_bpf` to `IntrinsicsBPF.td` I would have to 
> > include `IntrinsicsBPF.h` in `CGExpr.cpp`. However, I don't see any target 
> > specific includes in that file.
> > 
> I went through the related clang code and indeed found it is hard to get a 
> BPF target defined function in CGF or CGM. On the other hand, we can consider 
> this new builtin under the umbrella "Intrinsics that are used to preserve 
> debug information".  Maybe we can rename the intrinsic name
> to 'int_preserve_context_marker'? The goal of this builtin to preserve 
> certain load/store which should be immune from optimizations. I try to 
> generalize this so your function name 'wrapWithBPFContextMarker' can be 
> renamed to
> 'wrapWithContextMarker'. There is no need to mention BPF any more.
> 
> In the commit message, you can mention that
> similar to int_preserve_array_access_index, int_preserve_context_marker is 
> only implemented in BPF backend. But other architectures can implement 
> processing these intrinsics if they want to achieve some results similar to 
> bpf backend.
> 
> WDYT? 
I can rename these things, but tbh I don't think this functionality would be 
useful anywhere outside BPF, thus such renaming would be kind-of deceptive (and 
in case it would be useful, the renaming could be done at the time of second 
use).

---

Something more generic might look like `!btf_decl_tag ` metadata 
node attached to something. However, in the current form this would require 
transfer of such decl tag from type to function parameters and variables, e.g.:

```
lang=c
struct foo {  ... } __attribute__((btf_decl_tag("ctx")));
void bar(struct foo *p) { ... }
```

During code-gen for `bar` use rule like "if function parameter has type 
annotated with btf_decl_tag, attach metadata to such parameter":

```
lang=llvm
define void @bar(ptr %p !btf_decl_tag !1) { ... }
!1 = { "ctx" }
```

Such rule looks a bit weird:
- tag is transferred from type to it's usage
- what usages should be annotated? we care about function parameters but from 
generic point of view `alloca`s or field accesses should be annotated as well.

---

The same metadata approach but with "ctx" attributes annotating function 
parameters (as you suggested originally, if I recall correctly) seems to be 
most generic and least controversial of all, e.g.:

```
lang=c
void bar(struct foo *p __attribute__((btf_decl_tag("ctx" { ... }
```

Converted to:

```
lang=llvm
define void @bar(ptr %p !btf_decl_tag !1) { ... }
!1 = { "ctx" }
```

However, this is less ergonomic for BPF, because user will have to annotate 
function parameters. (On the other hand, no changes in the kernel headers would 
be necessary).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

I will try to review other parts of code in the next few days.




Comment at: llvm/include/llvm/IR/Intrinsics.td:2432
   ImmArg>]>;
+def int_context_marker_bpf : DefaultAttrsIntrinsic<[llvm_ptr_ty],
+   [llvm_ptr_ty],

eddyz87 wrote:
> yonghong-song wrote:
> > Is it possible to make this builtin as BPF specific one?
> Currently `llvm::Intrinsic::context_marker_bpf` gets defined in 
> `llvm/IR/Intrinsics.h` (via include of generated file `IntrinsicEnums.inc`, 
> same as `preserve_{struct,union,array}_access_index`).
> 
> BPF specific intrinsics are defined in `llvm/IR/IntrinsicsBPF.h` (generated 
> directly w/o .inc intermediary).
> 
> Thus, if I move `context_marker_bpf` to `IntrinsicsBPF.td` I would have to 
> include `IntrinsicsBPF.h` in `CGExpr.cpp`. However, I don't see any target 
> specific includes in that file.
> 
I went through the related clang code and indeed found it is hard to get a BPF 
target defined function in CGF or CGM. On the other hand, we can consider this 
new builtin under the umbrella "Intrinsics that are used to preserve debug 
information".  Maybe we can rename the intrinsic name
to 'int_preserve_context_marker'? The goal of this builtin to preserve certain 
load/store which should be immune from optimizations. I try to generalize this 
so your function name 'wrapWithBPFContextMarker' can be renamed to
'wrapWithContextMarker'. There is no need to mention BPF any more.

In the commit message, you can mention that
similar to int_preserve_array_access_index, int_preserve_context_marker is only 
implemented in BPF backend. But other architectures can implement processing 
these intrinsics if they want to achieve some results similar to bpf backend.

WDYT? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:2432
   ImmArg>]>;
+def int_context_marker_bpf : DefaultAttrsIntrinsic<[llvm_ptr_ty],
+   [llvm_ptr_ty],

yonghong-song wrote:
> Is it possible to make this builtin as BPF specific one?
Currently `llvm::Intrinsic::context_marker_bpf` gets defined in 
`llvm/IR/Intrinsics.h` (via include of generated file `IntrinsicEnums.inc`, 
same as `preserve_{struct,union,array}_access_index`).

BPF specific intrinsics are defined in `llvm/IR/IntrinsicsBPF.h` (generated 
directly w/o .inc intermediary).

Thus, if I move `context_marker_bpf` to `IntrinsicsBPF.td` I would have to 
include `IntrinsicsBPF.h` in `CGExpr.cpp`. However, I don't see any target 
specific includes in that file.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:2432
   ImmArg>]>;
+def int_context_marker_bpf : DefaultAttrsIntrinsic<[llvm_ptr_ty],
+   [llvm_ptr_ty],

Is it possible to make this builtin as BPF specific one?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits