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

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

[SanjayKumar J]

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

	Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>

Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>
---
 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)
 		trace_enqueue(prev);
 
 	pid = next->pid;
-- 
2.32.0

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

[Yonghong Song]

On 7/6/21 10:44 AM, SanjayKumar J wrote:
> 	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.

> 
> 	Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>

This Singed-off-by is not needed.

You can add my Ack in the next revision:
Acked-by: Yonghong Song <yhs@fb.com>

> 
> Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>
> ---
>   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;
...
};

Considering tools/bpf/runqslower is tied with a particular
kernel source, and vmlinux.h mostly derived from that
kernel source, I feel the above change is not necessary.

>   		trace_enqueue(prev);
>   
>   	pid = next->pid;
> 

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

[Andrii Nakryiko]

On Tue, Jul 6, 2021 at 11:26 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/6/21 10:44 AM, SanjayKumar J wrote:
> >       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.
>
> >
> >       Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>
>
> This Singed-off-by is not needed.
>
> You can add my Ack in the next revision:
> Acked-by: Yonghong Song <yhs@fb.com>
>
> >
> > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>
> > ---
> >   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.


>
> Considering tools/bpf/runqslower is tied with a particular
> kernel source, and vmlinux.h mostly derived from that
> kernel source, I feel the above change is not necessary.
>
> >               trace_enqueue(prev);
> >
> >       pid = next->pid;
> >

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

[Song Liu]

On Tue, Jul 6, 2021 at 3:05 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jul 6, 2021 at 11:26 AM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 7/6/21 10:44 AM, SanjayKumar J wrote:
> > >       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.
> >
> > >
> > >       Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>
> >
> > This Singed-off-by is not needed.
> >
> > You can add my Ack in the next revision:
> > Acked-by: Yonghong Song <yhs@fb.com>
> >
> > >
> > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>
> > > ---
> > >   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?

Thanks,
Song

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

[Andrii Nakryiko]

On Tue, Jul 6, 2021 at 4:11 PM Song Liu <song@kernel.org> wrote:
>
> On Tue, Jul 6, 2021 at 3:05 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Jul 6, 2021 at 11:26 AM Yonghong Song <yhs@fb.com> wrote:
> > >
> > >
> > >
> > > On 7/6/21 10:44 AM, SanjayKumar J wrote:
> > > >       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.
> > >
> > > >
> > > >       Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>
> > >
> > > This Singed-off-by is not needed.
> > >
> > > You can add my Ack in the next revision:
> > > Acked-by: Yonghong Song <yhs@fb.com>
> > >
> > > >
> > > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>
> > > > ---
> > > >   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

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

[Andrii Nakryiko]

On Tue, Jul 6, 2021 at 11:26 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/6/21 10:44 AM, SanjayKumar J wrote:
> >       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.
>
> >
> >       Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>
>
> This Singed-off-by is not needed.
>
> You can add my Ack in the next revision:
> Acked-by: Yonghong Song <yhs@fb.com>
>
> >
> > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>

Please use a full and proper name in Signed-off-by.

This currently breaks selftests build in bpf tree, so please
prioritize sending v2. Alternatively, we can apply Jiri's fix instead.

> > ---
> >   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;
> ...
> };
>
> Considering tools/bpf/runqslower is tied with a particular
> kernel source, and vmlinux.h mostly derived from that
> kernel source, I feel the above change is not necessary.
>
> >               trace_enqueue(prev);
> >
> >       pid = next->pid;
> >

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

[Sanjay Kumar J]

On Wed, Jul 7, 2021 at 9:53 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jul 6, 2021 at 11:26 AM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 7/6/21 10:44 AM, SanjayKumar J wrote:
> > >       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.
> >
> > >
> > >       Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>
> >
> > This Singed-off-by is not needed.
> >
> > You can add my Ack in the next revision:
> > Acked-by: Yonghong Song <yhs@fb.com>
> >
> > >
> > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com>
>
> Please use a full and proper name in Signed-off-by.
>
> This currently breaks selftests build in bpf tree, so please
> prioritize sending v2. Alternatively, we can apply Jiri's fix instead.
>
   I have send v4 .. apologies for the iterations.

> > > ---
> > >   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;
> > ...
> > };
> >
> > Considering tools/bpf/runqslower is tied with a particular
> > kernel source, and vmlinux.h mostly derived from that
> > kernel source, I feel the above change is not necessary.
> >
> > >               trace_enqueue(prev);
> > >
> > >       pid = next->pid;
> > >