Re: [PATCH] tools/runqslower: use __state instead of state

On Tue, Jul 6, 2021 at 4:11 PM Song Liu [off-list ref] wrote:

On Tue, Jul 6, 2021 at 3:05 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Tue, Jul 6, 2021 at 11:26 AM Yonghong Song [off-list ref] wrote:

quoted

On 7/6/21 10:44 AM, SanjayKumar J wrote:

quoted

      task->state is renamed to task->__state in task_struct

Could you add a reference to
   2f064a59a11f ("sched: Change task_struct::state")
which added this change?

I think this should go to bpf tree as the change is in linus tree now.
Could you annotate the tag as "[PATCH bpf]" ("[PATCH bpf v2]")?

Please align comments to the left without margins.

quoted

      Signed-off-by: SanjayKumar J [off-list ref]

This Singed-off-by is not needed.

You can add my Ack in the next revision:
Acked-by: Yonghong Song <redacted>

quoted

Signed-off-by: SanjayKumar J <redacted>
---
  tools/bpf/runqslower/runqslower.bpf.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/bpf/runqslower/runqslower.bpf.c b/tools/bpf/runqslower/runqslower.bpf.c
index 645530ca7e98..ab9353f2fd46 100644
--- a/tools/bpf/runqslower/runqslower.bpf.c
+++ b/tools/bpf/runqslower/runqslower.bpf.c

@@ -74,7 +74,7 @@ int handle__sched_switch(u64 *ctx)
      u32 pid;

      /* ivcsw: treat like an enqueue event and store timestamp */
-     if (prev->state == TASK_RUNNING)
+     if (prev->__state == TASK_RUNNING)

Currently, runqslower.bpf.c uses vmlinux.h.
I am thinking to use bpf_core_field_exists(), but we need to
single out task_struct structure from vmlinux.h
with both state and __state fields, we could make it work
by *changes* like

#define task_struct task_struct_orig
#include "vmlinux.h"
#undef task_struct

struct task_struct {
    ... state;
    ... __state;
...
};

no need for such surgery, recommended way is to use ___suffix to
declare incompatible struct definition:

struct task_struct___old {
    int state;
};

Then do casting in BPF code. We don't have to do it in kernel tree's
runqslower, but we'll definitely have to do that for libbpf-tools'
runqslower and runqlat.

Question on this topic: state and __state are of different sizes here. IIUC,
bpf_core_types_are_compat() does allow size mismatch. But it may cause
problems in some cases, no? For example, would some combination make
task->state return 32 extra bits from another field and cause confusion?

In this case it's two different fields, long state vs int __state, so
there is no confusion, you'd be using either one or another. But even
if it was the same field and its type changed from long to int, libbpf
will still try to accommodate that. Worst case,
BPF_CORE_READ_BITFIELD() is able to read any bitfield or integer
field, regardless of its size.

Thanks,
Song